MichaelAquilina / zsh-autoswitch-virtualenv

🐍 ZSH plugin to automatically switch python virtualenvs (including pipenv and poetry) as you move between directories
GNU General Public License v3.0
528 stars 82 forks source link

stat: bad file descriptor on execution of check_venv on OS X Mojave #110

Closed madsenwattiq closed 4 years ago

madsenwattiq commented 4 years ago

Issue Details

Please provide the following details when opening an issue:

Operating System (uname -a)

Darwin lewontinmadsenlaborg.local 18.6.0 Darwin Kernel Version 18.6.0: Thu Apr 25 23:16:27 PDT 2019; root:xnu-4903.261.4~2/RELEASE_X86_64 x86_64

zsh version (zsh --version)

zsh 5.5.1 (x86_64-apple-darwin17.5.0)

autoswitch-virtualenv version

1.14.0

How is zsh-autoswitch-virtualenv installed?

Steps to reproduce the issue

check_venv fails because "stat -f %u "$venv_path"" fails.

Simply doing "stat -f %u .venv" at command line gives:

stat: bad file descriptor

whereas "stat .venv" gives full output. Something is apparently different about the "stat" version syntax on newish (Mojave?) versions of OS X.

MichaelAquilina commented 4 years ago

Hi @madsenibis Could you check what the following command gives you?

stat -c %u .venv
madsenwattiq commented 4 years ago

On the current version of OS X, I get:

stat: bad option: -c

MichaelAquilina commented 4 years ago

what about stat --version?

madsenwattiq commented 4 years ago

Hmm. Weird.

stat: no files given

Ah! I found the problem. If you're using ZSH as your shell, stat becomes a built in shell command with decidedly non-standard options. The original stat -f %u .venv works fine if I use the path to the actual Unix executable /usr/bin/stat. How about I give you a PR which figures out how stat should be called?

MichaelAquilina commented 4 years ago

@madsenibis go for it :)

If possible please provide a test for it

madsenwattiq commented 4 years ago

Still thinking about the approach here -- it turns out that what is loading the zsh/stat plugin (and thus causing check_venv to fail given the different syntax), is a Kubernetes prompt-setting script. So it seems like perhaps the best generic approach would be to: (a) check if zsh/stat is loaded before really starting the bulk of check_venv (b) if so, call stat with an absolute path

would that be an acceptable approach, do you think?

rnc commented 4 years ago

@madsenibis @MichaelAquilina It already uses an explicit path for rm (/bin/rm) so my vote would be to keep it simple and just immediately use an explicit path ? On my Fedora system /bin is a symbolic link to /usr/bin - is /usr/bin is standard cross-platform location for stat?

madsenwattiq commented 4 years ago

What I'm testing is something where we determine the path for "stat" after determining if what's in the "path" is a shell builtin function first, which would find the executable non-shell version regardless of whether /bin or /usr/bin are separate or a symlink.

function _get_stat_path() {
    # default case is to use what's on the path
    local stat_executable="stat"

    # determine what's on the path
    local path_to_stat=`which stat`

    # if Unix stat is bypassed for zsh/stat module, ensure use of Unix stat
    if [[ $$path_to_stat =~ "built-in" ]];
    then
        stat_executable=`whence -p stat`
    fi

    printf "%s" "$stat_executable"
}

Would that work?

MichaelAquilina commented 4 years ago

@madsenibis @MichaelAquilina It already uses an explicit path for rm (/bin/rm) so my vote would be to keep it simple and just immediately use an explicit path ? On my Fedora system /bin is a symbolic link to /usr/bin - is /usr/bin is standard cross-platform location for stat?

I would prefer this solution if we are confident this will be consistent across systems.

What I'm testing is something where we determine the path for "stat" after determining if what's in the "path" is a shell builtin function first, which would find the executable non-shell version regardless of whether /bin or /usr/bin are separate or a symlink.

Alternatively this would work, but I would change the function so that it works to execute directly

function _stat() {
    # all the code you wrote stores the result in $stat_path local variable
    "$stat_path" $@
}
MichaelAquilina commented 4 years ago

@madsenibis would you mind testing out #121 to see if it works for you?

madsenwattiq commented 4 years ago

This works, thank you!