SPDX reporter: Stop creating dangling relationships to excluded packages #7488

fviernau · 2023-09-12T09:55:37Z

Please see individual commits.

codecov · 2023-09-12T10:25:52Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (e74531f) 68.03% compared to head (6eff9b1) 68.03%.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #7488   +/-   ##
=========================================
  Coverage     68.03%   68.03%           
  Complexity     2023     2023           
=========================================
  Files           344      344           
  Lines         16727    16727           
  Branches       2371     2371           
=========================================
  Hits          11381    11381           
  Misses         4363     4363           
  Partials        983      983

Flag	Coverage Δ
funTest-non-docker	`36.51% <ø> (ø)`
test	`35.52% <ø> (ø)`

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.

sschuberth · 2023-09-21T13:58:37Z

plugins/reporters/spdx/src/main/kotlin/SpdxDocumentModelMapper.kt


-            ortResult.getDependencies(project.id, 1).mapTo(relationships) { dependency ->


Just wondering, can't the same bug also appear around line 94, if a non-excluded package depends on an excluded package?

In general, I wonder whether a more general solution to the problem would be to also teach getDependencies() an omitExcluded parameter. If we had that, the fix would be a matter of just setting omitExcluded = true in current line 73 and 94, right?

Just wondering, can't the same bug also appear around line 94, if a non-excluded package depends on an excluded package?

I believe so as well, good catch. If I'm not mistaken, in that case it is way more of an edge case compared to above, because IIUC it happens only if a package X appears at least twice in dependency tree (one time included, one time excluded), whereas the two subtrees rooted at X differ.

In general, I wonder whether a more general solution to the problem would be to also teach getDependencies() an omitExcluded parameter.

I've decided against that, because it seemed to me that the function as-is is already too use case specific (and in particular encouraging mis-use) for the location it resides. I'm saying that, because to me it's not obvious that it merges the subtrees for identical identifiers. Anyhow, maybe the omitExcluded wouldn't make it that much worse. What do you think?

If we had that, the fix would be a matter of just setting omitExcluded = true in current line 73 and 94, right?

No, because that would not solve point #2 mentioned in the commit message.

No, because that would not solve point #2 mentioned in the commit message.

I anyway have another question regard that point. You write (typo in "depdendencies" BTW):

2. Dependencies which are direct depdendencies of a sub-project, but not of any root project are not considered a first level dependency. Such dependencies may not be linked into the dependency tree of resulting SPDX document at all.

I'm unsure about what "may not be linked ... at all" exactly means. So if I had (all non-excluded) a project structure like

root-project | +- sub-project | +- package (dependency of "sub-project" only)

are you saying there should be no relationship between "sub-project" and "package" being recorded in the SPDX document?

root-project-1 | +- sub-project-1 | +- package 1 root-project-2 | +- sub-project-2 | +- package 2

If we have above, and consider the original (current) algorithm implemented by you, I guess the intention was to not mention subproject-1 and subproject-2 in the SPDX report, but link the dependencies directly to root-project-1 and root-project-2.

To my understanding the implementation did not match this mentioned intention, which I wanted to fix with this new implementation.

are you saying there should be no relationship between "sub-project" and "package" being recorded in the SPDX document?

Given the above, I'd say no, because "package" should be a direct child of the "root" project, and nested submodules should not be modeled in the output.

I'm unsure about what "may not be linked ... at all"

If I'm not mistaken in my above tree, the current algorithm would produce a SPDX document where there is no path from any root to "package 2" and / or to "package 1". (Due to the limitation of depth==1)

I guess the intention was to not mention subproject-1 and subproject-2 in the SPDX report, but link the dependencies directly to root-project-1 and root-project-2.

Actually no, that was not my intention. The intention was to create all of the following relationships in this case:

root-project-1 depends on sub-project-1

sub-project-1 depends on package 1

(And the same for the number 2 projects / packages.)

In my opinion, only listing the top-level projects is better than exposing the whole sub-module structure.

Well, that differs from what @tsteenbe and others have expressed here then. The intent of my b471544 was to partly address that linked issue.

And we have the same ask to capture the full dependency tree for CycloneDX, BTW.

Would that be ok with you?

Not really, to be honest, as that takes us a step back again from solving #4099. However, I completely agree that I've introduced a bug in b471544 by disregarding excludes. That should certainly be fixed. And if I have introduced another bug related to sub-projects, that should be fixed, too.

Well, that differs from what @tsteenbe and others have expressed here then. The intent of my b471544 was to partly address that linked issue.

I understand Thomas' comment differently, however. It's not mentioning sub-modules explicitly, so this is IMO speculation. I believe we should discuss sub-module case explicitly / separately.

And we have the #3906, BTW.

Ok.

Not really, to be honest, as that takes us a step back again

Why is it a step back? My PR does not reduce information reported at all. But fixes this bug:

However, I completely agree that I've introduced a bug in b471544 by disregarding excludes.

So, can we use this PR as-is as a bug fix, and then moving forward discuss how exactly we want sub-module structure be represented in ORT community and then in a next iteration implement that?

So even if each entity (project or package) only relates to its direct dependencies, in the end we get a full dependency tree in total.

I'm not sure if this bug was clear to you already, but this sentence illustrates it pretty well. The idea does not work in case there are sub-projects, because then one does not iterate over the sub-projects dependencies at all in some cases.

root-project-1 | +- sub-project-1 | +- package 1 root-project-2 | +- sub-project-2 | +- package 2

[...]

Given the above, I'd say no, because "package" should be a direct child of the "root" project, and nested submodules should not be modeled in the output.

To me it would be misleading to report "package-1" as a direct dependency of "root-project-1" here, especially if both "root-project-1" and "sub-project-1" are published. I do not yet understand the issue with the current approach to build the tree, especially:

The idea does not work in case there are sub-projects, because then one does not iterate over the sub-projects dependencies at all in some cases.

Under which circumstances would that be the case?

@sschuberth @mnonnenmacher -

I've adjusted the implementation to what I understood We agreed upon in todays call.
@sschuberth I've incorporated your proposal from the PR comments to teach getDependencies() to omit excluded IDs.

fviernau · 2023-09-25T11:25:32Z

I've split out the first commit as it will become rather unrelated to the bug fix, see #7567.

sschuberth · 2023-09-26T06:37:58Z

plugins/reporters/spdx/src/main/kotlin/SpdxDocumentModelMapper.kt

+            ortResult.getDependencies(
+                id = project.id,
+                maxLevel = 1,
+                omitExcluded = false


At this commit, the omitExcluded parameter does not exist yet and should be removed here and below.

sschuberth · 2023-09-26T06:38:54Z

plugins/reporters/spdx/src/main/kotlin/SpdxDocumentModelMapper.kt

@@ -70,7 +70,11 @@ internal object SpdxDocumentModelMapper : Logging {
                ortResult
            )

-            ortResult.getDependencies(project.id, 1).mapTo(relationships) { dependency ->
+            ortResult.getDependencies(
+                id = project.id,


Commit message nit: It's not "a type parameter" but "typed parameters" as also id is named now.

sschuberth · 2023-09-26T06:41:34Z

model/src/main/kotlin/OrtResult.kt

        val dependencies = mutableSetOf<Identifier>()
+        val matcher = DependencyNavigator.MATCH_ALL.takeUnless { omitExcluded } ?: { !isExcluded(it.id) }


How about introducing a DependencyNavigator.MATCH_NON_EXCLUDED for { !isExcluded(it.id) }?

That's not possible, because the DependencyNavigator does not know about the excluded state.

Use named arguments to make it more obvious was the `1` is about. Also, use named arguments for `id` for consistency. Signed-off-by: Frank Viernau <[email protected]>

sschuberth · 2023-09-26T06:57:23Z

model/src/main/kotlin/OrtResult.kt

@@ -188,17 +188,19 @@ data class OrtResult(
    /**
     * Return the dependencies of the given [id] (which can refer to a [Project] or a [Package]), up to and including a
     * depth of [maxLevel] where counting starts at 0 (for the [Project] or [Package] itself) and 1 are direct
-     * dependencies etc. A value below 0 means to not limit the depth.
+     * dependencies etc. A value below 0 means to not limit the depth. If [omitExcluded] is set to true, idenfitifers of


Sorry, typo in "idenfitifers".

This is needed in a following change. Signed-off-by: Frank Viernau <[email protected]>

As of [1] the SPDX document was changed to have separate entries for all projects and sub-projects instead of a single artificial root project containing all dependencies. While excluded packages are not included in the package, the implementation [1] accidentally creates (dangling) relationships to such excluded packages, see [2]. Fix the issue visible in [2] by the code change further up and an analog issue not visible in the expected result diff with the code change some lines further down. Fixes #7487. [1] b471544 [2] b471544#diff-6de35dd2aff1f92b7f5ea558d3f77e02d0d596dd4ce2a8199056cfb31b47fcabR181-R184 Signed-off-by: Frank Viernau <[email protected]>

fviernau requested a review from a team as a code owner September 12, 2023 09:55

fviernau changed the title ~~SPDX reporter: Fix first level dependency rel~~ SPDX reporter: Fix first level dependency relationships Sep 12, 2023

fviernau force-pushed the spdx-reporter-first-level-deps-issues branch from b9fa474 to 28df37b Compare September 12, 2023 09:57

fviernau added release notes reporter About the reporter tool labels Sep 12, 2023

fviernau force-pushed the spdx-reporter-first-level-deps-issues branch from 28df37b to 6d779c0 Compare September 12, 2023 10:20

fviernau force-pushed the spdx-reporter-first-level-deps-issues branch 4 times, most recently from 91cd08a to c27cec6 Compare September 21, 2023 13:40

sschuberth reviewed Sep 21, 2023

View reviewed changes

fviernau requested a review from sschuberth September 21, 2023 14:24

fviernau force-pushed the spdx-reporter-first-level-deps-issues branch from c27cec6 to 18bac2e Compare September 25, 2023 13:02

fviernau changed the title ~~SPDX reporter: Fix first level dependency relationships~~ SPDX reporter: Stop creating dangling relationships to excluded packages Sep 25, 2023

fviernau requested a review from mnonnenmacher September 25, 2023 13:04

sschuberth requested changes Sep 26, 2023

View reviewed changes

chore(SpdxDocumentModelMapper): Remove some magic values

03d09e4

Use named arguments to make it more obvious was the `1` is about. Also, use named arguments for `id` for consistency. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the spdx-reporter-first-level-deps-issues branch from 18bac2e to e5ca10d Compare September 26, 2023 06:55

fviernau requested a review from sschuberth September 26, 2023 06:55

fviernau enabled auto-merge (rebase) September 26, 2023 06:55

sschuberth reviewed Sep 26, 2023

View reviewed changes

fviernau added 2 commits September 26, 2023 09:02

feat(OrtResult): Allow getDependencies() to omit excluded IDs

c47ca16

This is needed in a following change. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the spdx-reporter-first-level-deps-issues branch from e5ca10d to 6eff9b1 Compare September 26, 2023 07:02

fviernau requested a review from sschuberth September 26, 2023 07:02

sschuberth approved these changes Sep 26, 2023

View reviewed changes

fviernau merged commit 74ba431 into main Sep 26, 2023
22 checks passed

fviernau deleted the spdx-reporter-first-level-deps-issues branch September 26, 2023 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPDX reporter: Stop creating dangling relationships to excluded packages #7488

SPDX reporter: Stop creating dangling relationships to excluded packages #7488

fviernau commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading

sschuberth Sep 21, 2023

fviernau Sep 21, 2023 •

edited

Loading

sschuberth Sep 21, 2023

fviernau Sep 21, 2023 •

edited

Loading

sschuberth Sep 21, 2023 •

edited

Loading

sschuberth Sep 21, 2023

fviernau Sep 21, 2023 •

edited

Loading

fviernau Sep 21, 2023

mnonnenmacher Sep 21, 2023

fviernau Sep 25, 2023

fviernau commented Sep 25, 2023

sschuberth Sep 26, 2023

sschuberth Sep 26, 2023

sschuberth Sep 26, 2023

fviernau Sep 26, 2023

sschuberth Sep 26, 2023 •

edited

Loading


		ortResult.getDependencies(project.id, 1).mapTo(relationships) { dependency ->

		val dependencies = mutableSetOf<Identifier>()
		val matcher = DependencyNavigator.MATCH_ALL.takeUnless { omitExcluded } ?: { !isExcluded(it.id) }

SPDX reporter: Stop creating dangling relationships to excluded packages #7488

SPDX reporter: Stop creating dangling relationships to excluded packages #7488

Conversation

fviernau commented Sep 12, 2023 • edited Loading

codecov bot commented Sep 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

fviernau Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

sschuberth Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Sep 21, 2023 • edited Loading

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

fviernau commented Sep 25, 2023

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

sschuberth Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

fviernau commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 12, 2023 •

edited

Loading

fviernau Sep 21, 2023 •

edited

Loading

fviernau Sep 21, 2023 •

edited

Loading

sschuberth Sep 21, 2023 •

edited

Loading

fviernau Sep 21, 2023 •

edited

Loading

sschuberth Sep 26, 2023 •

edited

Loading