feat(scanner): Merge duplicate scan results that share a provenance #10502
+408
−30
Conversation
This file contains hidden or 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
038c082 to
40190e3
Compare
1 task
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10502 +/- ##
=========================================
Coverage 56.86% 56.86%
Complexity 1606 1606
=========================================
Files 337 337
Lines 12512 12512
Branches 1179 1179
=========================================
Hits 7115 7115
Misses 4945 4945
Partials 452 452
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:
|
40190e3 to
ed1df5f
Compare
There was a problem hiding this comment.
Hi @maennchen,
thanks for your contribution!
Do you mind adding a test that demonstrates the issue with the duplicated scan results and that your changes fix these issues?
Edit: As there is currently no existing test for the scan function that use an ORT result, I won't insist on this change.
Edit 2: I Found these existing tests which already test the scan() function:
ort/scanner/src/funTest/kotlin/scanners/ScannerIntegrationFunTest.kt
Lines 63 to 102 in c086b3b
Can you have a look at them and extend them to test your use-case?
|
@MarcelBochtler I added a test to test the deduplication. Running the same test on
$ ./gradlew scanner:funTest --tests "org.ossreviewtoolkit.scanner.scanners.ScannerIntegrationFunTest" |
4432de1 to
263be95
Compare
263be95 to
276bc2f
Compare
MarcelBochtler
previously approved these changes
Jun 25, 2025
sschuberth
requested changes
Jun 26, 2025
scanner/src/funTest/kotlin/scanners/ScannerIntegrationFunTest.kt
Outdated
Show resolved
Hide resolved
| message: "IOException: Could not resolve provenance for package 'Dummy::project:1.0.0'\ | ||
| \ for source code origins [VCS, ARTIFACT]." | ||
| severity: "ERROR" | ||
| package_provenance: |
There was a problem hiding this comment.
It's not obvious to me at a quick glance that this is a good change. Isn't it intended for this dummy project to have a resolution issue?
There was a problem hiding this comment.
Can you elaborate on this? I have moved the test into a separate test in the meantime. Does your comment still apply?
fviernau
requested changes
Jun 26, 2025
fviernau
requested changes
Jun 26, 2025
scanner/src/funTest/kotlin/scanners/ScannerIntegrationFunTest.kt
Outdated
Show resolved
Hide resolved
When the SpdxDocumentFile package manager is used, the *project* and all contained *packages* often resolve to the **same VCS provenance** (e.g. the root of the Git repository). Before this change ORT stored two separate `ScanResult`s for such a provenance – one keyed to the project, one keyed to the package. That caused two follow-on problems: * Both results appeared in the `OrtResult`, so evaluators saw **duplicate findings** for the *same* source tree. * Because projects and packages are handled by different rules the package result was additionally **padded with a `SpdxConstants.NONE` finding** whenever `includeFilesWithoutFindings` was enabled. The evaluator therefore compared *real* license findings from the project result with `NONE` from the package result and failed with a violation. This patch * groups scan results by the pair `(provenance, scanner)` and folds them into a single `ScanResult`, * unions the inner finding sets to avoid duplicates, and * performs the "pad with NONE" step only **after** deduplication, so every path is represented exactly once. As a consequence the evaluator now receives one consistent set of license findings per provenance / scanner, eliminating the false mismatch. Signed-off-by: Jonatan Männchen <[email protected]>
276bc2f to
dfb5f87
Compare
|
The code can be seen in action here: https://github.com/elixir-lang/elixir/actions/runs/16086802339/job/45398947306 |
fviernau
requested changes
Jul 7, 2025
| @@ -34,6 +34,7 @@ import org.ossreviewtoolkit.model.Package | |||
| import org.ossreviewtoolkit.model.PackageReference | |||
| import org.ossreviewtoolkit.model.PackageType | |||
| import org.ossreviewtoolkit.model.Project | |||
| import org.ossreviewtoolkit.model.Repository | |||
There was a problem hiding this comment.
commit-message:
I believe, from the commit message it should be understandable what exact issues happened before this
change, which this change is supposed to fix. I still need help understanding these, and I believe commit
message should be adjusted so that it becomes clearer. Unclear to me:
- "so evaluators saw duplicate findings for the same source tree."
- Rules operate on a per package / project basis. If multiple originate from same source tree,
it is expected that same source tree is processed mutliple times. Would it maybe help to
explain this with an example?
- Rules operate on a per package / project basis. If multiple originate from same source tree,
- "Because projects and packages are handled by different rules the package
result was additionally padded with aSpdxConstants.NONEfinding
wheneverincludeFilesWithoutFindingswas enabled."- I don't why this padding happened, and why it is wrong.
There was a problem hiding this comment.
Here's an example of a result before this change. As you can see in the file, there's multiple projects in the lib directory and the project contains everything. The project also contains files that belong to no package (for example the whole bin directory or the .formatter.exs file).
Taking the .formatter.exs file as an example, it does not belong to any packages but it belongs to the project. Before this change, it would be correctly listed in scan #1 as Apache-2.0 and incorrectly listed as NONE in scan result #2.
When the evaluator or reporter runs on this result, they will use both scan results since they share the provenance and since NONE is not allowed in our rules, they will report an error.
| } | ||
| } else { | ||
| deduplicatedScanResults | ||
| } |
There was a problem hiding this comment.
Could we add a commit prior to this commit which factors out line 150-152 and lines 155 - 180 into a function which is responsible for adding these LicenseFinding(SpdxConstants.NONE, TextLocation(it, TextLocation.UNKNOWN_LINE)), so that the diff of this commit becomes simpler?
e.g. idea is to separate the de-duplication code from the adding of these NONE findings.
|
|
||
| "Scanning a project with the same provenance as packages" should { | ||
| val analyzerResult = createAnalyzerResultWithProject(project0, pkg0) | ||
| val ortResult = createScanner().scan(analyzerResult, skipExcluded = false, emptyMap()) |
There was a problem hiding this comment.
Please move lines 106+107 below line 109.
| val ortResult = createScanner().scan(analyzerResult, skipExcluded = false, emptyMap()) | ||
|
|
||
| "not have duplicated scan results" { | ||
| val expectedResult = readResource("/scanner-integration-shared-project-package-provenance.yml") |
There was a problem hiding this comment.
Would it be sufficient (instead of asserting the expected result) to assert
OrtResult.scanner.scanResultshas only one entry per(provenance, scannerDetails).OrtResult.scanner.fileshas only one entry perprovenance.
I believe this what this commit ensures. Looking at these above proposed assertions, we could instead
add the as invariant checks into the constructor of ScannerRun.
(Another consideration could be to make this de-duplication unit test-able, so that the test does not need to do scanning at all, @oss-review-toolkit/core-devs what do you think?)
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.
When the SpdxDocumentFile package manager is used, the project and all contained packages often resolve to the same VCS provenance (e.g. the root of the Git repository).
Before this change ORT stored two separate
ScanResults for such a provenance – one keyed to the project, one keyed to the package.That caused two follow-on problems:
OrtResult, so evaluators saw duplicate findings for the same source tree.SpdxConstants.NONEfinding wheneverincludeFilesWithoutFindingswas enabled. The evaluator therefore compared real license findings from the project result withNONEfrom the package result and failed with a violation.This patch
(provenance, scanner)and folds them into a singleScanResult,As a consequence the evaluator now receives one consistent set of license findings per provenance / scanner, eliminating the false mismatch.
This is the first time for me writing Kotlin. Sorry if the code is not up to the usual standards.