chores(analyzer): Print warning if duplicate packages by bennati · Pull Request #9260 · oss-review-toolkit/ort

bennati · 2024-10-09T07:50:02Z

When duplicate packages or projects are found in the dependency tree, print a warning instead of throwing an exception and inteerrupting the scan. Duplicate packages may arise when the same package is imported twice, in these cases the dependency tree will be completed as the package is imported at least once.

Please ensure that your pull request adheres to our contribution guidelines. Thank you!

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

nnobelis · 2024-10-09T11:19:44Z

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

 * limitations under the License.
 *
 * SPDX-License-Identifier: Apache-2.0
 * License-Filename: LICENSE


Commit message: interrupting

reworded commit message

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

fviernau · 2024-10-09T11:37:35Z

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

- "Unable to create the AnalyzerResult as it contains packages and projects with the same ids: " +
- duplicates.values
+ if (duplicates.isNotEmpty()) {
+ logger.warn {"Unable to create the AnalyzerResult as it contains packages and projects with the same ids: ${duplicates.values}"}


Instead of just logging, I believe an OrtIssue should be created - because the problem can potentially be severe, so there should be no risk of going unnoticed.

However, with the current logic, it may be difficult to implement that, because the problem is detected way after the duplicate IDs got inserted, so undoing the insertion is probably no more possible.

sschuberth · 2024-10-09T11:53:51Z

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

- "Unable to create the AnalyzerResult as it contains packages and projects with the same ids: " +
- duplicates.values
+ if (duplicates.isNotEmpty()) {
+ logger.warn {"Unable to create the AnalyzerResult as it contains packages and projects with the same ids: ${duplicates.values}"}


The log message now does not reflect what you're doing: You say an analyzer result cannot be created, but still create one.

Changed log message

sschuberth · 2024-10-09T16:35:39Z

Besides the current build errors, I'm actually not sure if we want the change. I currently don't oversee all side effects of allowing duplicate identifiers.

bennati · 2024-10-10T07:18:16Z

I appreciate the need of understanding in detail all side effects of allowing duplicate identifiers.
What this proposal is meant for, is making the user aware of the possibility of these side effects, which can be manually corrected, while allowing the scan to complete in case side effects are unlikely.

In my opinion, the scan should crash only if there is a non-recoverable error. Having ORT produce a notice file with potential errors is still safer than having no notice file.

fviernau · 2024-10-10T07:30:22Z

Besides the current build errors, I'm actually not sure if we want the change. I currently don't oversee all side effects of allowing duplicate identifiers.

I agree, that side-effect are hard to oversee and probably huge. Also I am strongly for keeping identifiers unique.
What in my view would be the only option is not adding duplicate IDs to the result in the first place, and instead add an OrtIssue. This way it is indicated that something is missing while ids remain unique.

bennati · 2024-10-10T12:30:36Z

Is this what you had in mind?

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

+ logger.warn {
+ "AnalyzerResult contains packages and projects with the same ids: ${duplicates.values}"
+ }


analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

+ for ((key, items) in duplicates) {
+ val issue = createAndLogIssue(
+ source = "analyzer",
+ message = "AnalyzerResult contains packages and projects with the same ids: ${items}"
+ )
+
+ val existingIssues = issues.getOrDefault(key, emptyList())
+ issues[key] = existingIssues + issue
+ }


analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

+ for ((key, items) in duplicates) {
+ val issue = createAndLogIssue(
+ source = "analyzer",
+ message = "AnalyzerResult contains packages and projects with the same ids: ${items}"


analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

+ for ((key, items) in duplicates) {
+ val issue = createAndLogIssue(
+ source = "analyzer",
+ message = "AnalyzerResult contains packages and projects with the same ids: ${items}"


codecov · 2024-10-10T12:44:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.67%. Comparing base (546550e) to head (344d6cb).
Report is 7 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@ ## main #9260 +/- ## ========================================= Coverage 67.67% 67.67% Complexity 1187 1187 ========================================= Files 239 239 Lines 7796 7796 Branches 900 900 ========================================= Hits 5276 5276 Misses 2153 2153 Partials 367 367

Flag	Coverage Δ
funTest-non-docker	`34.71% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fviernau · 2024-10-11T09:42:42Z

Is this what you had in mind?

No it isn't. It disregards that:

Duplicates can be across projects / packages.
Removing afterwards probably has a different effect, than skipping over it when attempting to add it in the first place.
Also the scopes are already constructed, which contain reference to projects or packages and which correspond to a
project each.

...but, I also do not see a do-able way how to implement what I had in mind.

bennati · 2024-10-11T09:59:35Z

Ok, let me know if any better solution than this comes to mind which is also feasible and I can try to implement it.
Otherwise let's please merge this one so I can finally generate some notice file for my projects.

fviernau · 2024-10-11T10:21:11Z

Ok, let me know if any better solution than this comes to mind which is also feasible and I can try to implement it.

I won't have the time to look into this anytime soon.

Otherwise let's please merge this one so I can finally generate some notice file for my projects.

IMO it should not be merged, due to not understood side effects and possible broken referential integrity.

Would you be willing to invest the time / effort to find another solution @bennati ?

bennati · 2024-10-11T12:43:13Z

I can develop a different solution, but first I need someone to explain how the solution should work and the ORT maintainers to commit to merge it once developed.

bennati · 2024-11-01T10:31:30Z

Any update on this?

sschuberth · 2024-11-01T11:09:24Z

Any update on this?

I believe this PR needs to make more clear which of the three different root causes for duplicate identifiers it targets to address.

In any case, I believe it's wrong to unconditionally remove all duplicates from packages as that, depending on the root cause, could result in a loss of information.

Instead, to get started with addressing the general topic, I plan to revive #6533 which targets the case where duplicates occur between projects and packages.

dimitris-iliou · 2024-11-11T14:37:20Z

We have identified another instance related to the RapidJSON library that may need to be addressed. Specifically, the OSS Review Toolkit (ORT) reported a failure because it detected a duplicate dependency across two distinct packages:

[[Package(id=Identifier(type=SpdxDocumentFile, namespace=, name=rapidjson, version=v1.1.0), purl=pkg:generic/rapidjson@v1.1.0?download_url=https://github.com/Tencent/rapidjson/archive/refs/tags/v1.1.0.tar.gz, cpe=null, authors=[], declaredLicenses=[MIT], declaredLicensesProcessed=ProcessedDeclaredLicense(spdxExpression=MIT, mapped={}, unmapped=[]), concludedLicense=MIT, description=A fast JSON parser/generator for C++ with both SAX/DOM style API, homepageUrl=https://rapidjson.org/, binaryArtifact=RemoteArtifact(url=, hash=Hash(value=, algorithm=)), sourceArtifact=RemoteArtifact(url=https://github.com/Tencent/rapidjson/archive/refs/tags/v1.1.0.tar.gz, hash=Hash(value=, algorithm=)), vcs=VcsInfo(type=Git, url=ssh://git@main.gitlab.in.here.com:3389/professional-services/hdo/hdo-client.git, revision=2e21d71c1840c8d795ef73247b73b052acd73b8e, path=external/rapidjson), vcsProcessed=VcsInfo(type=Git, url=ssh://git@main.gitlab.in.here.com:3389/professional-services/hdo/hdo-client.git, revision=2e21d71c1840c8d795ef73247b73b052acd73b8e, path=external/rapidjson), isMetadataOnly=false, isModified=false, sourceCodeOrigins=null), Package(id=Identifier(type=SpdxDocumentFile, namespace=, name=rapidjson, version=v1.1.0), purl=pkg:generic/rapidjson@v1.1.0? download_url=https://github.com/Tencent/rapidjson/archive/refs/tags/v1.1.0.tar.gz, cpe=null, authors=[], declaredLicenses=[MIT], declaredLicensesProcessed=ProcessedDeclaredLicense(spdxExpression=MIT, mapped={}, unmapped=[]), concludedLicense=MIT, description=A fast JSON parser/generator for C++ with both SAX/DOM style API, homepageUrl=https://rapidjson.org/, binaryArtifact=RemoteArtifact(url=, hash=Hash(value=, algorithm=)), sourceArtifact=RemoteArtifact(url=https://github.com/Tencent/rapidjson/archive/refs/tags/v1.1.0.tar.gz, hash=Hash(value=, algorithm=)), vcs=VcsInfo(type=Git, url=ssh://git@main.gitlab.in.here.com:3389/professional- services/dra/clients/vehicle.git, revision=0b876fe71ff6056818fe4eebe0db31e21751679c, path=external/rapidjson), vcsProcessed=VcsInfo(type=Git, url=ssh://git@main.gitlab.in.here.com:3389/professional-services/dra/clients/vehicle.git, revision=0b876fe71ff6056818fe4eebe0db31e21751679c, path=external/rapidjson), isMetadataOnly=false, isModified=false, sourceCodeOrigins=null)]]

When duplicate packages or projects are found in the dependency tree, print a warning instead of throwing an exception and inteerrupting the scan. Duplicate packages may arise when the same package is imported twice, in these cases the dependency tree will be completed as the package is imported at least once. Signed-off-by: Stefano Bennati <stefano.bennati@here.com>

bennati requested a review from a team as a code owner October 9, 2024 07:50

github-advanced-security bot found potential problems Oct 9, 2024

View reviewed changes

nnobelis reviewed Oct 9, 2024

View reviewed changes

fviernau requested changes Oct 9, 2024

View reviewed changes

sschuberth reviewed Oct 9, 2024

View reviewed changes

bennati force-pushed the warning-duplicate-packages branch from 46924a6 to 49cb302 Compare October 9, 2024 13:30

bennati requested review from fviernau, nnobelis and sschuberth October 9, 2024 14:49

bennati force-pushed the warning-duplicate-packages branch from 49cb302 to 344d6cb Compare October 10, 2024 12:30

github-advanced-security bot found potential problems Oct 10, 2024

View reviewed changes

bennati force-pushed the warning-duplicate-packages branch from 344d6cb to 044ad84 Compare November 12, 2024 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chores(analyzer): Print warning if duplicate packages#9260

chores(analyzer): Print warning if duplicate packages#9260
bennati wants to merge 1 commit intooss-review-toolkit:mainfrom
bennati:warning-duplicate-packages

bennati commented Oct 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nnobelis Oct 9, 2024

bennati Oct 9, 2024

Uh oh!

fviernau Oct 9, 2024

sschuberth Oct 9, 2024

bennati Oct 9, 2024

sschuberth commented Oct 9, 2024

bennati commented Oct 10, 2024 •

edited

Loading

fviernau commented Oct 10, 2024

bennati commented Oct 10, 2024

Check warning

Check warning

Check warning

Check notice

codecov bot commented Oct 10, 2024

fviernau commented Oct 11, 2024 •

edited

Loading

bennati commented Oct 11, 2024

fviernau commented Oct 11, 2024 •

edited

Loading

bennati commented Oct 11, 2024

bennati commented Nov 1, 2024

sschuberth commented Nov 1, 2024

dimitris-iliou commented Nov 11, 2024

Labels

6 participants

Conversation

bennati commented Oct 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nnobelis Oct 9, 2024

Choose a reason for hiding this comment

bennati Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

fviernau Oct 9, 2024

Choose a reason for hiding this comment

sschuberth Oct 9, 2024

Choose a reason for hiding this comment

bennati Oct 9, 2024

Choose a reason for hiding this comment

sschuberth commented Oct 9, 2024

bennati commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fviernau commented Oct 10, 2024

bennati commented Oct 10, 2024

Check warning

Check warning

Check warning

Check notice

codecov bot commented Oct 10, 2024

Codecov Report

fviernau commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

bennati commented Oct 11, 2024

fviernau commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

bennati commented Oct 11, 2024

bennati commented Nov 1, 2024

sschuberth commented Nov 1, 2024

dimitris-iliou commented Nov 11, 2024

Labels

6 participants

bennati commented Oct 10, 2024 •

edited

Loading

fviernau commented Oct 11, 2024 •

edited

Loading

fviernau commented Oct 11, 2024 •

edited

Loading