Simplifications to determining valid license choices #8106
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8106 +/- ##
=========================================
Coverage 67.16% 67.16%
Complexity 2052 2052
=========================================
Files 358 358
Lines 17166 17166
Branches 2462 2462
=========================================
Hits 11530 11530
Misses 4613 4613
Partials 1023 1023
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| "MIT AND GPL-1.0-or-later AND GPL-2.0-only", | ||
| "GPL-2.0-only AND BSD-3-Clause AND BSD-3-Clause", | ||
| "GPL-2.0-only AND GPL-1.0-or-later AND BSD-3-Clause", | ||
| "GPL-2.0-only AND GPL-1.0-or-later AND GPL-2.0-only" |
There was a problem hiding this comment.
Should this not contain all possible permutations? Some are missing like "MIT AND BSD-3-Clause AND MIT" or "MIT AND BSD-3-Clause AND GPL-2.0-only".
There was a problem hiding this comment.
I was at first wondering the same, but validChoice() returns a Set, and it is "MIT AND BSD-3-Clause AND MIT".toSpdx() == "MIT AND MIT AND BSD-3-Clause".toSpdx(), so these permutations are not added to the set.
There was a problem hiding this comment.
Which indeed is unfortunate, as it's not clear to the user which permutation will be contained in the set.
However, that's required as isValidChoice() uses choice in validChoices() which does not check for semantic equality.
Edit: Scratch that, I've just added a test to prove the contrary.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, so if this change fixes the problem that the choices are wrong this is already a big improvement, but it seems we need to do more work for a nice UX. For example, "MIT AND MIT AND MIT" should also get collapsed to just "MIT".
There was a problem hiding this comment.
For example, "MIT AND MIT AND MIT" should also get collapsed to just "MIT".
You can already use "MIT" instead of "MIT AND MIT AND MIT", see the test I've added in the last commit of this PR.
There was a problem hiding this comment.
It could be that this is not wrong if the expression is in DNF (hence
validChoicesForDnf), but I don't remember the details.
No, it was wrong even if you assume the expression to be in DNF. That's actually the root cause of #8082.
There was a problem hiding this comment.
You can already use "MIT" instead of "MIT AND MIT AND MIT", see the test I've added in the last commit of this PR.
I was also thinking of being able to show the valid choices to the user, but that is about a future improvement anyway.
There was a problem hiding this comment.
Could you add one more test for an expression with deeper nesting, like (A OR (B AND (C OR D))) AND (E OR F)?
@sschuberth Are you planning to add such a test? I think it would be useful but if you don't want to I don't insist.
There was a problem hiding this comment.
I'm looking into it and meanwhile discovered another thing I'd like to double-check.
sschuberth
force-pushed
the
spdx-dnf-readability-2
branch
from
5923add to
2fed658
Compare
sschuberth
force-pushed
the
spdx-dnf-readability-2
branch
from
2fed658 to
854b7af
Compare
sschuberth
force-pushed
the
spdx-dnf-readability-2
branch
from
854b7af to
3892bd3
Compare
| "MIT AND GPL-1.0-or-later AND GPL-2.0-only", | ||
| "GPL-2.0-only AND BSD-3-Clause AND BSD-3-Clause", | ||
| "GPL-2.0-only AND GPL-1.0-or-later AND BSD-3-Clause", | ||
| "GPL-2.0-only AND GPL-1.0-or-later AND GPL-2.0-only" |
There was a problem hiding this comment.
Could you add one more test for an expression with deeper nesting, like (A OR (B AND (C OR D))) AND (E OR F)? Would be interesting to know if the problem is now also fixed for such epxressions. And even if it doesn't work it would be good to have this documented in a test case.
IIRC the reason to use the DNF was to remove the nesting from the expression to make it easier to find the valid choices.
The original code was clearly wrong in that it recursively decomposed an
expression...
It could be that this is not wrong if the expression is in DNF (hence validChoicesForDnf), but I don't remember the details.
sschuberth
force-pushed
the
spdx-dnf-readability-2
branch
from
3892bd3 to
7ad0a64
Compare
This avoids misunderstandings if SPDX expression equality is implicitly applied. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
For a non-`SpdxCompoundExpression`, `validChoices()` equals `validChoicesForDnf()`, so simplify the code accordingly. Do to some weird compile error, `validChoicesForDnf()` needs to change from `protected` to `internal`, but this is only temporary as the function will get removed in the following commits. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
The original code was clearly wrong in that it recursively decomposed an expression and composed the contained expressions always with `AND` although there could be nested `OR` expressions. Instead, if there is a top-level `AND` in an expressions, create all combinations of choices on the left and choices on the right to get the total valid choices. Reword affected tests according to the fact that returned choices are not simplified, but explicit, though still deduplicated. Commonly introduce a `choice` variable along the way to make tests for long license expressions more readable. Fixes #8082. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
Remove all DNF-related code as it is not actually necessary in order to determine the valid license choices anymore as of the recent changes to `validChoicesForDnf()`. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
See [1] for context. [1]: #8082 (comment) Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
sschuberth
force-pushed
the
spdx-dnf-readability-2
branch
from
7ad0a64 to
c787f7a
Compare
Please have a look at the individual commit messages for the details.