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: improve install.sh PATH handling and general robustness #2189

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

Arcitec
Copy link
Contributor

@Arcitec Arcitec commented Oct 2, 2024

  • Exported paths for Bash and ZSH are now surrounded by double quotes, which is necessary to avoid breaking user systems where the existing PATH contains spaces.

  • The PIXI_HOME environment variable now resolves incoming ~/ prefixes to the actual user's $HOME value. Otherwise we would generate invalid paths, since ~/ is not a valid prefix in PATH. It also fixes the fact that the old code installed into a literal sub-directory of the current working dir, named ~/.pixi (a literal ~), instead of into the user's home directory, whenever users tried to provide a custom env PIXI_HOME=~/.pixi variable themselves.

  • We now use double quotes around all variables. This is required for two reasons. It ensures that arguments with spaces are supported. And it prevents Bash from breaking when a variable contains an empty string. Otherwise, various tests such as [[ $HTTP_CODE -lt 200 ]] would break with errors about not providing enough parameters.

  • The PIXI_NO_PATH_UPDATE syntax has been clarified to use :- to clearly show that it's using an empty default. The old - syntax looks more like a typo.

Closes #2188

@Arcitec
Copy link
Contributor Author

Arcitec commented Oct 2, 2024

The ~ fix uses the "replace at beginning" technique:

${var_name/#pattern/replacement}

Example:

s="something"
echo ${s/#s*e/other}

produces "otherthing".

@Arcitec
Copy link
Contributor Author

Arcitec commented Oct 2, 2024

I have no way to check if fish and tcsh need any fixes, such as ensuring that we wrap their paths in double quotes too.

I tried searching for "fish_add_path spaces" to see if they require spaces, but all I found were endless unrelated discussions. We might need this:

LINE="fish_add_path \"${BIN_DIR}\""

As for tcsh, it might need something like this:

LINE="set path = ( \"${BIN_DIR}\" \"\$path\" )"

But there's lack of documentation for those weird shells. It seems like TCSH might need \ escaping for spaces in our BIN_DIR variable. Or maybe it handles that internally if given an argument with a lot of spaces? No idea! Even trying to think about those edge cases gives me a headache:

https://superuser.com/questions/52423/space-in-directory-path-in-path-variable-in-linux

At least Bash and ZSH, the normal shells, are working now.

@Arcitec
Copy link
Contributor Author

Arcitec commented Oct 2, 2024

Example to demonstrate that it works in Bash and ZSH now:

$ cat install.sh | env PIXI_HOME=~/.local/share/"pixi with spaces" bash
$ cat ~/.zshrc
export PATH="/home/johnny/.local/share/pixi with spaces/bin:$PATH"
$ which pixi
~/.local/share/pixi with spaces/bin/pixi

@Arcitec
Copy link
Contributor Author

Arcitec commented Oct 2, 2024

Before these fixes, the shell would always break when installing Pixi. That happened regardless of whether Pixi itself contained spaces in its own BIN_DIR. Since the user's entire PATH was being rewritten in a way that broke any existing spaces in the previous $PATH value. Example:

$ cat ~/.zshrc
export PATH=/home/johnny/.local/share/pixi:$PATH

Trying to start a new shell after PATH has been broken loses a lot of core utilities and breaks completely:

zsh: wc: command not found...
Packages providing this file are:
'coreutils'
'coreutils-single'

$ ls
zsh: sort: command not found...
Packages providing this file are:
'coreutils'
'coreutils-single'
zsh: ls: command not found...
Similar commands are::
'lc'
'lz'
zsh: wc: command not found...                                                   
Packages providing this file are:
'coreutils'
'coreutils-single'

This bug happened because shells drop everything after the first space unless quoted. So the old Pixi code basically drops the entire $PATH after its first space:

$ export FOO=bar yeah
$ echo "$FOO"       
bar

Edit: I will force push a new commit message to mention another bug fixed by this PR.

- Exported paths for Bash and ZSH are now surrounded by double quotes, which is necessary to avoid breaking user systems where the existing `PATH` contains spaces.

- The `PIXI_HOME` environment variable now resolves incoming `~/` prefixes to the actual user's `$HOME` value. Otherwise we would generate invalid paths, since `~/` is not a valid prefix in `PATH`. It also fixes the fact that the old code installed into a literal sub-directory of the current working dir, named `~/.pixi` (a literal `~`), instead of into the user's home directory.

- We now use double quotes around all variables. This is required for two reasons. It ensures that arguments with spaces are supported. And it prevents Bash from breaking when a variable contains an empty string. Otherwise, various tests such as `[[ $HTTP_CODE -lt 200 ]]` would break with errors about not providing enough parameters.

- The `PIXI_NO_PATH_UPDATE` syntax has been clarified to use `:-` to clearly show that it's using an empty default. The old `-` syntax looks more like a typo.
Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Also great explanation, I learned something today!

@ruben-arts ruben-arts merged commit e6459e5 into prefix-dev:main Oct 3, 2024
42 checks passed
@Arcitec
Copy link
Contributor Author

Arcitec commented Oct 5, 2024

@ruben-arts @wolfv Happy to help. :)

I just noticed that https://pixi.sh/install.sh needs to be updated to the new version too.

@Arcitec Arcitec deleted the fix-installer branch October 6, 2024 13:14
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.

Major flaw in PATH handling of install.sh
2 participants