Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Associate stand-alone ScanCode exceptions to licenses #5078

Merged
merged 4 commits into from
Feb 23, 2022
Merged

Associate stand-alone ScanCode exceptions to licenses #5078

merged 4 commits into from
Feb 23, 2022

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

@sschuberth sschuberth requested a review from a team as a code owner February 20, 2022 16:25
@sschuberth sschuberth force-pushed the scancode-parser-imps branch 6 times, most recently from a4fc1ef to 1adf0d3 Compare February 20, 2022 17:23
@sschuberth sschuberth marked this pull request as draft February 21, 2022 09:25
@sschuberth
Copy link
Member Author

I've split out some changes to #5080.

@sschuberth sschuberth marked this pull request as ready for review February 21, 2022 21:02
@sschuberth
Copy link
Member Author

I've generalized the solution in additional commits, please have another look.

@codecov
Copy link

codecov bot commented Feb 21, 2022 •
edited
Loading

Codecov Report

Merging #5078 (d304ea2) into main (099b2c1) will increase coverage by 0.05%.
The diff coverage is 89.65%.

❗ Current head d304ea2 differs from pull request most recent head 8bb4682. Consider uploading reports for the commit 8bb4682 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #5078      +/-   ##
============================================
+ Coverage     72.42%   72.47%   +0.05%     
  Complexity     1894     1894              
============================================
  Files           251      251              
  Lines         13412    13440      +28     
  Branches       1891     1898       +7     
============================================
+ Hits           9714     9741      +27     
  Misses         2710     2710              
- Partials        988      989       +1
Impacted Files Coverage Δ
...n/kotlin/scanners/scancode/ScanCodeResultParser.kt 92.61% <89.65%> (+0.72%) ⬆️
downloader/src/main/kotlin/vcs/Cvs.kt 17.72% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 099b2c1...8bb4682. Read the comment docs.

@sschuberth sschuberth added the release notes Changes that should be mentioned in release notes label Feb 22, 2022
@sschuberth sschuberth dismissed tsteenbe’s stale review February 22, 2022 14:02

Comments addressed.

mnonnenmacher
mnonnenmacher requested changes Feb 22, 2022
View reviewed changes
model/src/main/kotlin/TextLocation.kt Outdated Show resolved Hide resolved
scanner/src/main/kotlin/scanners/scancode/ScanCodeResultParser.kt Outdated Show resolved Hide resolved
scanner/src/main/kotlin/scanners/scancode/ScanCodeResultParser.kt Outdated
.firstOrNull { it.second <= LICENSES_WITH_EXCEPTIONS_TOLERANCE_LINES }

if (llvm != null) {
llvmExceptions.remove(llvm.first)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to keep it in case there are more Apache-2.0 findings within the tolerance range.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that wouldn't work easily as I'm later handling the remaining exceptions in llvmExceptions.

scanner/src/main/kotlin/scanners/scancode/ScanCodeResultParser.kt Outdated
"Apache-2.0" -> apacheLicenses += it
"LLVM-exception" -> llvmExceptions += it
else -> otherFindings += it
val e = remainingExceptions.iterator()
Copy link
Member

Choose a reason for hiding this comment

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

Using e here makes the code look a bit like it would handle actual throwable exceptions, not license exceptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to i instead 😁 (Unless you come up with a better name, but I'd like to keep it short.)

Copy link
Member Author

Choose a reason for hiding this comment

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

...although IMO e.remove() read nicer than i.remove().

scanner/src/test/kotlin/scanners/scancode/ScanCodeResultParserTest.kt Outdated Show resolved Hide resolved
@sschuberth sschuberth force-pushed the scancode-parser-imps branch 2 times, most recently from 49e2341 to d304ea2 Compare February 23, 2022 07:13
@sschuberth
spdx-utils: Add a mapping of exceptions to their applicable licenses
4b9584d 
Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
@sschuberth
TextLocation: Add a bunch of convenience functions
24fa331 
This will be used by upcoming changes.

Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
@sschuberth
FindingsMatcher: Add a function to associate exceptions by licenses
75900bf 
E.g. ScanCode reports exceptions to licenses as individual license
findings. That is problematic as exceptions on their own are not valid
SPDX expressions, also see [1].

Introduce a new function that fixes up findings by associating
exceptions by their belonging licenses.

[1]: aboutcode-org/scancode-toolkit#2873

Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
@sschuberth
ScanCodeResultParser: Associate stand-alone exceptions by licenses
8bb4682 
Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
@sschuberth
Copy link
Member Author

I've made a (hopefully) final refacting to put the (now) generic associateLicensesWithExceptions() function into the FindingsMatcher file, which IMO is a better fit.

@sschuberth sschuberth merged commit d3a76f0 into main Feb 23, 2022
@sschuberth sschuberth deleted the scancode-parser-imps branch February 23, 2022 10:14
@tsteenbe tsteenbe mentioned this pull request Feb 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnonnenmacher mnonnenmacher mnonnenmacher approved these changes

@tsteenbe tsteenbe Awaiting requested review from tsteenbe

Assignees
No one assigned
Labels
release notes Changes that should be mentioned in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sschuberth @tsteenbe @mnonnenmacher