Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions spdxexp/compare.go
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

very helpful comment!

// NOTE: Ranges are organized into groups (referred to as license groups) of the same base license (e.g. GPL).
// Groups have sub-groups of license versions (referred to as the range) where each member is considered
// to be the same version (e.g. {GPL-2.0, GPL-2.0-only}). The sub-groups are in ascending order within
// the license group, such that the first sub-group is considered to be less than the second sub-group,
// and so on. (e.g. {{GPL-1.0}, {GPL-2.0, GPL-2.0-only}} implies {GPL-1.0} < {GPL-2.0, GPL-2.0-only}).

// compareGT returns true if the first range is greater than the second range within the same license group; otherwise, false.
func compareGT(first *node, second *node) bool {
if !first.isLicense() || !second.isLicense() {
return false
Expand All @@ -13,6 +21,7 @@ func compareGT(first *node, second *node) bool {
return firstRange.location[versionGroup] > secondRange.location[versionGroup]
}

// compareLT returns true if the first range is less than the second range within the same license group; otherwise, false.
func compareLT(first *node, second *node) bool {
if !first.isLicense() || !second.isLicense() {
return false
Expand All @@ -26,10 +35,15 @@ func compareLT(first *node, second *node) bool {
return firstRange.location[versionGroup] < secondRange.location[versionGroup]
}

// compareEQ returns true if the first and second range are the same range within the same license group; otherwise, false.
func compareEQ(first *node, second *node) bool {
if !first.isLicense() || !second.isLicense() {
return false
}
if first.lic.license == second.lic.license {
return true
}

firstRange := getLicenseRange(*first.license())
secondRange := getLicenseRange(*second.license())

Expand All @@ -39,6 +53,8 @@ func compareEQ(first *node, second *node) bool {
return firstRange.location[versionGroup] == secondRange.location[versionGroup]
}

// sameLicenseGroup returns false if either license isn't in a range or the two ranges are
// not in the same license group (e.g. group GPL != group Apache); otherwise, true
func sameLicenseGroup(firstRange *licenseRange, secondRange *licenseRange) bool {
if firstRange == nil || secondRange == nil || firstRange.location[licenseGroup] != secondRange.location[licenseGroup] {
return false
Expand Down
9 changes: 9 additions & 0 deletions spdxexp/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@ func TestCompareGT(t *testing.T) {
{"expect greater than: AGPL-3.0 > AGPL-1.0", getLicenseNode("AGPL-3.0", false), getLicenseNode("AGPL-1.0", false), true},
{"expect equal: GPL-2.0-or-later > GPL-2.0-only", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0-only", false), false},
{"expect equal: GPL-2.0-or-later > GPL-2.0", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0", false), false},
{"expect equal: GPL-2.0-only > GPL-2.0", getLicenseNode("GPL-2.0-only", false), getLicenseNode("GPL-2.0", false), false},
{"expect equal: GPL-3.0 > GPL-3.0", getLicenseNode("GPL-3.0", false), getLicenseNode("GPL-3.0", false), false},
{"expect equal: MIT > MIT", getLicenseNode("MIT", false), getLicenseNode("MIT", false), false},
{"expect less than: MPL-1.0 > MPL-2.0", getLicenseNode("MPL-1.0", false), getLicenseNode("MPL-2.0", false), false},
{"incompatible: MIT > ISC", getLicenseNode("MIT", false), getLicenseNode("ISC", false), false},
{"incompatible: MIT > GPL-2.0-only", getLicenseNode("MIT", false), getLicenseNode("GPL-2.0-only", false), false},
{"incompatible: OSL-1.0 > OPL-1.0", getLicenseNode("OSL-1.0", false), getLicenseNode("OPL-1.0", false), false},
{"not simple license: (MIT OR ISC) > GPL-3.0", getLicenseNode("(MIT OR ISC)", false), getLicenseNode("GPL-3.0", false), false}, // TODO: should it raise error?
}
Expand All @@ -49,9 +52,12 @@ func TestCompareEQ(t *testing.T) {
{"expect greater than: AGPL-3.0 == AGPL-1.0", getLicenseNode("AGPL-3.0", false), getLicenseNode("AGPL-1.0", false), false},
{"expect equal: GPL-2.0-or-later > GPL-2.0-only", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0-only", false), true},
{"expect equal: GPL-2.0-or-later > GPL-2.0", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0", false), true},
{"expect equal: GPL-2.0-only == GPL-2.0", getLicenseNode("GPL-2.0-only", false), getLicenseNode("GPL-2.0", false), true},
{"expect equal: GPL-3.0 == GPL-3.0", getLicenseNode("GPL-3.0", false), getLicenseNode("GPL-3.0", false), true},
{"expect equal: MIT == MIT", getLicenseNode("MIT", false), getLicenseNode("MIT", false), true},
{"expect less than: MPL-1.0 == MPL-2.0", getLicenseNode("MPL-1.0", false), getLicenseNode("MPL-2.0", false), false},
{"incompatible: MIT == ISC", getLicenseNode("MIT", false), getLicenseNode("ISC", false), false},
{"incompatible: MIT == GPL-2.0-only", getLicenseNode("MIT", false), getLicenseNode("GPL-2.0-only", false), false},
{"incompatible: OSL-1.0 == OPL-1.0", getLicenseNode("OSL-1.0", false), getLicenseNode("OPL-1.0", false), false},
{"not simple license: (MIT OR ISC) == GPL-3.0", getLicenseNode("(MIT OR ISC)", false), getLicenseNode("GPL-3.0", false), false}, // TODO: should it raise error?
}
Expand All @@ -78,9 +84,12 @@ func TestCompareLT(t *testing.T) {
{"expect greater than: AGPL-3.0 < AGPL-1.0", getLicenseNode("AGPL-3.0", false), getLicenseNode("AGPL-1.0", false), false},
{"expect greater than: GPL-2.0-or-later < GPL-2.0-only", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0-only", false), false},
{"expect greater than: GPL-2.0-or-later == GPL-2.0", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0", false), false},
{"expect equal: GPL-2.0-only < GPL-2.0", getLicenseNode("GPL-2.0-only", false), getLicenseNode("GPL-2.0", false), false},
{"expect equal: GPL-3.0 < GPL-3.0", getLicenseNode("GPL-3.0", false), getLicenseNode("GPL-3.0", false), false},
{"expect equal: MIT < MIT", getLicenseNode("MIT", false), getLicenseNode("MIT", false), false},
{"expect less than: MPL-1.0 < MPL-2.0", getLicenseNode("MPL-1.0", false), getLicenseNode("MPL-2.0", false), true},
{"incompatible: MIT < ISC", getLicenseNode("MIT", false), getLicenseNode("ISC", false), false},
{"incompatible: MIT < GPL-2.0-only", getLicenseNode("MIT", false), getLicenseNode("GPL-2.0-only", false), false},
{"incompatible: OSL-1.0 < OPL-1.0", getLicenseNode("OSL-1.0", false), getLicenseNode("OPL-1.0", false), false},
{"not simple license: (MIT OR ISC) < GPL-3.0", getLicenseNode("(MIT OR ISC)", false), getLicenseNode("GPL-3.0", false), false}, // TODO: should it raise error?
}
Expand Down
7 changes: 7 additions & 0 deletions spdxexp/license.go
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,13 @@ func getExceptions() []string {
}
}

// licenseRanges returns a list of license ranges.
//
// Ranges are organized into groups (referred to as license groups) of the same base license (e.g. GPL).
// Groups have sub-groups of license versions (referred to as the range) where each member is considered
// to be the same version (e.g. {GPL-2.0, GPL-2.0-only}). The sub-groups are in ascending order within
// the license group, such that the first sub-group is considered to be less than the second sub-group,
// and so on. (e.g. {{GPL-1.0}, {GPL-2.0, GPL-2.0-only}} implies {GPL-1.0} < {GPL-2.0, GPL-2.0-only}).
func licenseRanges() [][][]string {
return [][][]string{
{
Expand Down
64 changes: 40 additions & 24 deletions spdxexp/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (n *node) isLicense() bool {
return n.role == licenseNode
}

// Return the value of the license field.
// license returns the value of the license field.
// See also reconstructedLicenseString()
func (n *node) license() *string {
if !n.isLicense() {
Expand Down Expand Up @@ -167,7 +167,7 @@ func (n *node) reconstructedLicenseString() *string {
return nil
}

// Sort an array of license and license reference nodes alphabetically based
// sortLicenses sorts an array of license and license reference nodes alphabetically based
// on their reconstructedLicenseString() representation. The sort function does not expect
// expression nodes, but if one is in the nodes list, it will sort to the end.
func sortLicenses(nodes []*node) {
Expand All @@ -186,29 +186,51 @@ func sortLicenses(nodes []*node) {

// ---------------------- Comparator Methods ----------------------

// Return true if two licenses are compatible; otherwise, false.
// licensesAreCompatible returns true if two licenses are compatible; otherwise, false.
// Two licenses are compatible if they are the same license or if they are in the same
// license group and they meet one of the following rules:
//
// * both licenses have the `hasPlus` flag set to true
// * the first license has the `hasPlus` flag and the second license is in the first license's range or greater
// * the second license has the `hasPlus` flag and the first license is in the second license's range or greater
// * both licenses are in the same range
func (nodes *nodePair) licensesAreCompatible() bool {
// checking ranges is expensive, so check for simple cases first
if !nodes.firstNode.isLicense() || !nodes.secondNode.isLicense() {
return false
}
if !nodes.exceptionsAreCompatible() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dangoor This is the check for exceptions. It is handled before checking ranges.

return false
}
if nodes.licensesExactlyEqual() {
return true
}

// simple cases don't apply, so check license ranges
// NOTE: Ranges are organized into groups (referred to as license groups) of the same base license (e.g. GPL).
// Groups have sub-groups of license versions (referred to as the range) where each member is considered
// to be the same version (e.g. {GPL-2.0, GPL-2.0-only}). The sub-groups are in ascending order within
// the license group, such that the first sub-group is considered to be less than the second sub-group,
// and so on. (e.g. {{GPL-1.0}, {GPL-2.0, GPL-2.0-only}} implies {GPL-1.0} < {GPL-2.0, GPL-2.0-only}).
if nodes.secondNode.hasPlus() {
if nodes.firstNode.hasPlus() {
// first+, second+
// first+, second+ just need to be in same range group
return nodes.rangesAreCompatible()
}
// first, second+
// first, second+ requires first to be in range of second
return nodes.identifierInRange()
}
// else secondNode does not have plus
if nodes.firstNode.hasPlus() {
// first+, second
// first+, second requires second to be in range of first
revNodes := &nodePair{firstNode: nodes.secondNode, secondNode: nodes.firstNode}
return revNodes.identifierInRange()
}
// first, second
return nodes.licensesExactlyEqual()
// first, second requires both to be in same range group
return nodes.rangesEqual()
Copy link
Collaborator Author

@elrayle elrayle Mar 20, 2023

Choose a reason for hiding this comment

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

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.

}

// licenseRefsAreCompatible returns true if two license references are compatible; otherwise, false.
func (nodes *nodePair) licenseRefsAreCompatible() bool {
if !nodes.firstNode.isLicenseRef() || !nodes.secondNode.isLicenseRef() {
return false
Expand All @@ -222,13 +244,9 @@ func (nodes *nodePair) licenseRefsAreCompatible() bool {
return compatible
}

// Return true if two licenses are compatible in the context of their ranges; otherwise, false.
// licenseRefsAreCompatible returns true if two licenses are in the same license group (e.g. all "GPL" licenses are in the same
// license group); otherwise, false.
func (nodes *nodePair) rangesAreCompatible() bool {
if nodes.licensesExactlyEqual() {
// licenses specify ranges exactly the same (e.g. Apache-1.0+, Apache-1.0+)
return true
}

firstNode := *nodes.firstNode
secondNode := *nodes.secondNode

Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume exception compatibility is still handled elsewhere?

}

// identifierInRange returns true if the (first) simple license is in range of the (second)
Expand All @@ -247,14 +265,7 @@ func (nodes *nodePair) identifierInRange() bool {
simpleLicense := nodes.firstNode
plusLicense := nodes.secondNode

if !compareGT(simpleLicense, plusLicense) && !compareEQ(simpleLicense, plusLicense) {
return false
}

// With simpleLicense >= plusLicense, licenses are compatible, as long as, any exception
// is also compatible
return nodes.exceptionsAreCompatible()

return compareGT(simpleLicense, plusLicense) || compareEQ(simpleLicense, plusLicense)
}

// exceptionsAreCompatible returns true if neither license has an exception or they have
Expand All @@ -274,10 +285,15 @@ func (nodes *nodePair) exceptionsAreCompatible() bool {
}

return *nodes.firstNode.exception() == *nodes.secondNode.exception()
}

// rangesEqual returns true if the licenses are in the same range; otherwise, false
// (e.g. GPL-2.0-only == GPL-2.0)
func (nodes *nodePair) rangesEqual() bool {
return compareEQ(nodes.firstNode, nodes.secondNode)
}

// Return true if the licenses are the same; otherwise, false
// licensesExactlyEqual returns true if the licenses are the same; otherwise, false
func (nodes *nodePair) licensesExactlyEqual() bool {
return strings.EqualFold(*nodes.firstNode.reconstructedLicenseString(), *nodes.secondNode.reconstructedLicenseString())
}
4 changes: 4 additions & 0 deletions spdxexp/satisfies_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ func TestValidateLicenses(t *testing.T) {
{"All invalid", []string{"MTI", "Apche-2.0", "0xDEADBEEF", ""}, false, []string{"MTI", "Apche-2.0", "0xDEADBEEF", ""}},
{"All valid", []string{"MIT", "Apache-2.0", "GPL-2.0"}, true, []string{}},
{"Some invalid", []string{"MTI", "Apche-2.0", "GPL-2.0"}, false, []string{"MTI", "Apche-2.0"}},
{"GPL-2.0", []string{"GPL-2.0"}, true, []string{}},
{"GPL-2.0-only", []string{"GPL-2.0-only"}, true, []string{}},
}

for _, test := range tests {
Expand Down Expand Up @@ -110,6 +112,8 @@ func TestSatisfies(t *testing.T) {
{"GPL-1.0+ satisfies [GPL-2.0+]", "GPL-1.0+", []string{"GPL-2.0+"}, true, nil},
{"! GPL-1.0 satisfies [GPL-2.0+]", "GPL-1.0", []string{"GPL-2.0+"}, false, nil},
{"GPL-2.0-only satisfies [GPL-2.0-only]", "GPL-2.0-only", []string{"GPL-2.0-only"}, true, nil},
{"GPL-2.0 satisfies [GPL-2.0-only]", "GPL-2.0", []string{"GPL-2.0-only"}, true, nil},
{"GPL-2.0 AND GPL-2.0-only satisfies [GPL-2.0-only]", "GPL-2.0 AND GPL-2.0-only", []string{"GPL-2.0-only"}, true, nil},
{"GPL-3.0-only satisfies [GPL-2.0+]", "GPL-3.0-only", []string{"GPL-2.0+"}, true, nil},

{"! GPL-2.0 satisfies [GPL-2.0+ WITH Bison-exception-2.2]",
Expand Down