Skip to content
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

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jxu10
Copy link

@jxu10 jxu10 commented Apr 19, 2024

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".

@chawyehsu
Copy link
Contributor

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.

@jxu10
Copy link
Author

jxu10 commented Apr 20, 2024

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.

@ruben-arts
Copy link
Contributor

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?

@jxu10
Copy link
Author

jxu10 commented Apr 22, 2024

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.

@chawyehsu
Copy link
Contributor

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.

@ruben-arts
Copy link
Contributor

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
Copy link
Contributor

friendly ping @chawyehsu

@chawyehsu
Copy link
Contributor

chawyehsu commented Apr 30, 2024

@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.

@ruben-arts
Copy link
Contributor

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 ruben-arts marked this pull request as draft April 30, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants