This repository was archived by the owner on Oct 23, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 142
New attributes for license tag #347
Open
ralph-lange
wants to merge
10
commits into
ros-infrastructure:master
Choose a base branch
from
boschresearch:0149_new_license_attributes
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+95
−25
Open
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
b753414
Added attribute source-files in license tag.
ralph-lange ce3a38b
Added attribute copyright in license tag.
ralph-lange 0efefb3
Added note on the use of the new license tag attributes.
ralph-lange 7fab8c0
Removed newly proposed copyright attribute in license tag.
ralph-lange 2af4e80
Introduced type attribute for license tag.
ralph-lange 1bd1c77
Added example for use of license type attribute.
ralph-lange a9b3579
Added paragraph on limitation to one filename pattern.
ralph-lange 040c7eb
Added note on spaces in filename patterns.
ralph-lange 007d8b2
Added section on important design decisions.
ralph-lange 49c1b3a
Fixed typo in new design decisions section.
ralph-lange File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
One suggestion I have here is to add a
typeattribute to the license tag. Thetypeattribute would describe what kind of license tag is being used, either anSPDXidentifier, or afreeformidentifier. If notypeattribute is specified, we would assumefreeform.Thus you could have:
<license>BSD</license>- a common license tag today, which can be assumed to be a "freeform" license tag (and thus nothing can mechanically be determined about it)<license type='freeform'>BSD</license>- which is the same as above but more explicit<license type='spdx'>BSD-3-Clause</license>- which is very explicitly using the SPDX identifier, and thus can be mechanically verifiedWith this in place, we could put in linters (for instance) that would complain when the "type" is SPDX, but the actual text doesn't match a known SPDX identifier. We could also have linters that would complain if any packages in the workspace don't use the "spdx" type (though we'd have to do a lot of work to enable that by default).
Does that make sense?
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.
Would it not be simpler to require the use of SPDX identifiers?
Other efforts to standardise licensing information (like REUSE) have gone that way and it makes parsing and validation much more straightforward.
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.
Ideally, yes, we would require SPDX identifiers.
The issue is backwards compatibility. There are thousands of ROS (1 & 2) packages out there in the wild, and almost none of the package.xml files in them follow SPDX. Explicitly putting a type field in will make it easier for tools (and humans) to determine whether the field is expected to be SPDX or not.
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 like the idea of introducing a type attribute. There are many ROS 1 packages that simply specify
BSDas license identifier, which is not enough information to infer the exact license.I would be a bit more optimistic for the linter and in particular for generating a debian/copyright file:
<license file="path_to_license.txt">BSD</license>: This is enough information to generate a Debian copyright file. The generator (and the linter) should check whetherpath_to_license.txtreally contains a BSD license text.<license>BSD</license>(or<license source-files="*">BSD</license>) with a fileLICENSEin the root of the repo: Again, this is enough information to generate a Debian copyright file in my opinion.<license>Some SPDX full name or SPDX identifier</license>: Again enough information to generate a Debian copyright file (and to validate it against a potential LICENSE file in the package or repo root).<license file="custom_license_text.txt">MyCustomLicenseIdentifier</license>: Also this would allow the generation of a debian/copyright file according to https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/. ("If there are licenses present in the package without a standard short name, an arbitrary short name may be assigned for these licenses. These arbitrary names are only guaranteed to be unique within a single copyright file.")Even the case that a debian/copyright file is generated where the license short name does not match the license text I do not consider dramatic: It simply repeats the inconsistent license names and texts from the source code and should be discovered by relevant tools applied to the debian/copyright file.
As a consequence, the
type="spdx"attribute is not really necessary but its use would introduce a safety layer that prevents that a typo in an SPDX license identifier leads to the interpretation as a custom license identifier (i.e., as case 4).What we must prevent in any case is that the future debian/copyright file generator refines the license specification for which there is no evidence - which is the case for some ROS 1 packages that specify
<license>BSD</license>without any further license headers orLICENSEfile.Uh oh!
There was an error while loading. Please reload this page.
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.
but don't we have the same problem with the attributes and elements proposed here? All the packages in existence today would also not have those.
if the package format version would be increased, tools could just assume "v4 manifest->spdx".
Anything below that does not (have to) conform to that.
If the concern is "there may be licenses in use for which there are no SPDX identifiers", then SPDX has a standard approach for that in place IIRC.
Edit: absence of the type attribute could of course also be used, but it seems much cleaner to me to be able to say "format 4 manifests must use SPDX identifiers" than "if the
licensetag doesn't have thetypeattribute then it's OK to not use SPDX identifiers".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 the
typeattribute.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.
Just for completeness: The SPDX Specification also describes a solution for naming custom licenses (cf. https://spdx.dev/spdx-specification-21-web-version/#h.1v1yuxt), but I still prefer the explicit
typeattribute to distinguish between licenses from the SPDX License List and custom ones.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.
typeattribution with default looks good to me.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.
Introduced the
typeattribute as proposed by @clalancette. (Sorry, I should have updated the PR before yesterday's discussion in the TSC meeting.)Is the description of the new attribute sufficient or should I give explicit explanations of the two possible values
freeformandspdx?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.
@clalancette, are you fine if I mark the discussion on the type attribute as resolved, cf. my last question on the explanation of the attribute in the REP?