fix bug where GPL-2.0 failed to match GPL-2.0-only #42
Conversation
This file contains 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
elrayle
commented
Mar 20, 2023
| // first, second | ||
| return nodes.licensesExactlyEqual() | ||
| // first, second requires both to be in same range group | ||
| return nodes.rangesEqual() |
There was a problem hiding this comment.
This is the change that fixes the bug. It makes sure that if neither license hasPlus, then the licenses must be in the same range (e.g. GPL-2.0 and GPL-2.0-only are in the same range).
Previously, it checked that the licenses were exactly the same. (e.g. GPL-2.0 != GPL-2.0-only). Since these should be treated as equivalent, but aren't literally the same, it led to the bug.
add/refine function documentation
dangoor
approved these changes
Mar 21, 2023
| @@ -1,5 +1,13 @@ | |||
| package spdxexp | |||
|
|
|||
| // The compare methods determine if two ranges are greater than, less than or equal within the same license group. | |||
| @@ -238,7 +256,7 @@ func (nodes *nodePair) rangesAreCompatible() bool { | |||
| // When both licenses allow later versions (i.e. hasPlus==true), being in the same license | |||
| // group is sufficient for compatibility, as long as, any exception is also compatible | |||
| // Example: All Apache licenses (e.g. Apache-1.0, Apache-2.0) are in the same license group | |||
| return sameLicenseGroup(firstRange, secondRange) && nodes.exceptionsAreCompatible() | |||
| return sameLicenseGroup(firstRange, secondRange) | |||
There was a problem hiding this comment.
I assume exception compatibility is still handled elsewhere?
elrayle
commented
Mar 21, 2023
| if !nodes.firstNode.isLicense() || !nodes.secondNode.isLicense() { | ||
| return false | ||
| } | ||
| if !nodes.exceptionsAreCompatible() { |
There was a problem hiding this comment.
@dangoor This is the check for exceptions. It is handled before checking ranges.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Testing the policy service identified a bug where
GPL-2.0was not allowed forGPL-2.0-only. This PR does some cleanup onnode.goto lay out the comparisons more clearly and fix the bug.Changes
Most of the changes are adding test-cases that would have caught the bug. The only substantive changes are in the range checks in
node.go. There is one minor change tocompareEQ()to short cut the comparison if the two licenses are the same.has_plus==truehas_plus