pixi icon indicating copy to clipboard operation
pixi copied to clipboard

fix: use `home_path()` in self-update location check

Open jxu10 opened this issue 1 year ago • 9 comments

Since install.sh installs to $PIXI_HOME/bin/pixi, I think it's reasonable for pixi self-update to match the installer's logic for determining the "default pixi binary path".

jxu10 avatar Apr 19 '24 23:04 jxu10

It's intentional to not use that here, to prevent breaking pixi installation via package managers.

/// Update pixi to the latest version or a specific version. If the pixi binary is not found in the default location
/// (e.g. `~/.pixi/bin/pixi`), pixi won't updated to prevent breaking the current installation (Homebrew, etc).
/// The behaviour can be overridden with the `--force` flag.

chawyehsu avatar Apr 20 '24 00:04 chawyehsu

Some cases I can think of are the following (/opt/homebrew is used as an example of a package manager directory):

  1. ~/.pixi/bin/pixi without PIXI_HOME: update (no change)
  2. ~/.pixi/bin/pixi with PIXI_HOME=~/.pixi2: abort (~~no change~~ CORRECTION: new behavior)
  3. ~/.pixi2/bin/pixi with PIXI_HOME=~/.pixi2: update (new behavior)
  4. /opt/homebrew/bin/pixi without PIXI_HOME: abort (no change)
  5. /opt/homebrew/bin/pixi with PIXI_HOME=~/.pixi2: abort (no change)
  6. /opt/homebrew/bin/pixi with PIXI_HOME=/opt/homebrew: update (new behavior)

In the last case, it seems unlikely that package managers would set PIXI_HOME, since Pixi does not require it to run, so PIXI_HOME pointing to the package manager directory is likely to be an explicit setting the user. If the user does do this, pixi global install and install.sh will happily overwrite binaries in /opt/homebrew/bin without warning, so there is still a logic mismatch between those commands (which consider PIXI_HOME) and self-update (which doesn't).

#779 discusses package manager detection, but that doesn't seem to be implemented yet.

Outside of the scope of this "fix" PR, an alternative implementation would be to gate self-update behind an opt-in build-time feature flag that is only enabled on Pixi's GitHub release binaries, but that probably requires more discussion in another issue/Discord.

jxu10 avatar Apr 20 '24 01:04 jxu10

Thanks for you write up, I was thinking the same thing as @chawyehsu. But your explanation makes sense to me. I'm going to approve it. @wolfv What do you think?

ruben-arts avatar Apr 22 '24 19:04 ruben-arts

Sorry, I realized I made a mistake in my reasoning: ~/.pixi/bin/pixi with PIXI_HOME=~/.pixi2 (case 2) will abort, which is new behavior. Commenting for visibility, though I think my reasoning still applies.

jxu10 avatar Apr 22 '24 20:04 jxu10

In the last case, it seems unlikely that package managers would set PIXI_HOME

Just want to point out that in my case this is already happened (I submitted the PIXI_HOME feature implementation and this is the primary use case where I use it, making pixi with its envs fully portable). While I'm not completely against this change, it does add new behaviors which may break the installation (primarily a versioning mismatch between pixi binary and the metadata of pm) and surprise users, compared to the current situation where --force can be used to force the overwrite though.

I don't know if there are other places using PIXI_HOME the same way as mine.

chawyehsu avatar Apr 23 '24 00:04 chawyehsu

Sorry, I realized I made a mistake in my reasoning: ~/.pixi/bin/pixi with PIXI_HOME=~/.pixi2 (case 2) will abort, which is new behavior. Commenting for visibility, though I think my reasoning still applies.

So that would abort for the same reason that the "non curl" install would abort right? @chawyehsu I'm not getting how this breaks for your use-case, could you help me understand?

ruben-arts avatar Apr 25 '24 19:04 ruben-arts

friendly ping @chawyehsu

ruben-arts avatar Apr 30 '24 11:04 ruben-arts

@ruben-arts Sorry I missed your first ping.

I'm not getting how this breaks for your use-case, could you help me understand?

Before this patch, pixi self-upgrade abort when trying to upgrade pixi installed in $PIXI_HOME that is not the default ~/.pixi (this can be escaped with the --force flag), it will do the upgrade no matter the $PIXI_HOME is ~/.pixi or else after this patch.

So for instance, I installed pixi (e.g. v0.11.0) with a packager manager that sets $PIXI_HOME to somewhere, before this patch I can't do pixi self-upgrade to upgrade it to a newer versionm which is expected, but after this patch, pixi self-upgrade will upgrade/overwrite the pixi binary (to e.g. v0.13.0) that is installed with the packager manager, after the overwrite a mismatch of the version between pixi --version (v0.13.0) and <the package manager> list pixi (v0.11.0) is introduced.

This is what @jxu10 talked about in their comment above

/opt/homebrew/bin/pixi with PIXI_HOME=/opt/homebrew: update (new behavior)

Again I'm not completely against this change, since the overwrite will eventually be triggered by the user using pixi self-upgrade (they have to know exactly what they are doing and are responsible for the mismatch), but it does introduce a new behavior that could confuse our users.

I personally won't use pixi self-upgrade as I install pixi via pacakge managers.

chawyehsu avatar Apr 30 '24 12:04 chawyehsu

Now I think Iunderstand it, thanks @chawyehsu!

To summarize, the important changes:

  • ~/.pixi/bin/pixi with PIXI_HOME=~/.pixi2: abort (new behavior)
    • Possibly unwanted if a user has a different location for it's binary then its global environments/config
  • ~/.pixi2/bin/pixi with PIXI_HOME=~/.pixi2: update (new behavior)
    • New wanted behavior as the user has used the PIXI_HOME as intended from the start.

I don't mind this new behavior but the warning should help the user a little more.

❯ PIXI_HOME=~/bla pixi self-update
  × pixi is not installed in the default location:
  │ 
  │ - Default pixi location: /home/rarts/bla/bin/pixi
  │ - Pixi location detected: /home/rarts/.cargo/bin/pixi
  │ 
  │ It can happen when pixi has been installed via a dedicated package manager (such as Homebrew on macOS).
  │ You can always use `pixi self-update --force` to force the update.

I think this should include that the $PIXI_HOME was set and because of that it acts like this. (Don't mind the cargo location as that is a development build)

ruben-arts avatar Apr 30 '24 13:04 ruben-arts

Close as stale

ruben-arts avatar Aug 19 '24 15:08 ruben-arts