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

[WIP/RFC]: use separate shim paths per version #1185

Closed
wants to merge 40 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jul 12, 2018

Main motivation: which foo should not return a pyenv shim in case foo is installed in some environment, but not the current.
This is a main issue with pyenv, and this fixes it.

Ref: #1112

Add PYENV_SHIM, and use it with remove_from_path

This is required to avoid infinite recursion with "system" version.

Could also use PYENV_SHIM_PATH, but having PYENV_SHIM is more
flexible/useful.

TODO:

  • make pyenv-shell and pyenv-global update $PATH. Currently it only works for "pyenv-local" via cd hook (using zsh-autoenv).
  • needs to remove any shim files in the root dir probably, otherwise it fails when there is a shim "foo" already, and rehash gets called for a version called "foo"
  • pyenv-virtualenv: Also should run (the new) rehashing when activating a virtualenv directly (not using the chpwd hook).

Uses the following chpwd handler (Zsh), e.g. via
zsh-autoenv:

  _my_autoenv_pyenv_chpwd() {
    # Experimental: maintain separate shims dir for pyenv.
    local version
    if [[ -z "$PYENV_VERSION" ]]; then
      # version=$(pyenv version-name)

      # Optimized version of pyenv-version-name:
      local -a pyenv_version_file
      setopt localoptions extendedglob
      pyenv_version_file=(./(../)#.python-version(NY1:A))
      if (( $#pyenv_version_file )); then
        version=$(<$pyenv_version_file[1])
      else
        if [[ -f "$PYENV_ROOT/version" ]]; then
          version=$(<$PYENV_ROOT/version)
        else
          version=system
        fi
      fi
    else
      version=$PYENV_VERSION
    fi

    if [[ -z "$_PYENV_AUTO_VERSION" || "$_PYENV_AUTO_VERSION" != "$version" ]]; then
      _PYENV_AUTO_VERSION=$version
      out=$(PATH=$PYENV_ROOT/libexec:$PATH pyenv-sh-shell --updated-path $version | grep '^PATH')
      if [[ -z "$out" ]]; then
        echo "chpwd: pyenv-sh-shell $version failed"
      fi
      eval "$out"
    fi
  }
  add-zsh-hook chpwd _my_autoenv_pyenv_chpwd

Drawbacks:

  • calls pyenv version-name every time you change a directory (if
    pyenv-shell was not used). This could be optimized e.g. by looking for the
    .python-version file only.

/cc @yyuu @mislav

@blueyed
Copy link
Contributor Author

blueyed commented Jul 12, 2018

Works quite well for pyenv-shell now already - but no support for fish yet.

As for pyenv-local, should we install a chpwd hook ourselves?
I know that you can have several functions in Zsh, but do not know if there is only a global one in Bash/fish.

@blueyed blueyed changed the title [WIP/RFC]: use separate shim paths per shell/version [WIP/RFC]: use separate shim paths per version Jul 12, 2018
shopt -s extglob
# Remove any shim dir from PATH.
path_without_shims="$(remove_from_path "${PYENV_ROOT}/shims*([^:])")"
PYENV_COMMAND_PATH="$(PATH="$path_without_shims" command -v "$PYENV_COMMAND" || true)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This required change breaks github.com/yyuu/pyenv-which-ext, but it is also not necessary with this PR anymore.
(pyenv-which-ext fails to remove the "system" shims dir (which should be empty, but if not (due to a bug) causes an infinite loop)).

@mislav
Copy link
Contributor

mislav commented Aug 22, 2018

Interesting approach! I can definitely see value in fixing which foo depending on Python version, but I think the extra complexity necessary to pull this off might not be worth porting to rbenv at this time. If rbenv ended up depending on a chpwd shell handler, I would rather recommend using chruby instead.

@1oglop1
Copy link

1oglop1 commented Sep 26, 2018

Hi guys any progress on this one?

@blueyed
Copy link
Contributor Author

blueyed commented Sep 26, 2018

@1oglop1
I am still using it myself, and invite you to try it - just check out my PR use it from there.
There are issues with other plugins at least, so if you use pyenv-virtualenv also (which is not really necessary), you should use my PR from there, too.
Also make sure to install a chpwd handler (and example for zsh/zsh-autoenv is above).
Later we would/should ship a chpwd handler probably, but I also agree with @mislav somehow that there are light solutions after all.

I am using pyenv myself mainly only for installing different Python versions, and use normal virtualenvs per project (created using python -m venv .venv and then have (another) chpwd handler to activate those automatically).

@1oglop1
Copy link

1oglop1 commented Sep 28, 2018

@blueyed Thanks I will try later, I was happy having it installed via brew, but I will.

btw what's chpwd? I've never heard of it and I'm not using zsh since it's not "standard" shell installed on servers.

But if I understand it correctly it does the same thing as direnv

@blueyed
Copy link
Contributor Author

blueyed commented Sep 28, 2018

You can install it in parallel using ~/.pyenv (https://github.com/pyenv/pyenv#basic-github-checkout), but make sure to only use it from there then. After checking it out there, add my remote and check out this branch there.
chpwd is a hook Zsh and Bash provide to execute a function if you change a directory, and direnv provide this via direnv hook bash for Bash.
You can use direnv (if you are using it already), but would need to change parts of _my_autoenv_pyenv_chpwd, since it uses Zsh globbing (the "Optimized version of pyenv-version-name" part).
Therefore it is not really trivial.

@blueyed
Copy link
Contributor Author

blueyed commented Sep 28, 2018

You can use direnv

Not really, sorry.

But you can use the method it uses to create the chpwd hook:

_pyenv_hook() {
  local previous_exit_status=$?;
  …
  return $previous_exit_status;
};
if ! [[ "$PROMPT_COMMAND" =~ _pyenv_hook ]]; then
  PROMPT_COMMAND="_pyenv_hook;$PROMPT_COMMAND";
fi

@1oglop1
Copy link

1oglop1 commented Oct 21, 2018

@blueyed I finally got some time, to play around your branch.
It took me some time since git installation does not mention that you need to separately install python-build plugin to get pyenv install working.
Now I can do pyenv shell <version> but my default path is still pointing to
~/.pyenv/shims instead of ~/.pyenv/shims/<version> + .python-version doesn't do anything. Is this connected to chpwd you mentioned or something else?
anyway spearating shims based on python version sounds like a very good idea.

I also noticed that number of PRs and Issues is increasing are there enough reviewers @joshfriend because there are opened PRs since 2014

@1oglop1
Copy link

1oglop1 commented Jan 26, 2019

EDIT:
I finally spent some quality time exploring the code and now I finally understand why

~/.pyenv/shims/<version> + .python-version doesn't do anything.

It was because everything executed was first looking for executable in ~/.pyenv/shims and when that was found it ran all version checks etc.

Now I understand jargon word chpwd hook - which is simply a function which runs when directory is changes.

Which means that this branch misses 1 thing.

pyenv/libexec/pyenv-init L89 + L93 - should go away (not sure about the rest, but maybe all can go)

And pyenv/libexec/pyenv-global should perform similar/same operation as updated_path_for_shims does

EDIT

I noticed that "$PYENV_ROOT/shims" plays important role in updated_path_for_shims. I'm not so familiar how entire pyenv operates the with the PATH

I would say that desired behaviour should look as follows:

  1. pyenv installed, PATH is empty - and set to system python
  2. pyenv global <version> adds $PYENV_ROOT/shims/<version> to the PATH
  3. pyenv shell <version> adds all it needs to the beginning of PATH
  4. pyenv shell --unset restores previous path

yyuu and others added 23 commits January 10, 2021 12:52
Add PyPy 3.7-7.3.2 (source distribution)
Add PyPy 3.7-7.3.2 (binary distribution)
OS X arm64 will be installed with Python 3.9 only. Other versions bundled with python 3.8. Miniforge does not have as wide a choice of python versions as miniconda.
The Python version is specific only to the base environment.
corrected fish shell command
more general installation readme instructions
Ref: pyenv#1112

Add PYENV_SHIM, and use it with remove_from_path

This is required to avoid infinite recursion with "system" version.

Could also use PYENV_SHIM_PATH, but having PYENV_SHIM is more
flexible/useful.

Uses the following in ~/.autoenv.zsh:

```zsh
  # Experimental: maintain separate shims dir for pyenv.
  local version
  if [[ -z "$PYENV_VERSION" ]]; then
    version=$(pyenv version-name)
  elif [[ -n "$_CUSTOM_PYENV_VERSION" ]]; then
    if [[ "$_CUSTOM_PYENV_VERSION" == "$PYENV_VERSION" ]]; then
      version=$(pyenv version-name)
      echo "Updating custom pyenv version ($_CUSTOM_PYENV_VERSION => $version)"
    else
      echo "PYENV_VERSION was changed manually, unsetting _CUSTOM_PYENV_VERSION"
      unset _CUSTOM_PYENV_VERSION
    fi
  fi
  # echo version=$version PYENV_VERSION=$PYENV_VERSION
  if [[ -n "$version" ]] && [[ "$version" != "$PYENV_VERSION" ]]; then
    echo "pyenv version changed ($PYENV_VERSION => $version)"
    shims_dir=$PYENV_ROOT/shims

    # Create shims for this version, once.
    if ! [[ -d $shims_dir/$version ]]; then
      echo "pyenv-rehash via $0 for $version"
      PYENV_SHIM_PATH=$shims_dir/$version pyenv rehash
    fi
    if [[ -z "$_CUSTOM_PYENV_SHIMDIR" ]]; then
      _CUSTOM_PYENV_SHIMDIR=$(mktemp -d --tmpdir pyenv_shims.XXX)
      PATH=${PATH//$shims_dir/$_CUSTOM_PYENV_SHIMDIR}
      rmdir $_CUSTOM_PYENV_SHIMDIR
      ln -s $shims_dir/$version $_CUSTOM_PYENV_SHIMDIR
    fi
    PYENV_VERSION=$version
    _CUSTOM_PYENV_VERSION=$version
    # mv ~/.pyenv/shims ~/.pyenv/shims.out
    # ln -s $_CUSTOM_PYENV_SHIMDIR ~/.pyenv/shims \
    #   && pyenv rehash \
    #   && rm ~/.pyenv/shims \
    #   && mv ~/.pyenv/shims.out ~/.pyenv/shims
  fi
```
Only removing the current one could result in the system version not
being found, e.g. when using the following:

    out=$(PATH=$PYENV_ROOT/libexec:$PATH pyenv-sh-shell $version | grep '^PATH')
":" could work on Unix, but likely not on Windows?
Would need to be handled/escaped in updated_path_for_shims then -
currently it would loop endlessly.
It gets not done during init now anymore.

Initially I was about to only remove the "2>/dev/null" redirection, to
see the intermediate message
'pyenv-rehash: should not be used with "system" version', but it should not
be necessary at all anymore.
@native-api
Copy link
Member

native-api commented May 18, 2021

This PR is now so polluted with unrelated commits that I cannot see what it actually does.

In particular, I don't see the coveted "chdir hook" that must be the key to its usability.

@native-api native-api requested review from native-api and removed request for native-api May 18, 2021 10:48
@blueyed
Copy link
Contributor Author

blueyed commented May 18, 2021

@native-api
Yeah, not sure what's up there - could rebase it again (the first commit is "WIP: use separate shim paths per shell/version", 4e7d582).

In particular, I don't see the coveted "chdir hook" that must be the key to its usability.

There's an example for one in Zsh in the description itself.

@native-api native-api closed this Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.