Repository navigation
Make volta-migrate regenerate shims and add --no-create option to it - #938
Conversation
|
There's something wrong with the macOS (ARM) build. |
charlespierce
left a comment
charlespierce
left a comment
There was a problem hiding this comment.
Thanks for this @uasi! I have one question about the added flag to volta-migrate.
Also, if you're willing, would you also remove the call to regenerate_shims_for_dir in https://github.com/volta-cli/volta/blob/main/src/common.rs#L22 ? That is always called right after calling volta-migrate, and since we're now doing that same thing within volta-migrate, we don't need to duplicate it.
| raw(global_setting = "structopt::clap::AppSettings::DeriveDisplayOrder"), | ||
| raw(global_setting = "structopt::clap::AppSettings::DisableVersion") | ||
| )] | ||
| struct VoltaMigrate { |
There was a problem hiding this comment.
Can you expand a bit on the use-case behind this option? The migration is designed to be idempotent, so running it multiple times shouldn't be a concern. Additionally, Volta will absolutely not work without the directory set up (and will force it to be set up when it's run), so I'm not clear what the benefits would of calling volta-migrate and having it no-op would be.
Finally, while I agree that calling volta-migrate from the Homebrew installer makes sense, I think that volta-migrate (similar to volta-shim) is primarily intended to be an implementation detail, so I'm a little wary of adding StructOpt to it as that involves a lot of extra compilation baggage and makes it seem more like a CLI that people should think about.
There was a problem hiding this comment.
I think brew intall volta alone should not modify anything under the home directory, so volta-migrate have to be no-op unless ~/.volta exists. Not creating ~/.volta in advance is not a problem as a first-time user will probably run volta install node afterwards anyway.
On the other hand, brew upgrade volta should take care of existing shims so that the user doesn't have to run volta setup themselves again.
As for option parsing, how about just doing std::env::args().nth(1) == Some("--no-create".to_owned()) without StructOpt?
There was a problem hiding this comment.
std::env::args().nth(1) == Some("--no-create".to_owned())
std::env::args().nth(1).map_or(false, |v| v == "--no-create")
There was a problem hiding this comment.
It's a bit of a weird state, since the script installer does modify the home directory (and even modifies the profile scripts unless turned off). However, it looks like the current Homebrew installer doesn't do that (meaning users will need to run volta setup after brew install volta), and we probably shouldn't change that out from underneath them.
I think having a small flag is a reasonable, though I would prefer to do a more simplistic matching like you showed, as opposed to bringing in all the complexity of StructOpt. I think your solution is pretty good, though I would use args_os since that won't panic if there are non-unicode characters (and you can probably take advantage of the fact that OsString implements PartialEq<&str>):
matches!(std::env::args_os().nth(1), Some(flag) if flag == "--no-create")
We should also probably have a comment around that branch about how it's used by the Homebrew formula to avoid making changes to the user's system.
There was a problem hiding this comment.
matches!(std::env::args_os().nth(1), Some(flag) if flag == "--no-create")
Didn't know that this is even possible! I replaced StructOpt with matches! and added a comment about the purpose of the flag.
Will squash & rebase If there is nothing else to discuss.
|
FYI I just merged #940 that includes (among other things) the ARM update you did here, as well as the formatting fix. GitHub at the moment doesn't seem to think there are conflicts, but it's possible there will be some merge conflicts. |
No problem. I removed it. |
4379217 to
b67586c
Compare
|
Rebased onto main and dropped commits included in #940. |
b67586c to
6d7f3bb
Compare
|
|
||
| if volta_migrate.no_create && !volta_home().map_or(false, |home| home.root().exists()) { | ||
| // In order to migrate the existing Volta directory while avoiding unconditional changes to the user's system, | ||
| // the Homebrew formula runs volta-migrate with `--no-create` flag in the post-install phase. |
There was a problem hiding this comment.
If there is anything wrong with my English, please feel free to correct it :)
charlespierce
left a comment
charlespierce
left a comment
There was a problem hiding this comment.
LGTM! Thanks for this!
2 participants
Addresses #927
Background
Shims are symbolic links to
/path/to/volta-shim. If a package manager installs a new version of Volta to a different location and removes the current version, shims will be broken.Changes
volta-migrateregenerate shims even if the layout is up-to-date.--no-createoption tovolta-migrate. When specified,volta-migrateruns migration only if the Volta directory already exists.