Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add test files for dtb #3

Merged
merged 4 commits into from
Jul 17, 2021
Merged

add test files for dtb #3

merged 4 commits into from
Jul 17, 2021

Conversation

armijnhemel
Copy link
Contributor

No description provided.

Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already fixed everything, just please keep this in mind for future commits:

SPDX-License-Identifier: CC0-1.0
-->

## xenvm-4.2.dts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't include anything else than actual sample files to the "file listing" in README. I mean that only the sample files of the appropriate format should have the second-level headings. Most users of the sample repo will probably only need the sample files for testing purposes. Listing any accompanying files using the same heading as for samples in README would be confusing, one would sometimes hardly recognize what is actual sample file and what not.


## xenvm-4.2.dts

Source: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/xenvm-4.2.dts?id=b244131
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A commit hash shortened to 7 characters is fine for most repos (that usually have only a few hundred commits), but the Linux kernel has over a million commits, so there is a non-negligible chance that the 7-character abbreviation you pick will become ambiguous as new commits are added.

An example of an ambiguous 7-character short SHA-1 hash is mentioned in this post - this commit torvalds/linux@6d7942d references another commit via a short SHA-1 name 78a8b35 in the commit message, but https:// github.com/torvalds/linux/commit/78a8b35 will yield 404 Not Found because there are multiple commits that this reference might lead to.

So for large repos like this, it's recommended to include a bit longer hash - 12 characters should be enough.

@@ -0,0 +1 @@
SPDX-License-Identifier: GPL-2.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the CI check complains, it's necessary to include the copyright info.

Also the GPL-2.0 SPDX identifier is deprecated - GPL-2.0-only or GPL-2.0-or-later should be used instead. The fact that the original xenvm-4.2.dts file contains it is their business :-)

Linus Torvalds explicitly writes in the COPYING file that

(...) the only valid version of the GPL as far as the kernel
is concerned is this particular version of the license (ie v2, not
v2.2 or v3.x or whatever), unless explicitly otherwise stated.

so here it is clear.

Also missing licenses that have not been used before (i.e. which are not in the LICENSES/ folder yet) need to be downloaded to LICENSES/ - the command reuse download --all will resolve and download all missing licenses (and tells you what license identifiers are deprecated or invalid, if any). But I can do that in an additional commit in each PR where it occurs, no big deal.

Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@generalmimon generalmimon merged commit 5835352 into kaitai-io:master Jul 17, 2021
@generalmimon
Copy link
Member

Oh, I forgot that the file is stored in the directory system/ - this is a problem, because the idea is to follow the https://github.com/kaitai-io/kaitai_struct_formats structure so that the sample files can be easily found by the appropriate .ksy format spec. I see in kaitai-io/kaitai_struct_formats#467 that the KSY is temporarily placed into database/, but that's not a great choice because it doesn't really capture the purpose of the format. I'll move to the PR kaitai-io/kaitai_struct_formats#467 to discuss the right directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants