prefix-dev / pixi

Package management made easy
https://pixi.sh
BSD 3-Clause "New" or "Revised" License
3.38k stars 192 forks source link

fix: pixi global dynamic PATH variable #2527

Closed wolfv closed 13 hours ago

wolfv commented 1 day ago

We almost did the right thing, but then the env var was overwritten again because we also store the PATH as part of the env vars in the dictionary in JSON trampoline.

However, I am not sure that this works correctly on Windows. On my macBook, the trampoline JSON contains:

{
  "exe": "/Users/wolfv/.pixi/envs/coreutils/bin/base32",
  "path": "/Users/wolfv/.pixi/envs/coreutils/bin",
  "env": {
    "PATH": "/Users/wolfv/.pixi/envs/coreutils/bin:/Users/wolfv/.pixi/bin:/Users/wolfv/Library/pnpm:/Users/wolfv/Programs/pixi/target/release/:/Users/wolfv/Programs/rattler-build/target/release:/Users/wolfv/Programs/rattler/target/release:/opt/homebrew/share/google-cloud-sdk/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/Library/Apple/usr/bin:/Users/wolfv/.cargo/bin:/Applications/iTerm.app/Contents/Resources/utilities:/Users/wolfv/.pixi/bin",
    "CONDA_PREFIX": "/Users/wolfv/.pixi/envs/coreutils"
  }
}

The path is the correct, single entry on unix systems. On Windows, we need to add 5 paths to the PATH variable https://github.com/conda/rattler/blob/32eefc87ef0f1bc5bcc0bb65183b97e71808f54c/crates/rattler_shell/src/activation.rs#L277-L290

Maybe @baszalmstra can quickly check if we need to correct the code for Windows?

wolfv commented 1 day ago

Looks like we're only storing a single path in the trampoline JSON which isn't the correct one:

/// Root path of the original executable that should be prepended to the PATH.
pub path: PathBuf,

I think we should change that to store the prefix: PathBuf, and then derive the correct PATH in the trampoline code by copying over the bits from the activation.

baszalmstra commented 1 day ago

Yep I think youre correct.

wolfv commented 1 day ago

This changes the trampoline config by adding a field path_variables that will be prepended to the actual path. This includes the folders that were added by the (cached) activation.

We also currently store the path that is the parent path of the executable. We don't really need that variable anymore. However, for the migration we might still need to parse it - I didn't check the internals of the bin discovery enough - maybe @Hofer-Julian you can review.

I would prefer to store the path to the prefix (not the bin directory) in a prefix: PathBuf variable.

wolfv commented 1 day ago

Also to clarify the goals:

This should keep "dynamic" path things working (still a bit different than real executables though because the top-paths will always be from the environment). And it should also keep activation scripts that add to the PATH working (like the dotnet one).

The current behavior is like this:

export PATH="/foo/bar/:$PATH"

python -c "import os; print(os.environ['PATH'])"

Should now print "/home/user/.pixi/envs/python/bin/:/foo/bar/:...rest of PATH..."

I think to match user expectations perfectly, we would figure out what PATH were added completely dynamically but I don't know how to do that. We could record the PATH at creation time and do a dynamic diff, potentially, but it migth also have pitfalls ...

Hofer-Julian commented 13 hours ago

Closing this in favor of https://github.com/prefix-dev/pixi/pull/2541