search

NixOS / nix

Nix, the purely functional package manager
https://nixos.org/
GNU Lesser General Public License v2.1
12.94k stars 1.53k forks source link

Profile scripts add duplicate entries to `PATH` in subshells #5950

Open lilyball opened 2 years ago

lilyball commented 2 years ago

Describe the bug

The Nix profile scripts unconditionally add directories to PATH without checking if those directories are already there. The multi-user profile script has a guard variable to prevent it from running multiple times, but this guard variable is not exported and so does not affect subshell behavior. The single-user profile script doesn't even have a guard variable.

The effect of this is that running an interactive subshell (if multi-user, or a login subshell if single-user) will source the profile script and add the directories to PATH again even though they're already there.

Steps To Reproduce

  1. Install Nix in an existing system
  2. Spawn a new shell. Verify that $PATH includes your nix profile bin directories.
  3. Spawn a subshell (a login subshell if you have a single-user install)
  4. Check what $PATH contains

Expected behavior

$PATH should contain your nix profile bin directories exactly once in both the parent shell and the subshell.

Additional context

I have not actually reproduced this problem locally, as I am not currently set up to test standalone Nix installs (the machines I have access to use either NixOS or nix-darwin and I'm not prepared to run a VM at the moment). But this can be verified by reading the profile scripts, and this issue was originally reported to me at https://github.com/lilyball/nix-env.fish/issues/11 in my fish plugin that imports the bash profile.

One potential fix is to just export the __ETC_PROFILE_NIX_SOURCED guard variable (and add this to the single-user profile too as that doesn't even have a guard variable right now), but I don't think that's a good fix as sudo will preserve PATH but not the other variables (and especially not the guard variable) and so running an interactive or login shell via sudo would still add the duplicate PATH entries.

Given that, I think the best approach is to just make sure the profile script is idempotent. I should be able to source it multiple times in a row (unsetting the guard variable each time) and end up with the same environment that I do if I only source it once. The guard variable still has a purpose with this approach, both as an optimization and to prevent re-sourcing the profile from blowing away any subsequent environment modifications (the installer sources the profile from potentially multiple files involved in shell initialization, and I might update the earliest such file to change the environment and so the subsequent re-sourcing of the profile shouldn't blow that away). Making the script idempotent simply requires making it check if the entries are in PATH before setting them, as all other environment changes it does are already idempotent, though it's probably also worth putting a comment in the file noting that it should be idempotent as a caution for anyone modifying it in the future.

n8henrie commented 2 years ago

A little relevant discussion (and my current PATH-deduplication workarounds): https://github.com/NixOS/nix/issues/5298

SuperSandro2000 commented 2 years ago

I solved it with two lines in my bashrc which work universally:

DEDUPE_PATH="$(printf %s "$PATH" | awk -v RS=: '{ if (!arr[$0]++) {printf("%s%s",!ln++?"":":",$0)}}')"
export PATH=$DEDUPE_PATH
n8henrie commented 2 years ago

Pretty clever! The ternary in the printf confused me for a bit.

I put a similar deduplicator in the same thread, though I didn't care for the extra ms added to my shell spawn time.

In my testing you can shave off a ms or so by using a herestring instead of piping from printf (and I've added a little more spacing, which helped me understand what was going on):

awk -v RS=: '{ if (!arr[$0]++) { printf("%s%s", !ln++ ? "" : ":", $0) }}' <<<"${PATH}"

Also, the version I'm using is no slower and included for comparison:

$ cat run.sh
#!/bin/bash

orig() {
  printf %s "${PATH}" | awk -v RS=: '{ if (!arr[$0]++) {printf("%s%s",!ln++?"":":",$0)}}'
}

herestring() {
  awk -v RS=: '{ if (!arr[$0]++) {printf("%s%s",!ln++?"":":",$0)}}' <<< "${PATH}"
}

mine() {
  env --ignore-environment HOME="${HOME}" sh -c '
  source /etc/static/bashrc
  echo "${PATH}"
  '
}

if [[ -n "$(herestring)" ]] && [[ "$(orig)" = "$(herestring)" ]]; then
  export -f orig herestring mine
  hyperfine -m 500 orig
  hyperfine -m 500 herestring
  hyperfine -m 500 mine
fi
$ ./run.sh
Benchmark 1: orig
  Time (mean ± σ):       4.5 ms ±   0.5 ms    [User: 1.9 ms, System: 1.3 ms]
  Range (min … max):     3.8 ms …   8.4 ms    500 runs
...
Benchmark 1: herestring
  Time (mean ± σ):       3.8 ms ±   0.4 ms    [User: 1.9 ms, System: 1.3 ms]
  Range (min … max):     3.3 ms …   7.2 ms    500 runs
...
Benchmark 1: mine
  Time (mean ± σ):       3.7 ms ±   0.4 ms    [User: 0.9 ms, System: 1.3 ms]
  Range (min … max):     2.4 ms …   8.6 ms    500 runs
lilyball commented 2 years ago

You could try checking SHLVL and only doing this if it's >1. That way it won't run on your initial login shell.

@n8henrie That said, if you're using nix-darwin, you shouldn't be sourcing the nix profile script at all. Your /etc/bashrc should be overwritten by nix-daemon to be a symlink to /etc/static/bashrc, which sets up your environment appropriately. And child shells should then skip the PATH setup (using __NIX_DARWIN_SET_ENVIRONMENT_DONE). And if you wipe that var and run a child shell it will just replace PATH entirely anyway.

So if you're sourcing the nix profile separately you should clean that up. If you had a single-user install on your machine then it probably added the single-user nix profile to your .bash_profile, .bash_login, or .profile scripts, as well as your .zshenv or .zshrc scripts (if these existed). The multi-user installer will have instead modified /etc/bashrc, which nix-darwin would have replaced.

This is of course assuming that programs.bash.enable is set (which it is by default). Zsh configuration would be programs.zsh.enable but that's actually off by default, so if you are using Zsh then you'll want to turn that on.

n8henrie commented 2 years ago

That said, if you're using nix-darwin, you shouldn't be sourcing the nix profile script at all.

Thanks @lilyball -- I don't think I am: https://github.com/NixOS/nix/issues/5298#issuecomment-1019589001

I'm sourcing /etc/static/bashrc directly.

I use tmux, and my .bashrc sets up my PATH (synced across multiple distros). Because it appends to PATH, it ends up with an increasingly long PATH every time it gets sourced. My workaround for this for years was to start with PATH=$(getconf PATH) at the beginning of .bashrc, so it builds it up from scratch.

EDIT: For example, .bashrc gets sourced when I open Terminal.app, then again when I launch tmux, and occasionally when I make changes to .bashrc and re source it. My env (including __NIX_DARWIN_SET_ENVIRONMENT_DONE) gets passed along which mucks things up with this approach.

This led to obvious problems with nix and nix-darwin trying to decide whether or not the PATH was correct by looking at __NIX_DARWIN_SET_ENVIRONMENT_DONE and such (which seemed like an odd decision anyway -- why not just look at PATH to see if PATH is correct)? More details in https://github.com/NixOS/nix/issues/5298 if interested.

In the end, sourcing /etc/static/bashrc at the beginning of my .bashrc and having it reset my path seems like a reasonable replacement for PATH=$(getconf PATH).

¯\_(ツ)_/¯

lilyball commented 2 years ago

I'm also curious about the speed of doing a bash-native deduplication, something like

newpath=:
while read -d : path; do
    if [[ $newpath != *:$path:* ]]; then
        newpath+=$path:
    fi
done <<<"$PATH:"
newpath=${newpath#:}
PATH=${newpath%:}

This won't stop the profile script from potentially moving the nix paths in front of anything else of course, but it should behave like the awk script except without spawning any additional processes.

That said, we really should just fix the profile scripts to be idempotent.

n8henrie commented 2 years ago

Good call!

$ cat run.sh
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: run.sh
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ #!/bin/bash
   2   │
   3   │ orig() {
   4   │   printf %s "${PATH}" | awk -v RS=: '{ if (!arr[$0]++) {printf("%s%s",!ln++?"":":",$0)}}'
   5   │ }
   6   │
   7   │ herestring() {
   8   │   awk -v RS=: '{ if (!arr[$0]++) {printf("%s%s",!ln++?"":":",$0)}}' <<< "${PATH}"
   9   │ }
  10   │
  11   │ mine() {
  12   │   env --ignore-environment HOME="${HOME}" sh -c '
  13   │   source /etc/static/bashrc
  14   │   echo "${PATH}"
  15   │   '
  16   │ }
  17   │
  18   │ bashnative() {
  19   │   newpath=:
  20   │   while read -r -d : path; do
  21   │     if [[ "${newpath}" != *:"${path}":* ]]; then
  22   │       newpath+=${path}:
  23   │     fi
  24   │   done <<< "${PATH}:"
  25   │   newpath=${newpath#:}
  26   │   echo "${newpath%:}"
  27   │ }
  28   │
  29   │ if [[ -n "$(herestring)" ]] && [[ "$(orig)" = "$(herestring)" ]] && [[ "$(bashnative)" = "$(orig)" ]]; then
  30   │   export -f orig herestring mine bashnative
  31   │   hyperfine -m 500 orig
  32   │   hyperfine -m 500 herestring
  33   │   hyperfine -m 500 mine
  34   │   hyperfine -m 500 bashnative
  35   │ fi
───────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
$ ./run.sh
Benchmark 1: orig
  Time (mean ± σ):       4.7 ms ±   0.3 ms    [User: 2.0 ms, System: 1.5 ms]
  Range (min … max):     3.9 ms …   7.5 ms    500 runs
...
Benchmark 1: herestring
  Time (mean ± σ):       4.1 ms ±   0.2 ms    [User: 2.0 ms, System: 1.3 ms]
  Range (min … max):     3.8 ms …   6.5 ms    500 runs
...
Benchmark 1: mine
  Time (mean ± σ):       3.7 ms ±   0.4 ms    [User: 1.0 ms, System: 1.4 ms]
  Range (min … max):     2.8 ms …   6.2 ms    500 runs
...
Benchmark 1: bashnative
  Time (mean ± σ):       1.7 ms ±   0.1 ms    [User: 1.9 ms, System: 0.5 ms]
  Range (min … max):     1.5 ms …   2.8 ms    522 runs
...
stevecheckoway commented 2 years ago

It seems this issue could be solved similar to how Cargo does it.

This line could become something like

case ":$PATH:" in
  *:"$HOME/.nix-profile/bin:@localstatedir@/nix/profiles/default/bin":*)
    ;;
  *)
    export PATH="$HOME/.nix-profile/bin:@localstatedir@/nix/profiles/default/bin:$PATH"
    ;;
esac
SuperSandro2000 commented 2 years ago

That way it won't run on your initial login shell.

For my usecase I also want to run it in the login shell because I already have duped PATHs at that level.

This won't stop the profile script from potentially moving the nix paths in front of anything else of course, but it should behave like the awk script except without spawning any additional processes.

Ordering for PATH is super important for reboot traps, safe-rm and sudo wrappers.

n8henrie commented 2 years ago

My strategy of "starting PATH fresh" (described above) continues to be problematic.

On either of these, using nix shell nixpkgs#hello doesn't add hello to resulting shell's PATH -- it tries but is apparently scrubbed by sourcing ~/.bashrc after PATH is modified.

So I think the "forgiveness over permission" approach of cleaning duplicates at the end is probably a better idea, will try out @lilyball's recommendation.

n8henrie commented 2 years ago

By no longer creating the PATH from scratch (with a clean env) every time, it looks like I've now traded one problem for another:

$ which -a bash
/run/current-system/sw/bin/bash
/bin/bash
$ bash -c 'which -a bash'
/run/current-system/sw/bin/bash
/bin/bash
$ bash --login -c 'which -a bash'
/bin/bash
/run/current-system/sw/bin/bash

Login shells now "see" that the nix is already in the environment, and I think the MacOS path_helper is placing the system PATH before my nix stuff. I'm a heavy tmux user, which by default spawns a login shell, so inside tmux I'm getting e.g. MacOS's BSD sed instead of nix's GNU-sed. What a pain.

EDIT: Fix for now by having tmux not use a login shell (set -g default-command "${SHELL}").

madsonic commented 1 year ago

Another approach to the problem on macOS with zsh. I try to avoid manipulating PATH directly which gets quite hairy when when path_helper is involved.

  1. comment out the export PATH line
  2. prepend profile dir to /etc/paths e.g. 
    /Users/foo/.nix-profile/bin
/nix/var/nix/profiles/default/bin
...

This works well even with tmux. my $PATH are the same in terminal on startup and in tmux. no duplicates

/Users/foo/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/MacGPG2/bin

path_helper is invoked in /etc/zprofile, its behaviour is documented here. Some of its behaviour are not documented but from my testing I believe the constructed PATH follows this format

</etc/paths>:</etc/paths.d/>:<all other paths in current $PATH>

for example, given an existing PATH /sbin:/usr/local/foo/bin:/baz and the following

cat /etc/paths
/sbin

tree /etc/paths.d
.
└── /usr/local/foo/bin

when a shell is spawned the resulting $PATH is

/sbin:/usr/local/foo/bin:/baz

Perhaps the way to fix the dup path issue is to utilise path_helper by adding nix paths to /etc/paths during the installation phase.

steerio commented 10 months ago

I used the Arch Linux package, so your mileage may vary, but this works for me. You may want to check your /etc/profile.d/nix-daemon.* files whether they use this environment variable.

In your .tmux.conf:

set-environment -g __ETC_PROFILE_NIX_SOURCED 1

In your Bash or Zsh profile:

unset __ETC_PROFILE_NIX_SOURCED

Why the profile.d script doesn't check the existence of $NIX_PROFILES instead is beyond me.

carschandler commented 9 months ago

This is a pretty big issue in my opinion. If I have a python environment loaded up that I want to be persisted through an entire tmux session, I can't do that because PATH will just be overwritten with my nix-profile path at the front as soon as the new shell spawns, containing the global python instance I have defined rather than the one I just had loaded up.

carschandler commented 8 months ago

A bash-native workaround is to add this to a home-manager config:

programs.bash.bashrcExtra = ''
NIX_PATHS="$HOME/.nix-profile/bin:/nix/var/nix/profiles/default/bin:"
NEWPATH=''${PATH/$NIX_PATHS}
while [[ $NEWPATH =~ $NIX_PATHS ]]; do
  PATH=$NEWPATH
  NEWPATH=''${NEWPATH/$NIX_PATHS}
done
''

Simply removes the paths string until it's gone, leaving the result with only one match as the PATH.

chaoky commented 7 months ago

similar problem happens in zsh, nix-daemon.sh keeps prepending to path, causing other /nix/ paths (set by direnv) to not have priority on new shells

added this as a workarround in zshrc

NIX_PATHS=$(echo $PATH | tr ':' '\n' | grep "/nix/" | tail -n +2 | tr '\n' ':')
if [[ $NIX_PATHS ]]; then
  PATH=$NIX_PATHS$PATH
fi
ramboman commented 6 months ago

I have duplicated nix bin paths as well. There are several criterias my solution has to respect while solving the problem:

Here is my solution when placed in a separated script (nix_paths_idempotent.sh):

#!/bin/sh

keep_chosen_lines_unique () {
    # stdin: lines
    local chosen_lines=$1

    local awk_code='
    BEGIN {
    '"$( \
        echo "$chosen_lines" \
        | tr ' ' '\n' \
        | sed 's|\(.*\)|is_presents["\1"]="false"|')"'
    } {
        is_present=is_presents[$0]
        if (length(is_present) > 0) {
            if (is_present == "false") {
                print $0
                is_presents[$0]="true"
            }
        } else {
            print $0
        }
    }
    '
    # stdin
    awk "$awk_code"
}

match () {
    local pattern=$1
    local string=$2

    eval "
    case \"\$string\" in
        $pattern) return 0 ;;
    esac"
    return 1
}

insert_before () {
    # stdin: lines
    local section=$1
    local pattern=$2

    while read line; do
        if match "$pattern" "$line"; then
            echo "$section"
            echo "$line"
            break
        else
            echo "$line"
        fi
    done # stdin
    cat # stdin
}

get_nix_bin_paths () {
    local nix_profiles=$1

    if [ -z "${nix_profiles}" ]; then
        return 0
    fi
    local output=$( \
        echo "x $nix_profiles" \
        | tr ' ' '\n' \
        | tac \
        | sed 's|$|/bin|' \
        | tr '\n' ':')
    echo "${output%:x/bin:}"
}

insert_unique_paths_after_home () {
    local paths=$1
    local new_paths=$2

    if [ -n "${paths}" ]; then
        if [ -n "${new_paths}" ]; then
            local new_lines=$( \
                echo "$new_paths" \
                | tr ':' '\n' \
                | tac)
            local output=$( \
                echo "${new_paths}:${paths}:x" \
                | tr ':' '\n' \
                | tac \
                | insert_before "$new_lines" "$HOME/*" \
                | keep_chosen_lines_unique "$new_lines" \
                | tac \
                | tr '\n' ':')
            echo "${output%:x:}"
        else
            echo "$paths"
        fi
    else
        if [ -n "${new_paths}" ]; then
            echo "$new_paths"
        fi
    fi
}

test_insert_unique_paths_after_home () {
    local nix_profiles=$1
    local paths=$2

    local nix_bin_paths=$(get_nix_bin_paths "$nix_profiles")
    local output=$(insert_unique_paths_after_home "$paths" "$nix_bin_paths")

    echo "input:"
    echo "$paths"
    echo
    echo "output:"
    echo "$output"
    echo
}

test_all () {
    local nix_profiles="/nix/var/nix/profiles/default $HOME/.nix-profile"

    echo "Empty PATH"
    test_insert_unique_paths_after_home "$nix_profiles" ""
    echo

    echo "PATH without $HOME/* + trailing empty paths"
    test_insert_unique_paths_after_home "$nix_profiles" \
        "/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:::"
    echo

    echo "PATH with repeated $HOME/bin + trailing empty paths"
    test_insert_unique_paths_after_home "$nix_profiles" \
        "$HOME/bin:/usr/local/bin:$HOME/bin:/usr/bin:/bin:/usr/local/games:/usr/games:::"
    echo

    echo "PATH with repeated $HOME/bin + trailing empty paths + repeated nix bin paths"
    local nix_bin_paths=$(get_nix_bin_paths "$nix_profiles")
    test_insert_unique_paths_after_home "$nix_profiles" \
        "$nix_bin_paths:$HOME/bin:/usr/local/bin:$HOME/bin:$nix_bin_paths:/usr/bin:/bin:/usr/local/games:/usr/games:::"
    echo
}

"$@"

insert_unique_paths_after_home is the function that keeps the PATH idempotent while adding nix bin paths.

To test do:

$ ./nix_paths_idempotent.sh test_all

Here is my result:

Empty PATH
input:

output:
/home/user/.nix-profile/bin:/nix/var/nix/profiles/default/bin

PATH without /home/user/* + trailing empty paths
input:
/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:::

output:
/home/user/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:::

PATH with repeated /home/user/bin + trailing empty paths
input:
/home/user/bin:/usr/local/bin:/home/user/bin:/usr/bin:/bin:/usr/local/games:/usr/games:::

output:
/home/user/bin:/usr/local/bin:/home/user/bin:/home/user/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/bin:/bin:/usr/local/games:/usr/games:::

PATH with repeated /home/user/bin + trailing empty paths + repeated nix bin paths
input:
/home/user/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/home/user/bin:/usr/local/bin:/home/user/bin:/home/user/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/bin:/bin:/usr/local/games:/usr/games:::

output:
/home/user/bin:/usr/local/bin:/home/user/bin:/home/user/.nix-profile/bin:/nix/var/nix/profiles/default/bin:/usr/bin:/bin:/usr/local/games:/usr/games:::