feat(Analyzer): Add a flag to skip excluded projects / scopes in dependency graph #6405

oheger-bosch · 2023-01-27T10:58:59Z

This flag indicates that dependencies in excluded scopes should not be added to the dependency graph. For large projects, this could reduce the size of the dependency graph and the analysis time significantly.

model/src/main/kotlin/config/AnalyzerConfiguration.kt

sschuberth · 2023-01-27T14:57:22Z

I'd like to just point out #5968. It would be great if this work could fully address that issue.

oheger-bosch · 2023-01-27T15:12:24Z

@fviernau, @sschuberth: This is currently a bit experimental. What I am after is indeed not a full solution for #5968, but only a first step:

We have currently problems with an Android project that frequently crashes our ORT pipeline due to resource exhaustion. In the cases the build runs through, the Analyzer step takes more than 2 hours. The time seems to be consumed mainly when constructing the dependency graph.

I did some tests in which I filtered out test scopes when adding the dependencies to the graph builder - this made the build stable and reduced the time of the Analyzer step to 8 minutes! So, I think, this intermediate step to skip excluded scopes when building the dependency graph is already beneficial - and the implementation should not be too complex.

WDYT?

sschuberth · 2023-01-27T15:31:50Z

I did some tests in which I filtered out test scopes when adding the dependencies to the graph builder - this made the build stable and reduced the time of the Analyzer step to 8 minutes!

Feels a bit like cheating 😉 More seriously, I'd really like to understand what exactly is taking so long to construct the dependency graph. Also, I wouldn't expect test scopes to have much more dependencies than "regular" scopes, so why does excluding them make such a big difference?

oheger-bosch · 2023-01-30T08:44:48Z

Feels a bit like cheating wink More seriously, I'd really like to understand what exactly is taking so long to construct the dependency graph. Also, I wouldn't expect test scopes to have much more dependencies than "regular" scopes, so why does excluding them make such a big difference?

Fair points. What we have noticed is that, starting from a certain size, adding further dependencies to the graph becomes rather slow. This is probably due to the fact that the subtrees that need to be compared become larger and larger. For Android projects with their numerous synthetic scopes this seems to be a real issue; so reducing the overall size of the graph by excluding unneeded scopes can actually have a significant effect.

Regarding "cheating": I think, handling scope excludes when constructing the dependency graph needs to be done anyway when implementing full support for exclusions as requested by #5968. So, these changes are not a side-track, but already a partial solution to this goal. I have pushed some more commits, so hopefully it becomes a bit clearer now what I try to achieve.

model/src/main/kotlin/utils/DependencyGraphConverter.kt

oheger-bosch · 2023-02-02T16:02:46Z

I have extended the PR to cover the functionality requested by #5968. Still testing whether there is something missing.

analyzer/src/funTest/assets/projects/external/git-repo-expected-output.yml

model/src/main/kotlin/config/AnalyzerConfiguration.kt

analyzer/src/main/kotlin/PackageManager.kt

analyzer/src/funTest/assets/projects/synthetic/gradle-expected-output-scopes-excludes.yml

model/src/main/kotlin/utils/DependencyGraphConverter.kt

analyzer/src/main/kotlin/Analyzer.kt

analyzer/src/main/kotlin/AnalyzerResultBuilder.kt

analyzer/src/main/kotlin/PackageManager.kt

fviernau · 2023-02-06T13:07:51Z

analyzer/src/main/kotlin/PackageManager.kt

         */
        fun findManagedFiles(
            directory: File,
-            packageManagers: Collection<PackageManagerFactory> = ALL.values
+            packageManagers: Collection<PackageManagerFactory> = ALL.values,
+            excludes: Excludes = EMPTY_EXCLUDES


The PackageManager has an instance of RepositoryConfiguration containing all excludes.
So, in case --skip-excluded is passed, the excludes are passed redundantly. Would it make sense to pass a Boolean / flag instead?

findManagedFiles() is a function of the companion object. So the RepositoryConfiguration is not available here.

analyzer/src/main/kotlin/PackageManager.kt

oheger-bosch · 2023-02-06T14:16:10Z

It is not clear to me why the commit-lint check fails. Could anybody help me understand what is wrong?

sschuberth · 2023-02-06T14:33:21Z

It is not clear to me why the commit-lint check fails. Could anybody help me understand what is wrong?

For some reason it seems to believe graph format natively (see #3825). is a foother line. No idea why. Maybe try omitting the parentheses and using a comma instead, like , see #3825.

oheger-bosch · 2023-02-06T14:57:21Z

It is not clear to me why the commit-lint check fails. Could anybody help me understand what is wrong?

For some reason it seems to believe graph format natively (see #3825). is a foother line. No idea why. Maybe try omitting the parentheses and using a comma instead, like , see #3825.

The comma was not sufficient, obviously it has problems with the '#' character. I now used the reference style.

analyzer/src/test/kotlin/AnalyzerTest.kt

model/src/main/kotlin/utils/DependencyGraphConverter.kt

addressed.

analyzer/src/funTest/assets/projects/external/git-repo-expected-output.yml

This flag determines whether the excludes defined in the repository configuration should lead to paths being skipped completely during analysis. This means, if set to true, only definition files in non-excluded paths are processed, and dependencies in non-excluded scopes are added to the dependency graph. Signed-off-by: Oliver Heger <[email protected]>

This is analogous to other model classes. The constant can be used where an `Excludes` object is required, but no elements need to be excluded. Signed-off-by: Oliver Heger <[email protected]>

This property contains the excludes to be applied during analysis, which depend on both the analyzer configuration and the configuration of the current repository. Having this property available centrally simplifies the implementation of concrete PackageManagers. Signed-off-by: Oliver Heger <[email protected]>

Before adding a dependency to the dependency graph builder, make sure that the scope is not excluded if the 'skipExcluded' flag is set. Signed-off-by: Oliver Heger <[email protected]>

The convert() function now accepts an Excludes object. The result of the conversion contains only dependencies not defined by an excluded scope. This functionality is going to be used to implement scope exclusions centrally for package managers that do not support the dependency graph format natively, see [1]. [1] oss-review-toolkit#3825 Signed-off-by: Oliver Heger <[email protected]>

Pass the configured `Excludes` to the dependency graph converter when constructing the analyzer result. That way, scope excludes are effective for all package manager implementations that do not support the dependency graph format natively. Signed-off-by: Oliver Heger <[email protected]>

Signed-off-by: Oliver Heger <[email protected]>

This allows skipping projects in specific paths already before starting the analysis. That way problematic / large projects that are irrelevant compliance-wise can be excluded leading to reduced resource usage and analysis time. Signed-off-by: Oliver Heger <[email protected]>

If the analyzer is configured to skip excludes, it passes the Excludes defined in the repository configuration to PackageManager.findManagedFiles(). That way the analyzer result will contain only projects that are not matched by a path exclude. Resolves oss-review-toolkit#5968. Signed-off-by: Oliver Heger <[email protected]>

sschuberth

LGTM, but I'd prefer to have at least one other approval from @oss-review-toolkit/kotlin-devs.

MarcelBochtler · 2023-02-07T12:07:20Z

analyzer/src/main/kotlin/managers/Yarn2.kt

@@ -338,55 +338,55 @@ class Yarn2(
                mapping += it.value
            }
        }
-        graphBuilder.addPackages(allPackages.values)


You've removed this, but you aren't changing anything below aside from applying the filter.
Was this never required, or am I missing something?

Good catch. This was never required, since the calls to addDependency() automatically add the packages to the graph builder. This would only be needed if packages were constructed in an alternative way, which is not the case here.

MarcelBochtler · 2023-02-07T12:31:40Z

model/src/main/kotlin/config/AnalyzerConfiguration.kt

+    /**
+     * A flag to control whether excluded scopes and paths should be skipped during the analysis.
+     */
+    val skipExcluded: Boolean = false


Please document this new option. I guess the best place to do this would be this file:
https://github.com/oss-review-toolkit/ort/blob/main/docs/config-file-ort-yml.md#excludes

In there, you'd also need to adjust this sentence, as this changes with this PR:

Note that the excluded parts are analyzed and scanned

Sure, this is on my agenda, but I want to get this merged first to be sure that there are no more changes in naming or otherwise. I will then open a follow-up PR.

fviernau requested changes Jan 27, 2023

View reviewed changes

model/src/main/kotlin/config/AnalyzerConfiguration.kt Outdated Show resolved Hide resolved

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch from 1c9ff9a to c023700 Compare January 30, 2023 08:25

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch 5 times, most recently from a5279a2 to 1d4db04 Compare February 1, 2023 13:57

github-advanced-security bot found potential problems Feb 1, 2023

View reviewed changes

model/src/main/kotlin/utils/DependencyGraphConverter.kt Fixed Show fixed Hide fixed

model/src/main/kotlin/utils/DependencyGraphConverter.kt Fixed Show fixed Hide fixed

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch 3 times, most recently from c4eeb22 to bb2c739 Compare February 2, 2023 16:00

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch 2 times, most recently from af9c053 to c675c27 Compare February 6, 2023 08:09

oheger-bosch marked this pull request as ready for review February 6, 2023 09:05

oheger-bosch requested a review from a team as a code owner February 6, 2023 09:05

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch from c675c27 to 8ad39d4 Compare February 6, 2023 10:05

sschuberth reviewed Feb 6, 2023

View reviewed changes

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch from 8ad39d4 to 5c1520e Compare February 6, 2023 12:31

oheger-bosch requested a review from sschuberth February 6, 2023 12:32

fviernau previously requested changes Feb 6, 2023

View reviewed changes

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch 2 times, most recently from cb0f0c7 to a49e6d8 Compare February 6, 2023 14:10

oheger-bosch requested a review from fviernau February 6, 2023 14:15

sschuberth reviewed Feb 6, 2023

View reviewed changes

analyzer/src/test/kotlin/AnalyzerTest.kt Outdated Show resolved Hide resolved

model/src/main/kotlin/utils/DependencyGraphConverter.kt Outdated Show resolved Hide resolved

model/src/main/kotlin/utils/DependencyGraphConverter.kt Outdated Show resolved Hide resolved

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch from f12d66f to 89b28b7 Compare February 6, 2023 15:12

oheger-bosch requested a review from sschuberth February 6, 2023 15:13

fviernau changed the title ~~feat(Analyzer): Add a flag to skip excluded scopes in dependency graph~~ feat(Analyzer): Add a flag to skip excluded projects / scopes in dependency graph Feb 6, 2023

sschuberth requested changes Feb 6, 2023

View reviewed changes

analyzer/src/funTest/assets/projects/external/git-repo-expected-output.yml Outdated Show resolved Hide resolved

sschuberth requested a review from a team February 6, 2023 17:02

oheger-bosch added 12 commits February 7, 2023 07:21

feat(Excludes): Add a constant for an empty instance

0ea70ca

This is analogous to other model classes. The constant can be used where an `Excludes` object is required, but no elements need to be excluded. Signed-off-by: Oliver Heger <[email protected]>

feat(Gradle): Filter out excluded scopes in the dependency graph

08c67a6

Before adding a dependency to the dependency graph builder, make sure that the scope is not excluded if the 'skipExcluded' flag is set. Signed-off-by: Oliver Heger <[email protected]>

feat(Maven): Filter out excluded scopes in the dependency graph

8a1ede9

Before adding a dependency to the dependency graph builder, make sure that the scope is not excluded if the 'skipExcluded' flag is set. Signed-off-by: Oliver Heger <[email protected]>

feat(Npm): Filter out excluded scopes in the dependency graph

7523b28

Before adding a dependency to the dependency graph builder, make sure that the scope is not excluded if the 'skipExcluded' flag is set. Signed-off-by: Oliver Heger <[email protected]>

feat(Yarn2): Filter out excluded scopes in the dependency graph

9c3725a

Before adding a dependency to the dependency graph builder, make sure that the scope is not excluded if the 'skipExcluded' flag is set. Signed-off-by: Oliver Heger <[email protected]>

style(PackageManagerTest): Fix a typo in a variable name

a42ba15

Signed-off-by: Oliver Heger <[email protected]>

oheger-bosch force-pushed the oheger-bosch/analyzer/dependency_graph_exclusions branch from 89b28b7 to 414be04 Compare February 7, 2023 06:33

oheger-bosch requested a review from sschuberth February 7, 2023 06:46

sschuberth approved these changes Feb 7, 2023

View reviewed changes

sschuberth requested a review from a team February 7, 2023 07:32

MarcelBochtler requested changes Feb 7, 2023

View reviewed changes

MarcelBochtler added the release notes label Feb 7, 2023

oheger-bosch requested a review from MarcelBochtler February 7, 2023 13:23

MarcelBochtler approved these changes Feb 7, 2023

View reviewed changes

oheger-bosch merged commit 6451027 into oss-review-toolkit:main Feb 7, 2023

oheger-bosch deleted the oheger-bosch/analyzer/dependency_graph_exclusions branch February 7, 2023 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Analyzer): Add a flag to skip excluded projects / scopes in dependency graph #6405

feat(Analyzer): Add a flag to skip excluded projects / scopes in dependency graph #6405

oheger-bosch commented Jan 27, 2023

sschuberth commented Jan 27, 2023

oheger-bosch commented Jan 27, 2023

sschuberth commented Jan 27, 2023

oheger-bosch commented Jan 30, 2023

oheger-bosch commented Feb 2, 2023

fviernau Feb 6, 2023

oheger-bosch Feb 6, 2023

oheger-bosch commented Feb 6, 2023

sschuberth commented Feb 6, 2023

oheger-bosch commented Feb 6, 2023

sschuberth left a comment

MarcelBochtler Feb 7, 2023

oheger-bosch Feb 7, 2023

MarcelBochtler Feb 7, 2023

oheger-bosch Feb 7, 2023

feat(Analyzer): Add a flag to skip excluded projects / scopes in dependency graph #6405

feat(Analyzer): Add a flag to skip excluded projects / scopes in dependency graph #6405

Conversation

oheger-bosch commented Jan 27, 2023

sschuberth commented Jan 27, 2023

oheger-bosch commented Jan 27, 2023

sschuberth commented Jan 27, 2023

oheger-bosch commented Jan 30, 2023

oheger-bosch commented Feb 2, 2023

fviernau Feb 6, 2023

Choose a reason for hiding this comment

oheger-bosch Feb 6, 2023

Choose a reason for hiding this comment

oheger-bosch commented Feb 6, 2023

sschuberth commented Feb 6, 2023

oheger-bosch commented Feb 6, 2023

sschuberth left a comment

Choose a reason for hiding this comment

MarcelBochtler Feb 7, 2023

Choose a reason for hiding this comment

oheger-bosch Feb 7, 2023

Choose a reason for hiding this comment

MarcelBochtler Feb 7, 2023

Choose a reason for hiding this comment

oheger-bosch Feb 7, 2023

Choose a reason for hiding this comment