volta-cli / volta

Volta: JS Toolchains as Code. ⚡
https://volta.sh
Other
11.1k stars 235 forks source link

volta update through homebrew requires to run volta setup again #927

Open olivier-martin-sf opened 3 years ago

olivier-martin-sf commented 3 years ago

Hi,

I just upgraded volta from version 1.0.0 to version 1.0.1 using homebrew. Right after the update, I had to run volta setup again (which I already did for version 1.0.0) to have node and yarn being sourced in my path. Before running, volta setup I couldn't access to the toolchain binaries. It seems that an upgrade via homebrew is resetting the path. I don't know if it's intentional or a bug but I wanted to report nonetheless.

System information:

charlespierce commented 3 years ago

Hi @olivier-martin-sf, thanks for reporting this! I suspect it's related to how Homebrew installs the different versions, but I'm not super familiar with the specifics of that formula. I'll take a look and see if there's a way we can get it to re-target the symlinks when upgrading.

olivier-martin-sf commented 3 years ago

Thank you @charlespierce, I am not familiar either but I will try to look into it on my side as well, hopefully I will come up with some insights

uasi commented 3 years ago

Since toolchain binaries are just symbolic links to <brew-prefix>/Cellar/volta/<volta-version>/bin/volta-shim, they can be broken if brew upgrade volta removes old versions of installed volta.

% brew --version
Homebrew 3.0.1-10-g0d36d54
Homebrew/homebrew-core (git revision 8857a2; last commit 2021-02-11)
Homebrew/homebrew-cask (git revision bed9c2; last commit 2021-02-11)

% brew --prefix
/opt/brew

% ls -la ~/.volta/bin/node
lrwxr-xr-x  1 uasi  staff  43  2 12 01:14 /Users/uasi/.volta/bin/node -> /opt/brew/Cellar/volta/1.0.1/bin/volta-shim
uasi commented 3 years ago

It might be a good idea to add a post-install hook that runs volta setup. IMO less intrusive is better, something like volta setup --only-update-existing-toolchain.

https://github.com/Homebrew/homebrew-core/blob/d8f2ab2e9620208dfee599b01b83a7f4be08110f/Formula/volta.rb

diff --git a/Formula/volta.rb b/Formula/volta.rb
index 86cdd53971..55b08185e1 100644
--- a/Formula/volta.rb
+++ b/Formula/volta.rb
@@ -23,6 +23,10 @@ class Volta < Formula
     system "cargo", "install", *std_cargo_args
   end

+  def post_install
+    system "#{bin}/volta", "setup"
+  end
+
   test do
     system "#{bin}/volta", "install", "node@12.16.1"
     node = shell_output("#{bin}/volta which node").chomp
charlespierce commented 3 years ago

That makes sense @uasi, and thanks for looking into the kind of change we would need. I agree we want to make it as unobtrusive as possible. The best bet (that I can see) would be to add "regenerate shim" behavior (which we already do during volta setup) directly to the volta-migrate binary. Then we can call that directly. It also would mean that any migration happens immediately when a new version is installed, instead of later on when it is invoked.

miikka commented 3 years ago

I'd like to see this issue fixed as well. Do I understand correctly that now the only thing missing is adding a call to volta-migrate to the post-install phase of the formula, like @uasi proposes above?

charlespierce commented 3 years ago

@miikka Yes, I believe that's correct. In the post-install for the formula we should call volta-migrate --no-create (using the flag that @uasi created in that PR, that way we can regenerate the shims for updates, but retain the behavior that we don't make any profile modifications if the user has installed but not started using Volta.

miikka commented 3 years ago

I looked into it and turns out Homebrew filters the environmental variables heavily. This means that even the post-install step won't see if the user has set VOLTA_HOME. This means that we'd need some trick to figure it out. I'm not a Homebrew expert, but some options occur to me:

uasi commented 3 years ago

Sorry for leaving work on the Homebrew side unfinished.

@miikka Good point. I'm not an expert either, but I think we can/should implement both of them. For caveat, it would be better to suggest volta setup as volta-migrate --no-create is an internal command.

uasi commented 3 years ago

I modified Formula/volta.rb and ran postinstall:

diff --git a/Formula/volta.rb b/Formula/volta.rb
index e0094b803b..d4f16ee5c7 100644
--- a/Formula/volta.rb
+++ b/Formula/volta.rb
@@ -37,6 +37,19 @@ class Volta < Formula
     (fish_completion/"volta.fish").write fish_output
   end

+  def post_install
+    # Migrate ~/.volta only if exists.
+    system "#{bin}/volta-migrate", "--no-create"
+  end
+
+  def caveats
+    <<~EOS
+      If you have set $VOLTA_HOME to somewhere other than ~/.volta and have used Volta,
+      you need to run migration manually:
+        volta setup
+    EOS
+  end
+
   test do
     system "#{bin}/volta", "install", "node@12.16.1"
     node = shell_output("#{bin}/volta which node").chomp
% brew postinstall --debug volta
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FormulaLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/volta.rb
==> Postinstalling volta
==> /usr/local/Cellar/volta/1.0.4/bin/volta-migrate --no-create
Last 15 lines from /Users/uasi/Library/Logs/Homebrew/volta/post_install.01.volta-migrate:
2021-05-19 00:33:58 +0900

/usr/local/Cellar/volta/1.0.4/bin/volta-migrate
--no-create

Volta update error: Could not remove shim for "pnpm"

Please ensure you have correct permissions to the Volta directory.
Unable to write error log!
Warning: The post-install step did not complete successfully
You can try again using:
  brew postinstall volta
==> An exception occurred within a child process:
  BuildError: Failed executing: /usr/local/Cellar/volta/1.0.4/bin/volta-migrate --no-create

/usr/local/Homebrew/Library/Homebrew/formula.rb:2175:in `block in system'
/usr/local/Homebrew/Library/Homebrew/formula.rb:2111:in `open'
/usr/local/Homebrew/Library/Homebrew/formula.rb:2111:in `system'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/volta.rb:42:in `post_install'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1101:in `block (2 levels) in run_post_install'
/usr/local/Homebrew/Library/Homebrew/formula.rb:924:in `with_logging'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1100:in `block in run_post_install'
/usr/local/Homebrew/Library/Homebrew/utils.rb:558:in `with_env'
/usr/local/Homebrew/Library/Homebrew/formula.rb:1089:in `run_post_install'
/usr/local/Homebrew/Library/Homebrew/postinstall.rb:22:in `<main>'

It seems the process is sandboxed.

uasi commented 3 years ago

So the only thing we can do about this issue is adding a caveat. I'm going to send a PR to homebrew/core if the above caveat text is enough. Then I'll revert #938, now that volta-migrate --no-create is unnecessary.

EDIT: caveat should be If you have already installed tools using Volta, you need to migrate them manually: volta setup

miikka commented 3 years ago

Ah well! At least volta setup is now safe to run as #990 was merged.

carlocab commented 3 years ago

Is it possible for symlinks in ~/.volta/bin to not use Cellar paths? i.e. instead of

/Users/uasi/.volta/bin/node -> /opt/brew/Cellar/volta/1.0.1/bin/volta-shim

we should have

/Users/uasi/.volta/bin/node -> /opt/brew/opt/volta/bin/volta-shim

/opt/brew/opt/volta/bin/volta-shim will be a path that persists between upgrades, and will always point to the latest installed version of volta-shim.

uasi commented 3 years ago

Yes, that would certainly allow shims to work between upgrades. However, an upgrade may involve other changes to the structure of ~/.volta or anything, so migration may need to be performed after all.

@charlespierce Any thoughts on this?

charlespierce commented 3 years ago

@uasi @carlocab If we can come up with a way to programmatically detect that path, then I'd definitely support linking the shims to that directory. The difficulty comes in that the shims are created on-demand (e.g. when you install a new package with npm i -g), we can detect the start of the symlink chain, which is often ~/.volta/bin/npm or similar, and the end of the chain (which is the actually running executable), but it's not obvious how to detect the middle of the chain for a "stable" link point.

One possible solution to this would be to set VOLTA_INSTALL_DIR, which allows for overriding the directory in which Volta was installed. One complication with that, however, is that we don't currently write that variable into the profile scripts, so it would need to be manually updated.

Since the profile script modification only happens on volta setup, which should always be called with the "stable" location to start, we may be able to update it so that volta setup detects cases where it isn't the default value (like Homebrew) and write out the install dir variable as well. That would then cause all shims to be written to point at the stable location. We'll probably need to work through some bootstrapping issues with that (since Volta will be running before it detects the difference, we'll need to effectively set the value in local state as well as writing it to the profile script).

uasi commented 3 years ago

@charlespierce Thank you. I'm still a little worried about not running migration. For example, I think it is possible that, after upgrading, volta-shim itself will require the latest layout of ~/.volta, or volta-shim will have been replaced with a new shimming mechanism. It's unlikely though, and it may be possible to make volta-shim perform migration transparently, so I may not need to worry too much about it. Maybe we can just run migration or at least detect outdated shims and print warnings in the profile scripts.

FWIW I have looked into the status of several other version managers:

carlocab commented 3 years ago

rbenv has an open pull request to not use cellar paths in https://github.com/Homebrew/homebrew-core/pull/75996. Will probably apply it at the next version bump.

charlespierce commented 3 years ago

@charlespierce Thank you. I'm still a little worried about not running migration. For example, I think it is possible that, after upgrading, volta-shim itself will require the latest layout of ~/.volta, or volta-shim will have been replaced with a new shimming mechanism. It's unlikely though, and it may be possible to make volta-shim perform migration transparently, so I may not need to worry too much about it. Maybe we can just run migration or at least detect outdated shims and print warnings in the profile scripts.

@uasi Both volta and volta-shim already do an automatic migration if they are called and it's needed, so I don't think this will be a problem.

charlespierce commented 3 years ago

One thought here: We've already had an idea floating around for a volta doctor or volta troubleshoot type command, that could detect some common problems (e.g. the shim directory is missing from the PATH or there is another conflicting node executable earlier than Volta's). We could create that and also detect if the shims are pointing to a different location than the currently-running volta executable and show that as a possible problem. The output if issues are detected could include a call-to-action to run volta setup.

Then the formula could run that command in post-install, which would provide the appropriate messaging for possible problems, as needed. It would also give some feedback on the first install, that volta setup is necessary to get it working in the first place. The only tricky part is we'd need to make sure that it doesn't try to do an automatic migration when running that command, since that would fail due to Homebrew's sandbox.

carlocab commented 3 years ago

If we can come up with a way to programmatically detect that path, then I'd definitely support linking the shims to that directory.

The correct prefix is given by brew --prefix volta. For example:

❯ brew --prefix volta
/usr/local/opt/volta

So you should be able to find volta-shim at $(brew --prefix volta)/bin/volta-shim. You may want to cache the value of brew --prefix volta somewhere -- it's very unlikely to change on any given machine, and calling it repeatedly can get slow. (We sped that up quite a bit a while ago, but it could slow down again when someone does refactoring.)

Then the formula could run that command in post-install

This is a fine solution to me too. However, the ideal solution would be one that doesn't rely on users reading anything. In my experience, a lot of users don't read messages they get from brew.

charlespierce commented 3 years ago

The correct prefix is given by brew --prefix volta. For example:

❯ brew --prefix volta
/usr/local/opt/volta

So you should be able to find volta-shim at $(brew --prefix volta)/bin/volta-shim. You may want to cache the value of brew --prefix volta somewhere -- it's very unlikely to change on any given machine, and calling it repeatedly can get slow. (We sped that up quite a bit a while ago, but it could slow down again when someone does refactoring.)

The difficulty here is that only works when Volta was installed by Homebrew. There are other ways to install Volta where that path would definitely not be the correct one, and we need to be able to detect the proper one on the fly.

This is a fine solution to me too. However, the ideal solution would be one that doesn't rely on users reading anything. In my experience, a lot of users don't read messages they get from brew.

Agreed there, though having the command anyway would at least be a quick first step in any troubleshooting.

lobsterkatie commented 1 year ago

Any news on this? Just spent an hour chasing my tail until I realized it was only my volta-installed commands which weren't working. (I had done a general homebrew upgrade-everything pass, and it took me a while to figure out that volta had even been updated.) Something automated would be lovely!

Pyenv has you add eval "$(pyenv init --path)" to your .basrc/.zshrc/.someothershellrc. Could volta perhaps have something like that, which would check for broken links and warn the user if it finds any? (Could be as simple as: volta list includes yarn, but which yarn doesn't find anything -> print a warning.)

UPDATE: In the meantime, I've aded the following to my /zshrc:

if [[ $(volta list) =~ "yarn" && $(which yarn) =~ "not found" ]]; then
    echo "\nWARNING: Volta link to yarn is broken. Run \`volta setup\` to fix."
fi

(Note that this has to come after the export VOLTA_HOME="$HOME/.volta" and export PATH="$VOLTA_HOME/bin:$PATH" lines.)