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 (507ee30) to head (c5f1c7e).

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?

Did you try out -1? (I wonder why you are so sure that this works)

pnpm list --help | grep depth --depth -1 Display only projects. Useful in a monorepo. `pnpm ls -r --depth -1` lists all projects in a monorepo --depth <number> Max display depth of the dependency tree --depth 0 Display only direct dependencies

I wonder why you are so sure that this works

Because it's being used in the existing code. But now I realize the use is in a different context that does not apply here.

Anyway, the question remains why it is valid to artificially limit the depth of the dependency tree now at an (arbitrary?) number of 10000. I would have expected no limit at all, or if there was a limit, it should be configurable via a package manager specific option to go into the direction of #5626.

To my understanding there is simply no way to make the command work without a limit.
So, using a large value in my opinion makes sense. I don't think 10k is too small. In fact it is so large that it is only a theroretical problem IMO. ORT's dependency graph breaks the cycles anyway and I doubt the depth limit could be exceeded without printing cycles.

If you don't like it still, please make a counter proposal.

Because it's being used in the existing code. But now I realize the use is in a different context that does not apply here.

I don't know what context you are talking about. But previously existing code didn't call this command at all.

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]>

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

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

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 added 3 commits October 25, 2024 18:13

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

4a519c9

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

a7e7b91

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

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

6d4fcaa

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

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 from 6a4e321 to c5f1c7e Compare October 25, 2024 16: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 25, 2024

fviernau Oct 26, 2024

sschuberth Oct 26, 2024

fviernau Oct 26, 2024 •

edited

Loading

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

Are you sure you want to change the base?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau Oct 26, 2024 • edited Loading

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 26, 2024 •

edited

Loading