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

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

@@ -17,6 +17,8 @@
 * 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.

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 <[email protected]>

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.

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

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 commented Oct 9, 2024

nnobelis Oct 9, 2024

bennati Oct 9, 2024

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

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

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

Are you sure you want to change the base?

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

Conversation

bennati commented Oct 9, 2024

nnobelis Oct 9, 2024

Choose a reason for hiding this comment

bennati Oct 9, 2024

Choose a reason for hiding this comment

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

fviernau commented Oct 10, 2024

bennati commented Oct 10, 2024

codecov bot commented Oct 10, 2024

Codecov Report

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

bennati commented Oct 10, 2024 •

edited

Loading

fviernau commented Oct 11, 2024 •

edited

Loading

fviernau commented Oct 11, 2024 •

edited

Loading