node: Represent workspace submodules as Projects #9247

fviernau · 2024-10-07T09:12:49Z

See individual commits for the details.

Fixes #9196, fixes #8940.

Important: Previously, submodules were represented as packages which were not referenced by any project scope.
As consequence, this could lead to incorrect handling of all their transitive dependencies by the policy rules.

codecov · 2024-10-07T09:30:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.67%. Comparing base (8b07534) to head (510909b).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #9247   +/-   ##
=========================================
  Coverage     67.67%   67.67%           
- Complexity     1185     1187    +2     
=========================================
  Files           239      239           
  Lines          7795     7796    +1     
  Branches        899      900    +1     
=========================================
+ Hits           5275     5276    +1     
  Misses         2153     2153           
  Partials        367      367

Flag	Coverage Δ
funTest-docker	`60.32% <100.00%> (+0.02%)`	⬆️
funTest-non-docker	`34.71% <ø> (ø)`
test	`37.05% <50.00%> (-0.02%)`	⬇️

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-07T09:32:41Z

...kage-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces-expected-output.yml

+      - name: "dependencies"
+        dependencies:
+        - id: "PNPM::testing-pnpm-package-a:1.0.2"
+          linkage: "PROJECT_DYNAMIC"


Should we exclude the subtree of testing-pnpm-package-a, to not duplicate it. testing-pnpm-package-a now is a project and it has the dependencies in its scopes.

Good question. How do we handle it e.g. for Pub with references to Gradle projects, do we there also list the Gradle projects separately, @mnonnenmacher?

In any case, at least in the dependency graph representation the duplication should not matter much memory-wise, as it should be deduplicated.

If we decide to do this, I believe it'd be good to do this in a separate commit anyway, to have it properly documented and to not increase this commit further. So, I propose we make the discussion non-blocking for this PR, and I will make a follow-up PR if needed.

How do we handle it e.g. for Pub with references to Gradle projects, do we there also list the Gradle projects separately

There are two ways how Gradle dependencies can appear in Pub/Flutter projects:

As project dependencies, when they refer to Gradle projects in the same repository.

As package dependencies, when they refer to Android specific code in Pub packages.

In both cases we include transitive dependencies in the dependency tree.

I see two main reasons to include transitive dependencies for project dependencies:

Not all scopes are relevant, for example, in Node projects only the "dependencies" scope should be added, not the "devDependencies" scope. This is important to correctly determine excluded packages.

For some package managers the version resolution could yield different results if a project is not analyzed on its own but as a dependency of another project.

plugins/package-managers/node/src/main/kotlin/Npm.kt

...kage-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces-expected-output.yml

sschuberth · 2024-10-07T14:37:55Z

...kage-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces-expected-output.yml

+      - name: "dependencies"
+        dependencies:
+        - id: "PNPM::testing-pnpm-package-a:1.0.2"
+          linkage: "PROJECT_DYNAMIC"


Good question. How do we handle it e.g. for Pub with references to Gradle projects, do we there also list the Gradle projects separately, @mnonnenmacher?

In any case, at least in the dependency graph representation the duplication should not matter much memory-wise, as it should be deduplicated.

sschuberth · 2024-10-07T14:41:58Z

plugins/package-managers/node/src/main/kotlin/Npm.kt

-            graphBuilder.addPackages(packages)
-        }
+        return projectDirs.map { projectDir ->
+            val issues = mutableListOf(*installIssues.takeIf { projectDir == workingDir }.orEmpty().toTypedArray())


Maybe it's with to extract

installIssues.takeIf { projectDir == workingDir }.orEmpty()

to a projectIssues variable or similar?

I tried to make it less cryptic by introducing an if condition on a separate line.

plugins/package-managers/node/src/main/kotlin/Npm.kt

sschuberth

Do you know why no changes to the expected results for NPM and Yarn2 workspaces were necessary?

sschuberth · 2024-10-08T11:31:34Z

...kage-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces-expected-output.yml

@@ -50,16 +86,31 @@ analyzer:
      scopes:
      - name: "dependencies"
        dependencies:
-        - id: "NPM::chalk:4.0.0"
+        - id: "NPM::json-stable-stringify:1.0.1"


Shouldn't PNPM::pnpm-workspaces:1.0.1 also have PROJECT_DYNAMIC dependencies on the projects referred to by its workspces, i.e. on PNPM::pnpm-app-example:1.1.4, PNPM::testing-pnpm-package-a:1.0.2 and PNPM::testing-pnpm-package-b:1.0.2?

I'm actually not sure about this myself. Maybe a workspace declaration does not really express a dependency relationship by itself, and thus these projects should not be listed here.

I planned to investigate this in my following work. In pnpm I noticed that a workspace actually can have dependencies, but workspace submodules are not shown under dependencies. To me this makes sense.
I believe a workspace is just some construct which helps with development, and also helps with using a single lockfile for all projects, but it does not really matter in terms of the actual dependencies.

I propose to not add any such dependency from the workspace to the submodules. At least not in this PR. I guess there will be a better understanding on this on my side soon anyway, so if you're fine, let's defer the topic.

(my planned next step is to separate the implementations (Yarn, Pnpm, Npm) and then it will be much easier to make such changes)

sschuberth · 2024-10-08T11:47:33Z

...kage-managers/node/src/funTest/assets/projects/synthetic/yarn/workspaces-expected-output.yml

+    revision: "<REPLACE_REVISION>"
+    path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/yarn/workspaces"
+  homepage_url: ""
+  scopes: []


Continuing on my earlier question, what do you think about having a follow-up PR that creates a scope called "workspace" which does list the projects that belong to a workspace as PROJECT_DYNAMIC dependencies?

I'd tend to not do that, however, similar to my answer above, I'd propose to defer this decision.

fviernau · 2024-10-08T14:04:19Z

Do you know why no changes to the expected results for NPM and Yarn2 workspaces were necessary?

For NPM we never implemented support for workspaces / we do not support it. Yarn2 is a separate implementation, it luckily does not also inherit from NPM.

sschuberth · 2024-10-08T15:24:06Z

For NPM we never implemented support for workspaces / we do not support it.

Oh! I was mislead by us having an npm/workspaces directory, but indeed it's not being used from NpmFunTest...

Keep `searchDirs` as a `Sequence`. Signed-off-by: Frank Viernau <[email protected]>

There can be no duplicate files in the same file system location. A `Set` better reflects those semantics. Signed-off-by: Frank Viernau <[email protected]>

Reduce the diff of an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

Remove the need to worry about a good location within the function in upcoming changes of the function. Signed-off-by: Frank Viernau <[email protected]>

Prepare for a following change. Signed-off-by: Frank Viernau <[email protected]>

Previously, the submodules of any `Yarn` or `Pnpm` workspaces were represented as packages. This was inconsistent, because ORT normally represents any definition files found in the analyzed sources as a `Project`, not as a `Package`. Besides being inconsistent, the `Package` representation renders the `ort.yml` features unusable. For example, workspace submodules could not be excluded via path excludes. Furthermore, the previous implementation represented the workspace root project (in case of Pnpm) as both, as a `Package` and as a `Project`. Finally, the previous Package representation of a submodule did not have any reference from any project scope. As a consequnce, any (license) policy rules which operates only on non-excluded dependencies would have disregarded the submodules and their transitive dependencies, potentially leading to an incorrect underreporting of rule violations. Extend the `NpmModuleInfo` class by the flag `isProject` and make use of it in the `NpmDependencyHandler` for creating the packages and determining the linkage type. This change guarantees that `parsePackage()` is no more called for `Project`s, but only for `Package`s which is why the project-specific logic is dropped from `parsePackage()`. For projects the dedicated `parseProjects()` is now consistently used instead. As in the new representation there are no more unreferenced packages, the dependency handler does take care of creating all packages. So, the logic which calls `graph.addPackage()` for each module returned by `parseInstalledModules()` became unnecessary and is dropped. Note: This commit fixes multiple things at once, because it seemed too complicated to fix each issue separately due to various chicken-egg like problems. Fixes #9196, fixes #8940. Signed-off-by: Frank Viernau <[email protected]>

sschuberth · 2024-10-09T06:33:34Z

For NPM we never implemented support for workspaces / we do not support it.

Oh! I was mislead by us having an npm/workspaces directory, but indeed it's not being used from NpmFunTest...

Actually, are you sure about that @fviernau? At least the NPM code has stuff like

ort/plugins/package-managers/node/src/main/kotlin/Npm.kt

Lines 127 to 148 in 864d19f

    
               /** 
        
                * Check if [this] represents a workspace within a `node_modules` directory. 
        
                */ 
        
               protected open fun File.isWorkspaceDir() = isSymbolicLink() 
        
               /** 
        
                * Load the submodule directories of the project defined in [moduleDir]. 
        
                */ 
        
               protected open fun loadWorkspaceSubmodules(moduleDir: File): Set<File> { 
        
                   val nodeModulesDir = moduleDir.resolve("node_modules") 
        
                   if (!nodeModulesDir.isDirectory) return emptySet() 
        
                   val searchDirs = nodeModulesDir.walk().maxDepth(1).filter { 
        
                       (it.isDirectory && it.name.startsWith("@")) || it == nodeModulesDir 
        
                   } 
        
                   return searchDirs.flatMapTo(mutableSetOf()) { dir -> 
        
                       dir.walk().maxDepth(1).filter { 
        
                           it.isDirectory && it.isSymbolicLink() && it != dir 
        
                       } 
        
                   } 
        
               }

sschuberth · 2024-10-09T06:37:11Z

Maybe I just forgot to add an actual test for plugins/package-managers/node/src/funTest/assets/projects/synthetic/npm/workspaces in f99e2ed, hmm...

fviernau · 2024-10-09T07:44:25Z

Actually, are you sure about that @fviernau? At least the NPM code has stuff like

If found that quite a bit of logic has been introduced into NPM which at the time of introduction had only been used by one child class. So, I wouldn't assume per default that code in NPM is actually used for NPM.

fviernau · 2024-10-09T07:45:38Z

Maybe I just forgot to add an actual test for plugins/package-managers/node/src/funTest/assets/projects/synthetic/npm/workspaces in f99e2ed, hmm...

I saw this commit as well when working on this PR. I though that it was only about the detection, e.g. decide which package manager to use for a particular project dir / definition file.

fviernau · 2024-10-09T07:46:39Z

Note: Adding NPM workspace support was on my TODO list, and I plan to do this only after separating the package managers from one another.

edit: I ended up creating an issue, so I can simply refer to it and for transparency: #9261

sschuberth · 2024-10-09T08:09:28Z

I though that it was only about the detection, e.g. decide which package manager to use for a particular project dir / definition file.

Could very well be, I currently do not recall the details anymore.

fviernau force-pushed the node-submodules-as-projects branch from abe5c90 to d36ae67 Compare October 7, 2024 09:16

fviernau force-pushed the node-submodules-as-projects branch from d36ae67 to 4273dca Compare October 7, 2024 09:31

fviernau commented Oct 7, 2024

View reviewed changes

fviernau force-pushed the node-submodules-as-projects branch 3 times, most recently from 3c9457a to 0ffad1b Compare October 7, 2024 09:37

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

View reviewed changes

plugins/package-managers/node/src/main/kotlin/Npm.kt Fixed Show fixed Hide fixed

fviernau force-pushed the node-submodules-as-projects branch from 0ffad1b to 634af92 Compare October 7, 2024 10:41

fviernau marked this pull request as ready for review October 7, 2024 10:42

fviernau requested a review from a team as a code owner October 7, 2024 10:42

fviernau force-pushed the node-submodules-as-projects branch from 634af92 to 2afb639 Compare October 7, 2024 10:58

sschuberth requested changes Oct 7, 2024

View reviewed changes

fviernau force-pushed the node-submodules-as-projects branch from 2afb639 to bb683ef Compare October 7, 2024 17:40

fviernau requested a review from sschuberth October 7, 2024 17:40

fviernau force-pushed the node-submodules-as-projects branch 2 times, most recently from b0d9a48 to 6a5e61e Compare October 8, 2024 06:30

sschuberth reviewed Oct 8, 2024

View reviewed changes

fviernau requested a review from sschuberth October 8, 2024 14:09

sschuberth approved these changes Oct 8, 2024

View reviewed changes

sschuberth force-pushed the node-submodules-as-projects branch from 6a5e61e to 685e636 Compare October 8, 2024 16:57

sschuberth enabled auto-merge (rebase) October 8, 2024 16:57

fviernau added 6 commits October 8, 2024 22:19

refactor(npm): Reduce the amount of filterTo()s

81bab93

Keep `searchDirs` as a `Sequence`. Signed-off-by: Frank Viernau <[email protected]>

refactor(node): Use Set<File> as the type for submodule directories

7afd6c9

There can be no duplicate files in the same file system location. A `Set` better reflects those semantics. Signed-off-by: Frank Viernau <[email protected]>

refactor(npm): Use named arguments in a constructor call

9aca612

Reduce the diff of an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

chore(npm): Move a TODO comment above the function signature

42ca16b

Remove the need to worry about a good location within the function in upcoming changes of the function. Signed-off-by: Frank Viernau <[email protected]>

refactor(npm): Replace two !! with checkNotNull()

7c516d4

Prepare for a following change. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the node-submodules-as-projects branch from 685e636 to 510909b Compare October 8, 2024 20:19

fviernau disabled auto-merge October 9, 2024 06:09

fviernau enabled auto-merge (rebase) October 9, 2024 06:09

fviernau merged commit 864d19f into main Oct 9, 2024
22 of 23 checks passed

fviernau deleted the node-submodules-as-projects branch October 9, 2024 06:24

sschuberth mentioned this pull request Oct 9, 2024

Path Excludes not working when workspaces are introduced in the code repository based on NPM, PNPM, Yarn2 #8940

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: Represent workspace submodules as Projects #9247

node: Represent workspace submodules as Projects #9247

fviernau commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

fviernau Oct 7, 2024

sschuberth Oct 7, 2024 •

edited

Loading

fviernau Oct 7, 2024

mnonnenmacher Oct 8, 2024 •

edited

Loading

sschuberth Oct 7, 2024 •

edited

Loading

sschuberth Oct 7, 2024

fviernau Oct 7, 2024

sschuberth left a comment

sschuberth Oct 8, 2024

sschuberth Oct 8, 2024 •

edited

Loading

fviernau Oct 8, 2024

fviernau Oct 8, 2024

sschuberth Oct 8, 2024

fviernau Oct 8, 2024

fviernau commented Oct 8, 2024

sschuberth commented Oct 8, 2024

sschuberth commented Oct 9, 2024

sschuberth commented Oct 9, 2024

fviernau commented Oct 9, 2024

fviernau commented Oct 9, 2024

fviernau commented Oct 9, 2024 •

edited

Loading

sschuberth commented Oct 9, 2024

node: Represent workspace submodules as Projects #9247

node: Represent workspace submodules as Projects #9247

Conversation

fviernau commented Oct 7, 2024 • edited Loading

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sschuberth Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnonnenmacher Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

sschuberth Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth Oct 8, 2024 • 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

Choose a reason for hiding this comment

fviernau commented Oct 8, 2024

sschuberth commented Oct 8, 2024

sschuberth commented Oct 9, 2024

sschuberth commented Oct 9, 2024

fviernau commented Oct 9, 2024

fviernau commented Oct 9, 2024

fviernau commented Oct 9, 2024 • edited Loading

sschuberth commented Oct 9, 2024

fviernau commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

sschuberth Oct 7, 2024 •

edited

Loading

mnonnenmacher Oct 8, 2024 •

edited

Loading

sschuberth Oct 7, 2024 •

edited

Loading

sschuberth Oct 8, 2024 •

edited

Loading

fviernau commented Oct 9, 2024 •

edited

Loading