-
Notifications
You must be signed in to change notification settings - Fork 332
Make simple license mappings more strict #10334
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
base: main
Are you sure you want to change the base?
Conversation
@@ -22,34 +22,8 @@ | |||
# Sort the entries below via IntelliJ's "Edit" -> "Sort Lines". | |||
--- | |||
|
|||
# Ambiguous mappings (mapping reason not obvious without additional information) |
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.
As you can see, the new check accurately removes all entries that were already deemed to be ambiguous. Additionally, it removes some entries that probably should have been regarded to be ambiguous in the first place as well.
@@ -23,41 +23,22 @@ | |||
--- | |||
|
|||
AFLv2.1: AFL-2.1 | |||
ALv2: Apache-2.0 |
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.
This further strictens the check to not tolerate differences in trailing zeros when extract digits. This avoids the theoretical issue that e.g. Apache2 could either refer to Apache-2.0
or Apache-2.1
if the latter came out at some point.
Make these explicit for clarity. Signed-off-by: Sebastian Schuberth <[email protected]>
Make mroe clear what distinguished these test containers. Signed-off-by: Sebastian Schuberth <[email protected]>
Tolerate differences in trailing zeros. Signed-off-by: Sebastian Schuberth <[email protected]>
Signed-off-by: Sebastian Schuberth <[email protected]>
f43a1fa
to
b922c7d
Compare
@oss-review-toolkit/core-devs I would really like to finally get rid of the ambiguous simple license mappings that we already identified quite some time ago. I have implemented two variants via a check: A lenient and a stricter one. Let's discuss whether we can at least get the lenient check in. To me it's important that the criteria for checking ambiguity are objective and reproducible by tests. |
@@ -57,6 +57,23 @@ class SpdxSimpleLicenseMappingTest : WordSpec({ | |||
if (license.id.endsWith("-only")) key should containADigit() | |||
} | |||
} | |||
|
|||
"contain equal digits omitting trailing zeros" { | |||
val exceptions = mapOf( |
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.
There are license IDs which have dates in their identifier, e.g. some LicenseRef-scancode-*
licenses.
There may be further cases, where this heuristic fails.
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'd be fine with adding anything that start with LicenseRef-
to the list of exceptions. But we don't have such mappings currently anyway.
ALv2: Apache-2.0 | ||
ASL: Apache-2.0 | ||
Apache-style: Apache-2.0 | ||
Apache: Apache-2.0 |
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.
commit: The title says Check for...
But the commit also removes a bunch of entries. I recall in the past, that we removed only some BSD
related entry which caused huge amounts of issues popping up.
So, I believe this is critical and should be made more prominent by dedicating the commit title to it.
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 can tweak commit messages later if we agree on the general idea and I move this PR out of draft state.
@@ -23,41 +23,22 @@ | |||
--- | |||
|
|||
AFLv2.1: AFL-2.1 | |||
ALv2: Apache-2.0 |
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.
commit-msg: I believe the title should make clear that entries from the mapping are removed.
BSD-3: BSD-3-Clause | ||
BSD2: BSD-2-Clause | ||
BSD3: BSD-3-Clause | ||
Bouncy: MIT | ||
EDL-1.0: BSD-3-Clause | ||
FreeBSD: BSD-2-Clause-Views | ||
GPL-2: GPL-2.0-only | ||
GPL2: GPL-2.0-only |
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 prefer not to remove these entries, but instead separate them.
So, their use can be configured.
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.
Making simple license mappings configurable is certainly a good idea, but out of scope of this PR. I first like to get the baseline clean.
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.
Agree with @fviernau we can't remove these declared license mappings - this will cause major service disruption for many ORT users.
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'm fine with simply dropping this last, stricter commit from the PR. Would that resolve any of your concerns?
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 would propose a splitting out the (not so risky) ambiguous ones and introduce a ambiguousMappingsEnabled
configuration option.
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.
Please, not yet more configuration options 🙄
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10334 +/- ##
=========================================
Coverage 56.48% 56.48%
+ Complexity 1611 1609 -2
=========================================
Files 331 331
Lines 12268 12268
Branches 1138 1138
=========================================
Hits 6929 6929
Misses 4896 4896
Partials 443 443
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BSD-3: BSD-3-Clause | ||
BSD2: BSD-2-Clause | ||
BSD3: BSD-3-Clause | ||
Bouncy: MIT | ||
EDL-1.0: BSD-3-Clause | ||
FreeBSD: BSD-2-Clause-Views | ||
GPL-2: GPL-2.0-only | ||
GPL2: GPL-2.0-only |
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.
Agree with @fviernau we can't remove these declared license mappings - this will cause major service disruption for many ORT users.
Splitting out the first two commits to #10336. |
Please have a look at the individual commit messages for the details.