Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New attributes for license tag #347
base: master
Are you sure you want to change the base?
New attributes for license tag #347
Changes from all commits
b753414
ce3a38b
0efefb3
7fab8c0
2af4e80
1bd1c77
a9b3579
040c7eb
007d8b2
49c1b3a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required? Since you describe above that multiple space-separated paths are not allowed, I don't understand why we cannot support spaces in paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how the Machine-readable debian/copyright file specification V1.0 states it. We could do differently, but then we would make the process how the package.xml is translated to the copyright file less transparent. And I consider spaces in source file/folder names an absolute exception.
@ct2034 has built a linter for the new package.xml format, which will of course include a check for space characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we would simply replace space characters by
?
during the generation of the copyright file, this could even change the set of matched files. This must be prevented. Therefore I would force those developers, who want to use space characters in the filenames/paths, to think about it themselves.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to eliminate this
type
attribute. I haven't seen it in any other package managers. We can still state that the license SHOULD be an SPDX identifier (and linter can throw a warning).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is important to have as a way to suppress linter errors. In particular, I think that we should change it to be spdx is the default type. As that should be our recommendation. But if you have a non-spdx license you can set it to be "freeform" explicitly and then the linter will be suppressed and not complain about the string not matching spdx.
This will encourage spdx by it being the shortest path to usage. And if they aren't using an spdx license people can be given a linting warning. And the fix for the linter is to either fix their license declaration to be spdx compatible, or declare that it's freeform with the non-default type attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If spdx is the default that works 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also in favor of making spdx the standard. @clalancette understandably argued that this would lead to very many error messages for older packages.
Idea: We make freeform the default (as currently stated in this PR and approved by the TSC yesterday) but make a prominent info message in the linter tooling if the type-attribute is not specified explicitly and the license identifier is not from the SPDX list. Furthermore, we raise awareness for the license documentation topic by a Discourse post and a ROSCon talk. Then, in about a year, we can consider changing the default of the type attribute to SPDX (by a new PR to this REP) to force the maintainers of the remaining packages to resolve/clarify the license documentation. What do you think, @amacneil, @clalancette ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for doing a followup/tick tock process to make it the default with strong linting/warning. But lets close this out first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the way I would do this is the following:
freeform
the default for now.ament_copyright
for packages that don't specify the type and the identifier is not in SPDX.