Allow project / package "duplicates" with the same VCS location #6533

sschuberth · 2023-02-22T11:03:49Z

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

This is analogous to `addPackages()` and will be used in an upcoming test. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>

sschuberth · 2023-02-22T11:06:18Z

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

+        // Some source code repositories contain projects that are used as packages by other contained projects. So the
+        // same code is once seen as a project, and once as a package by ORT. Allow such "duplicates" as the ids
+        // actually refer to the same thing by requiring duplicates to refer to different VCS locations.
+        val realDuplicates = duplicates.filter { (_, duplicates) ->


@oss-review-toolkit/core-devs, what do you thing about this? Should we rather only keep one of these (either the project or the package) instead of keeping both of them?

I'm specifically asking as I currently don't oversee all side-effects that might occur if we keep a project and a package with the same id in the result.

I think it's likely that assumption that Id's are unique are spread all over the place. To list a few, while guessing many further things can be found:

ort/model/src/main/kotlin/OrtResult.kt

Line 451 in 19c89ff

fun getAdvisorResultsForId(id: Identifier): List<AdvisorResult> = advisorResultsById[id].orEmpty()

ort/model/src/main/kotlin/OrtResult.kt

Line 532 in 19c89ff

fun getScanResultsForId(id: Identifier): List<ScanResult> = scanResultsById[id].orEmpty()

ort/helper-cli/src/main/kotlin/commands/ListLicensesCommand.kt

Line 66 in 19c89ff

private val packageId by option(

https://github.com/oss-review-toolkit/ort/blob/main/model/src/main/kotlin/OrtResult.kt#L132

I think it's likely that assumption that Id's are unique are spread all over the place.

Right. But some of these only assume ids to be unique within projects or packages, but not within the union of both.

Right. But some of these only assume ids to be unique within projects or packages, but not within the union of both.

Yeah, that's for sure.

ISTR that there was also a problem with the WebApp report when duplicate IDs were present: The JavaScript part uses an index-based mapping to reference objects, and if there were projects and packages with the same ID, this mapping got broken.

Have you considered as alternative solution: Making sure that the Identifier.type for projects differs from Identifier.type for packages?

Actually, I was playing around with similar ideas 3 years (!) ago:

In the identifier-types branch I tried to turn an Identifier's type into a sealed class with ProjectIdentifier and PackageIdentifier sub-classes.

In the multiple-identifier-classes branch the Identifier class itself is tried to be turned in a sealed class.

Both approaches had the goal to distinguish project and package ids, and to iterate over all known id types (e.g. for when statements).

I agree with @fviernau that there are probably lots of places in the code which assume that identifiers are unique and it could be very hard to find all of them, and with @oheger-bosch that likely the WebApp report and probably other report formats would break.
There are also situations where the same package could appear twice with different metadata (e.g. if different repositories are used), I'm not sure if we currently fail in this case or just ignore it.
I think properly addressing this issue would be a big effort, another approach than the sealed class you suggested could also be to extend the identifier with properties like isProject and provider details.

I think properly addressing this issue would be a big effort

Indeed. That's why I never completed my branches mentioned above...

There are also situations where the same package could appear twice with different metadata (e.g. if different repositories are used), I'm not sure if we currently fail in this case or just ignore it.

I verify this. We have a repo in which ORT is failing because three or four packages appear twice due to different metadata.

…tion For some background information see [1], and specifically the NPM project / package at [2]. [1]: https://gitlab.eclipse.org/eclipsefdn/emo-team/eclipsefdn-ort/-/issues/56#note_1084923 [2]: https://git.eclipse.org/r/plugins/gitiles/cdt/org.eclipse.cdt/+/7c6bd5bdcb4cf8c31050aeeb102250a27a157728/qt/org.eclipse.cdt.qt.core/acorn-qml/package.json Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>

fviernau · 2023-02-22T12:20:25Z

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

@@ -46,9 +46,18 @@ class AnalyzerResultBuilder {

    fun build(excludes: Excludes = Excludes.EMPTY): AnalyzerResult {
        val duplicates = (projects.map { it.toPackage() } + packages).getDuplicates { it.id }
-        require(duplicates.isEmpty()) {
+


I've got some story maybe worth sharing here: I recently scanned ORT with ORT locally,
it exited complaining in about duplicates in this function. I debugged it only quick and had to stop at some point due to other tasks. If I didn't make a mistake the finding was that there were multiple packages (not projects) with the same id which were completely identical except for on attribute. IIRC it was the homepage URL.

That's just FYI, didn't look into this PR deeply yet .

I also had the case of multiple packages with the same identifier. This happened when a package was referenced from multiple package managers using the same package type (for instance GoMod and GoDep). If the package metadata is not exactly the same, multiple Package instances are created.

In my above case the full identifiers were identical, including the type.

gmathiou4 · 2023-03-30T14:05:06Z

The change have been tested and is fine from our side

sschuberth · 2023-03-30T14:37:02Z

The change have been tested and is fine from our side

Thanks for the feedback. However, this PR will more likely evolve into the direction of dropping the duplicate package in favor of only keeping the project.

refactor(AnalyzerResultBuilder): Introduce an addProject() function

e98a536

This is analogous to `addPackages()` and will be used in an upcoming test. Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>

sschuberth commented Feb 22, 2023

View reviewed changes

sschuberth force-pushed the lenient-project-package-duplicates branch from d9327fc to 7b1c5c9 Compare February 22, 2023 11:07

fviernau reviewed Feb 22, 2023

View reviewed changes

sschuberth mentioned this pull request Feb 23, 2023

Dotnet: Add support for nuget-inspector #6209

Merged

sschuberth marked this pull request as ready for review February 23, 2023 13:50

sschuberth requested a review from a team as a code owner February 23, 2023 13:50

sschuberth requested review from a team and removed request for a team February 23, 2023 13:50

sschuberth marked this pull request as draft February 27, 2023 10:00

sschuberth mentioned this pull request Mar 13, 2023

ORT analyzer keeps scanning node_modules files #6664

Closed

sschuberth mentioned this pull request May 17, 2023

AnalyzerResultBuilder: improve error message for duplicated packages #6465

Open

ToBeHH added a commit to bex-technologies-GmbH/oss-review-toolkit that referenced this pull request Feb 2, 2024

Applied patch for oss-review-toolkit#6533

c59980e

sschuberth mentioned this pull request Feb 29, 2024

Fails in monorepo with multiple package.json files with same deps #8365

Closed

sschuberth mentioned this pull request Apr 8, 2024

Failing to analyze - with Cargo failed to resolve dependencies for path 'rust-client/Cargo.toml': NoSuchElementException: Collection contains no element matching the predicate. #8480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow project / package "duplicates" with the same VCS location #6533

Allow project / package "duplicates" with the same VCS location #6533

sschuberth commented Feb 22, 2023

sschuberth Feb 22, 2023

sschuberth Feb 23, 2023

fviernau Feb 23, 2023 •

edited

sschuberth Feb 23, 2023

fviernau Feb 23, 2023

oheger-bosch Feb 23, 2023

sschuberth Feb 23, 2023

mnonnenmacher Feb 23, 2023

sschuberth Feb 23, 2023

dimitris-iliou Jun 21, 2023

fviernau Feb 22, 2023

oheger-bosch Feb 23, 2023

fviernau Feb 23, 2023

gmathiou4 commented Mar 30, 2023

sschuberth commented Mar 30, 2023 •

edited

Allow project / package "duplicates" with the same VCS location #6533

Are you sure you want to change the base?

Allow project / package "duplicates" with the same VCS location #6533

Conversation

sschuberth commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Feb 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmathiou4 commented Mar 30, 2023

sschuberth commented Mar 30, 2023 • edited

fviernau Feb 23, 2023 •

edited

sschuberth commented Mar 30, 2023 •

edited