-
Notifications
You must be signed in to change notification settings - Fork 309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Pnpm
separate from Npm
#9274
base: main
Are you sure you want to change the base?
Conversation
9e82e41
to
24a0b9f
Compare
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
24a0b9f
to
ed8f31d
Compare
5e3706e
to
f29ee32
Compare
3f9123c
to
f590776
Compare
4d83590
to
21eacdd
Compare
9d3df9a
to
5a995cd
Compare
plugins/package-managers/node/src/funTest/kotlin/PnpmFunTest.kt
Outdated
Show resolved
Hide resolved
...rs/node/src/funTest/assets/projects/synthetic/pnpm/project-with-lockfile-expected-output.yml
Show resolved
Hide resolved
plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt
Outdated
Show resolved
Hide resolved
957fa2f
to
3c0b68d
Compare
fd69f17
to
aa9247d
Compare
aa9247d
to
273da38
Compare
This comment was marked as resolved.
This comment was marked as resolved.
07e2f98
to
259c255
Compare
This comment was marked as resolved.
This comment was marked as resolved.
...rs/node/src/funTest/assets/projects/synthetic/pnpm/project-with-lockfile-expected-output.yml
Show resolved
Hide resolved
plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt
Outdated
Show resolved
Hide resolved
259c255
to
a5ca477
Compare
@@ -0,0 +1,177 @@ | |||
/* | |||
* Copyright (C) 2019 The ORT Project Authors (see <https://github.com/oss-review-toolkit/ort/blob/main/NOTICE>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above mentioned comparison should give a good idea that the change is highly likely correct (without verifying the change manually against the list output)
Scope.DEV_DEPENDENCIES -> "--dev" | ||
} | ||
|
||
val json = run(workingDir, "list", "--json", "--recursive", "--depth", "10000", scopeOption).stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we confidently limit the depth to 10000 now if previously it was -1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10000 prints out more than -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? Isn't -1 an alias for "unlimited"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seem like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try out -1? (I wonder why you are so sure that this works)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/PnpmDependencyHandler.kt
Outdated
Show resolved
Hide resolved
plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt
Outdated
Show resolved
Hide resolved
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]>
a5ca477
to
6a4e321
Compare
plugins/package-managers/node/src/main/kotlin/pnpm/PnpmDependencyHandler.kt
Fixed
Show fixed
Hide fixed
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]>
6a4e321
to
c5f1c7e
Compare
Rewrite large parts of Pnpm from scratch to figure out all information based on
pnpm
CLI output, andremove the inheritance from
Npm
.Part of #9261.