-
Notifications
You must be signed in to change notification settings - Fork 245
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
feat: Initial pnpm support #1273
Conversation
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Thanks for the PR |
Any update on this? |
@chawyehsu this is excellent to see! 👏🏼 I approved CI to run. @charlespierce or @mikrostew if either of you has time to look and see what needs to be done for the global intercept, that would be fantastic. 🤩 |
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
@chriskrycho Thanks! I fixed tests on Unix according to the CI result. |
The GH setting is such that I’ll have to reapprove every time, so I'll try to keep an eye on this to keep rerunning it – sorry about the hassle on that front! |
I didn't notice there is a I'll try to fix them today when I have a couple of hours to figure out. |
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.
Thanks for this! Apologies for taking a bit longer to get to it than I expected, it's been a very busy couple of weeks at work 😅
I haven't finished reading through the meatiest parts (resolving, parsing, etc.), but left a few minor style comments on the surrounding infrastructure. Will follow-up later in the week with a review on the core parts, as well as some potential thoughts on the FIXME
comments.
Super excited for this to land!
This allows rust-analyzer to recognize tests modules. For development convenience. Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
…g for pnpm Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Any news on this boyz ? Need it |
@ScreamZ Sorry no ETA, let's wait for the team back from their main work. |
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.
Thank you again! Took a look through the meat of the code and it's all reasonable. Also points to a lot of things it would be good for us to clean up re: Adding package support (lots of boilerplate).
Left a few (mostly minor) suggestions. For the ones in parser.rs
, I think it's totally reasonable to resolve them with FIXME
comments, as I see the overall global interception isn't totally wired up yet anyway.
@@ -87,6 +88,10 @@ impl PackageManager { | |||
if let PackageManager::Yarn = self { | |||
command.env("npm_config_global_folder", self.source_root(package_root)); | |||
} | |||
|
|||
// FIXME: Find out if there is a good way to redirect pnpm global installs |
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.
Thanks for leaving this comment! Totally reasonable to leave this as a follow-up, I know we've done some research and it's possible, so we should be able to add that on later.
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.
From my research, I think there will be much work to implement global commands manipulation for pnpm. pnpm relies on absolute path symlinks heavily (binaries generation, symlinked node_modules
structure), this makes it very difficult for volta to install and update global packages via pnpm without a layout change. I don't know if there is a proper way to update those generated symlinks after moving packages from the temp directory to the image directory to ensure they won't be broken. Related annotation in e830d3d#diff-80dfff59af9a37069dea094c900c30eafd02dd51c2dea21d150a017d1a86c9d3R90.
@@ -622,6 +674,7 @@ impl SandboxBuilder { | |||
ok_or_panic! { fs::create_dir_all(node_cache_dir()) }; | |||
ok_or_panic! { fs::create_dir_all(node_inventory_dir()) }; | |||
ok_or_panic! { fs::create_dir_all(package_inventory_dir()) }; | |||
ok_or_panic! { fs::create_dir_all(pnpm_inventory_dir()) }; |
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.
Random question: Since we aren't doing a migration (and all users will be without this directory to start out once this is released), do these tests work if this is left out? Ideally we would leave this directory uncreated and allow it to be automatically created as needed. Totally reasonable if the tests don't support that at the moment, however.
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.
This is just for the sandbox tests. The inventory directory is ensured to exist by the fetch function in production, https://github.com/volta-cli/volta/pull/1273/files#diff-cecf323599e6ac86e26655f65f1935e8d8435f00e05003ad7e228a9dc29d056cR92-R94
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 looks like this is not needed - I removed this line and the tests still pass (for me locally on OSX).
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
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.
This will also need to add the pnpm
and pnpx
shims to the get_shim_list_deduped()
fn in crates/volta-core/src/shim.rs
, for Unix systems to get the new shims (looks like Windows is fine though).
Otherwise I agree with @charlespierce, it seems reasonable not to block on pnpm globals and leave that unimplemented for now.
Sorry there is so much boilerplate needed to onboard a new package manager.
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
It's easy to unblock global commands by simply commenting out the global commands guard in |
Could we get this PR merged as-is with a disclaimer that We'd really like to use this in our workflows for local development and Docker builds, so the missing global commands support is not really impacting us. |
I second that: pnpm ships often enough that running |
Hi folks, thanks for all your interest, response, and emoji reactions to this. Undoubtedly, I hope we could make this happen as possible, Volta and pnpm are both great tools for me to be involved in the ecosystem, and I'm happy with them. Therefore I made the PR to try to improve these everyday tooling things a little bit more productive and efficient, with a micro goal of hunting some bounties because of my state of out-of-work. The waiting time was a little longer than I expected, expected two months at most for rolling this out. But it's definitely up to the Volta team, I'm not one of its members hence I ain't able to push it to go further. The team is so busy I guess, let's wait a bit more time and we will see. If you have a strong demand for it right now, you may build a nightly version based on my branch. It's also the base of the local build that I'm currently using. |
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.
Exciting! A couple of small changes to make sure we're not impacting the performance of the existing flows with unused code or extra data copying.
To the release strategy, I agree with the plan of merging this and then releasing experimental support. Then we can take another look at the global command interception and look to get that working to remove the experimental label.
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Changes are applied. A side note here: When a user updates to a version of Volta that has first-class pnpm support, if they have installed pnpm (as a package) before they might need to uninstall it manually by |
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.
That's a great callout re: existing pnpm installed as a package! We'll probably want to think a bit about how we want to message that before publishing the release as the latest version. I can make the build however, which will allow people to try it out.
Thank you @chawyehsu! |
Glad that! CI artifacts are now available to try https://github.com/volta-cli/volta/actions/runs/3383641697 |
Just tried, seems to work, now we need a way to auto-update volta ahah v1.1.0 released |
Also tested the artifact above and it works 🥳 What does the official release cycle look like for this binary to be available? |
Does this take pnpm/node version compatibility into account? |
@PindaPixel Didn't have any extra implementation for the node version compatibility, the compatibility check will be passed to pnpm. |
Hey @charlespierce just wanted to check on when this PR will be officially released |
@smblee See this comment; there's no ETA as of now. |
This PR is going to implement the first-class support for the pnpm package manager.
Thanks to @mikrostew for the well-written volta-cli/rfcs#46, I've been able to implement this feature initially. Most of the critical functionalities have been implemented, except for the global package management. Due to a lack of further details on how to intercept pnpm to relocate its global package installs to fit Volta's current layout.
Implemented
--pnpm
and--no-pnpm
Not Implemented
In fact the argument parser
for_pnpm()
has been introduced to try to intercept pnpm args, but I found that there is no proper way to make pnpm's global installs whose original location is<path to pnpm home>/global/5/node_modules
fit Volta's current global package image layout<path to volta home>/tools/image/packages
. Hence I intercepted all--global
pnpm commands and marked them as unimplemented.Trackers
RFC: volta-cli/rfcs#46
Tracking issue: #737
This is currently a draft PR, please leave your review or comment if you have any ideas to improve the PR to push forward.
Demo