Make `Pnpm` separate from `Npm` #9274

fviernau · 2024-10-11T07:16:57Z

Rewrite large parts of Pnpm from scratch to figure out all information based on pnpm CLI output, and
remove the inheritance from Npm.

Part of #9261.

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

codecov · 2024-10-11T07:31:17Z

Codecov Report

Attention: Patch coverage is 84.16667% with 19 lines in your changes missing coverage. Please review.

Project coverage is 67.67%. Comparing base (b688a9c) to head (15fa947).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...e-managers/node/src/main/kotlin/pnpm/ModuleInfo.kt	57.14%	9 Missing ⚠️
...package-managers/node/src/main/kotlin/pnpm/Pnpm.kt	91.42%	1 Missing and 5 partials ⚠️
...node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt	86.20%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9274      +/-   ##
============================================
+ Coverage     67.47%   67.67%   +0.20%     
- Complexity     1200     1232      +32     
============================================
  Files           241      244       +3     
  Lines          8506     8626     +120     
  Branches        904      911       +7     
============================================
+ Hits           5739     5838      +99     
- Misses         2403     2413      +10     
- Partials        364      375      +11

Flag	Coverage Δ
funTest-docker	`62.08% <84.16%> (+1.48%)`	⬆️
funTest-non-docker	`33.57% <ø> (ø)`
test	`37.00% <1.66%> (-0.50%)`	⬇️

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.

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

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

plugins/package-managers/node/src/funTest/kotlin/PnpmFunTest.kt

...rs/node/src/funTest/assets/projects/synthetic/pnpm/project-with-lockfile-expected-output.yml

plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt

...rs/node/src/funTest/assets/projects/synthetic/pnpm/project-with-lockfile-expected-output.yml

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

sschuberth · 2024-10-25T15:22:07Z

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

@@ -0,0 +1,177 @@
+/*
+ * Copyright (C) 2019 The ORT Project Authors (see <https://github.com/oss-review-toolkit/ort/blob/main/NOTICE>)


On a related note, I only skimmed other the code changes in this file due to a lack of time. As also the expected output changes, it's a bit hard to understand just from the diff whether the implementation is correct. However, I trust that you did the mentioned comparison to NPM results to get confidence that the new expected results are more correct than the old ones.

pnpm list outputs a tree. Maybe this helpds with checking. Comparing with npm you can easily reproduce e.g. with a difftool comparing the two expected results for babel.

Above mentioned comparison should give a good idea that the change is highly likely correct (without verifying the change manually against the list output)

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

sschuberth · 2024-10-25T15:28:26Z

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

+            Scope.DEV_DEPENDENCIES -> "--dev"
+        }
+
+        val json = run(workingDir, "list", "--json", "--recursive", "--depth", "10000", scopeOption).stdout


How can we confidently limit the depth to 10000 now if previously it was -1?

10000 prints out more than -1

Really? Isn't -1 an alias for "unlimited"?

It seem like.

Now I'm confused. If -1 is an alias for "unlimited", shouldn't -1 print out more than 10000 then, and not the other way around? And shouldn't we stick with -1 then?

So can you please explain why omitting --depth 10000 would not work?

Because then it uses the default which is -1 or 0, I don't know that. But 0 does not work either because it display only direct dependencies.

Very weird that pnpm list --help says "All dependencies are printed by default", but the docs say "pnpm ls --depth 0 (default) will list direct dependencies only".

However, apparently we can use "Infinity" as a depth value instead, which worked in my tests. Please use that value as it's less arbitrary and documents the intention.

Infinity is of course much nicer than 10000. How did you find that one?

It's in the linked docs.

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt

plugins/package-managers/node/src/main/kotlin/pnpm/Pnpm.kt

plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt

Make things consistent again in this regard. This is a fix-up for 4e19bbd. Signed-off-by: Frank Viernau <[email protected]>

Prepare for re-use in an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

Prepare for adding further `Pnpm` specific classes. Signed-off-by: Frank Viernau <[email protected]>

Stop inheriting from `Npm` and rely entirely on the output of `pnpm` commands to figure out the necessary information. For example, do not re-construct the dependency from the file structure within `node_modules`, but rely on `pnpm list` instead. This reduces complexity and makes the implementation easy to understand, maintain and change. From the diff of the expected result files it becomes aparent that this change contains the following fixes: 1. project-with-lockfile: `htmlparser2` now dependends on `domutils 1.7.0` instead of `domutils 1.5.1`, which is in line with the dependency tree output by `pnpm list` 2. workspaces: The cyclic reference from the workspace root project to itself is now included. 3. babel: A lot of changes in the dependency tree which thereby aligns with the output of `pnpm list`. Comparing the new behavior with the one from `NPM` for the Babel project shows that the differences become very small. These differences can be explained either by the two package managers indeed behaving differently or by a bug in ORT's `NPM` implementation. The latter seems likely and should be investigated. For example, by re-writing `Npm` to obtain all information solely from `npm` CLI output. Also note that it is good to drop the `--shamefully-hoist` command line option, because it is not needed and its use is discouraged. Omitting it may also reduce the amount of warnings. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the pnpm-rewrite branch from 9e82e41 to 24a0b9f Compare October 11, 2024 07:17

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

View reviewed changes

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

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

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

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

View reviewed changes

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

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

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

fviernau force-pushed the pnpm-rewrite branch from 24a0b9f to ed8f31d Compare October 11, 2024 08:14

fviernau changed the title ~~Pnpm rewrite~~ Make Pnpm separate from Npm Oct 11, 2024

fviernau force-pushed the pnpm-rewrite branch 6 times, most recently from 5e3706e to f29ee32 Compare October 11, 2024 11:05

fviernau force-pushed the pnpm-rewrite branch 2 times, most recently from 3f9123c to f590776 Compare October 24, 2024 13:38

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

View reviewed changes

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

fviernau force-pushed the pnpm-rewrite branch 3 times, most recently from 4d83590 to 21eacdd Compare October 24, 2024 20:38

fviernau marked this pull request as ready for review October 24, 2024 20:39

fviernau requested a review from a team as a code owner October 24, 2024 20:39

fviernau force-pushed the pnpm-rewrite branch 2 times, most recently from 9d3df9a to 5a995cd Compare October 25, 2024 07:20

sschuberth requested changes Oct 25, 2024

View reviewed changes

fviernau force-pushed the pnpm-rewrite branch 3 times, most recently from 957fa2f to 3c0b68d Compare October 25, 2024 10:03

fviernau requested a review from sschuberth October 25, 2024 10:10

fviernau force-pushed the pnpm-rewrite branch 3 times, most recently from fd69f17 to aa9247d Compare October 25, 2024 10:17

fviernau force-pushed the pnpm-rewrite branch from aa9247d to 273da38 Compare October 25, 2024 10:21

This comment was marked as resolved.

Sign in to view

fviernau force-pushed the pnpm-rewrite branch 2 times, most recently from 07e2f98 to 259c255 Compare October 25, 2024 12:07

This comment was marked as resolved.

Sign in to view

fviernau enabled auto-merge (rebase) October 25, 2024 12:30

sschuberth requested changes Oct 25, 2024

View reviewed changes

fviernau force-pushed the pnpm-rewrite branch from 259c255 to a5ca477 Compare October 25, 2024 14:44

fviernau requested a review from sschuberth October 25, 2024 14:44

sschuberth requested changes Oct 25, 2024

View reviewed changes

fviernau force-pushed the pnpm-rewrite branch from a5ca477 to 6a4e321 Compare October 25, 2024 16:13

fviernau requested a review from sschuberth October 25, 2024 16:14

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

View reviewed changes

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

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

fviernau force-pushed the pnpm-rewrite branch 3 times, most recently from 28e3249 to 337ffce Compare October 28, 2024 10:03

sschuberth approved these changes Oct 28, 2024

View reviewed changes

fviernau added 4 commits October 28, 2024 11:24

test(yarn2): Move the functional test into the yarn2 package

97e3391

Make things consistent again in this regard. This is a fix-up for 4e19bbd. Signed-off-by: Frank Viernau <[email protected]>

refactor(npm): Move parseProject() out of the class

529c6a9

Prepare for re-use in an upcoming change. Signed-off-by: Frank Viernau <[email protected]>

refactor(node)!: Move Pnpm into its own dedicated package

0c92e3a

Prepare for adding further `Pnpm` specific classes. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the pnpm-rewrite branch from 337ffce to 15fa947 Compare October 28, 2024 10:24

fviernau merged commit 0eb1eea into main Oct 28, 2024
21 of 23 checks passed

fviernau deleted the pnpm-rewrite branch October 28, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Pnpm` separate from `Npm` #9274

Make `Pnpm` separate from `Npm` #9274

fviernau commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

sschuberth Oct 25, 2024

fviernau Oct 25, 2024 •

edited

Loading

fviernau Oct 25, 2024

sschuberth Oct 25, 2024

fviernau Oct 25, 2024

sschuberth Oct 25, 2024

fviernau Oct 25, 2024

sschuberth Oct 25, 2024

fviernau Oct 27, 2024 •

edited

Loading

sschuberth Oct 28, 2024

fviernau Oct 28, 2024

sschuberth Oct 28, 2024

fviernau Oct 28, 2024

		@@ -0,0 +1,177 @@
		/*
		* Copyright (C) 2019 The ORT Project Authors (see <https://github.com/oss-review-toolkit/ort/blob/main/NOTICE>)

Make Pnpm separate from Npm #9274

Make Pnpm separate from Npm #9274

Conversation

fviernau commented Oct 11, 2024 • edited Loading

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

fviernau Oct 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Oct 27, 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

Make `Pnpm` separate from `Npm` #9274

Make `Pnpm` separate from `Npm` #9274

fviernau commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading

fviernau Oct 25, 2024 •

edited

Loading

fviernau Oct 27, 2024 •

edited

Loading