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

Update install instructions for Bash and Zsh #1920

Merged
merged 8 commits into from
May 13, 2021

Conversation

native-api
Copy link
Member

@native-api native-api commented May 11, 2021

pushd "$(pyenv root)"
git remote add pr https://github.com/native-api/pyenv
git fetch pr
git status    # see what branch/tag/commit you're currently at
git checkout bash_zsh_instructions

pyenv init

#undo the changes
git checkout <old branch/tag/commit from `git status` above>
git branch -D bash_zsh_instructions
git remote remove pr
popd

Make sure you have checked all steps below.

Prerequisite

  • Please consider implementing the feature as a hook script or plugin as a first step.
    • pyenv has some powerful support for plugins and hook scripts. Please refer to Authoring plugins for details and try to implement it as a plugin if possible.
  • Please consider contributing the patch upstream to rbenv, since we have borrowed most of the code from that project.
    • We occasionally import the changes from rbenv. In general, you can expect changes made in rbenv will be imported to pyenv too, eventually.
    • Generally speaking, we prefer not to make changes in the core in order to keep compatibility with rbenv.
  • My PR addresses the following pyenv issue (if any)

Description

  • Here are some details about my PR

Updated README and instructions for Bash and Zsh based on user feedback.
Going to ask for reviews before merging.

Tests

  • [N/A] My PR adds the following unit tests (if any)

@agoose77
Copy link

agoose77 commented May 11, 2021

@native-api thanks for creating a PR to try and tackle this.

The Issue 🐛

I feel that with all of this work on documenting exactly what to do for each shell, we're trying to solve the wrong problem; shell RC files are confusing, and not always consistent between shells and environments.

All of the issues that have been raised about this have stemmed from the fact that pyenv has several locations that need to end up in PATH, and users either don't know of all of them, or don't understand why they need to go in different RC files. This is not a criticism of these users; I have to reference what each shell does when I reply to these issues.

New Init API 🔩

I think that we should consider dropping the shell-specific approach entirely, and instead pyenv should just expose two commands:

pyenv init --path

and

pyenv init --shell

These are the two kinds of configuration that pyenv needs to set AFAICT. The pyenv init --shell command is an attempt to remove the visual precedence that pyenv init - has over pyenv init --path, and to make it clear that one is for shell functions, the other is for environment.

My rationale for dropping the per-shell documentation in pyenv itself is that it quickly explodes with the number of shells, OS default configurations (e.g. .profile sources .bashrc on Ubuntu), and login shells (e.g. I'm using GDM which explicitly sources ~/.profile despite SHELL=/usr/bin/zsh). This gets more confusing if users have non-standard configuration files already in use.

I think this would be easier to deal with through a wiki entry.

Old Init API 🕸️

Of course, this means that things will change for users. I don't know what the policy for API change is for this project, but if we don't want to break pyenv init - outright, then the safest approach is just to add a new --shell flag and retain the existing pyenv init - behaviour (but mark as deprecated). However, I would perhaps go further, and just make pyenv init - return cat <(echo "warning ...") <(pyenv init --path) <(pyenv init --shell). This way, users who haven't modified their RC files will be notified of the upcoming deprecation, but will not be confused by breakages.

Shell-Specific Help ℹ️

I still think that the shell-specific information is useful, but perhaps it doesn't belong in the tool. If we repurpose pyenv init - as an old-style (deprecated) configuration command, then we can add some information about what the new-style looks like, e.g.

cat <<- 'EOF' 
Warning, `pyenv init -` will no longer work in future releases of pyenv.
To avoid breakages when this occurs, please move to the new configuration mechanism.

The old `pyenv init -` command has been split into two new commands:
`pyenv init --path` and `pyenv init --shell`. 

You should remove the existing `pyenv init -` code, and instead 
1. Add `pyenv init --path` to your `~/.profile` (or equivalent) file
2. Add `pyenv init --shell` to your `~/.bashrc` (or equivalent) file

In some cases you might find that this does not work. This is likely because shell configuration is hard! 
Please visit .... for more information with shell-specific guidance.
EOF

source pyenv init --path
source pyenv init --shell 

What do you think?

@The-Judge
Copy link

No offense, but reading all the replies in #1915 to my initial request, it looks a bit as the thread got "hijacked" for "all-things-shell-issues" ;-)
Initially, I reported an issue for "bash" shell. Here's what I did:

  1. I cloned Update install instructions for Bash and Zsh #1920 to /usr/src/pyenv and cd'ed into it
  2. src/configure && make -C src (successfully)
  3. export PYENV_ROOT="/usr/src/pyenv"
  4. export PATH="${PYENV_ROOT}/bin:${PATH}"
  5. which pyenv (provides /usr/src/pyenv/bin/pyenv)
  6. Both, pyenv init and pyenv init bash provide:
# Load pyenv automatically by appending
# the following to ~/.bash_profile:

eval "$(pyenv init -)"

Nothing more; nothing about setting the PATH or anything; which seems wrong already, isn't it?

Anyways: I removed everything pyenv related from my .bashrc and .profile already before running this. I started with 1:1 doing what pyenv init suggests, knowing this will fail without the PATH manipulation and - to very few surprises - it did:

-bash: pyenv: command not found

So, I also added this before that line in .bash_profile, making it look like this:

root@judgepi:~# cat .bash_profile
#source ${HOME}/.bash_pyenv
export PYENV_ROOT="/usr/src/pyenv"
export PATH="$PYENV_ROOT/bin:$PATH"
eval "$(pyenv init -)"

root@judgepi:~#

Actually, it looks like this did the trick! I can use pyenv and do no longer receive the WARNING when SSH'ing into it:

~ $ ssh pi /bin/true
~ $ echo $?
0
~ $

@The-Judge
Copy link

PS: I like @agoose77 's idea! Debugging the global RC-landscape is out of scope for pyenv; at least "core". Supporting all shells and advice on them seems to be more related to sidekick-projects like pyenv/pyenv-installer.
As long as it is clear and transparent to the user what's the goal he/she has to take care about, I feel like pyenv's duty is done.

@native-api
Copy link
Member Author

1. I cloned #1920 to `/usr/src/pyenv` and cd'ed into it

This is wrong. You need to switch your installed Pyenv to it. This is why I gave the exact commands needed right at the top.

@The-Judge
Copy link

1. I cloned #1920 to `/usr/src/pyenv` and cd'ed into it

This is wrong. You need to switch your installed Pyenv to it. This is why I gave the exact commands needed right at the top.

I did by modifying the PATH to that new location - didn't I?

@native-api
Copy link
Member Author

I did by modifying the PATH to that new location - didn't I?

Oh, sorry, I missed that part (I honestly thought that no-one would go for the trouble :-) ). Then it indeed should work.

How exactly did you "clone #1920"? I suspect that you didn't check out the right branch and ended up at master from my fork -- which is stale and unused.

README.md Outdated Show resolved Hide resolved
@aiguofer
Copy link

I'm wondering if pyenv init --path is even needed at all? the installation is already asking users to add export PATH="$PYENV_ROOT/bin:$PATH, why not just ask them to add export PATH="$PYENV_ROOT/shims:$PYENV_ROOT/bin:$PATH" (or the equivalent in each shell) instead?

I feel like the extra command to set the PATH is what's tripping people up as they think they need to replace the old command with that one.

@native-api
Copy link
Member Author

native-api commented May 11, 2021

I'm wondering if pyenv init --path is even needed at all? the installation is already asking users to add export PATH="$PYENV_ROOT/bin:$PATH, why not just ask them to add export PATH="$PYENV_ROOT/shims:$PYENV_ROOT/bin:$PATH" (or the equivalent in each shell) instead?

The idea is to make the configuration more future-proof. In the future, we may add/change steps in --path if something else needs to be done in the login shell.

I feel like the extra command to set the PATH is what's tripping people up as they think they need to replace the old command with that one.

I think that it's rather the fact that I failed to include the PYENV_ROOT and PATH= steps into the pyenv init instructions at all! (They were only included into the pyenv-installer postinstall message.) So the users who saw the warning didn't readily see that they need to move these lines to ~/.profile!
That was a blunder on my part.

README.md Outdated Show resolved Hide resolved
* Synchronize README.md with `pyenv init`
* Add a Bash installation option (only to README) that would still allow
  automated install if ~/.profile sources ~/.bashrc
* Add a ~/.bash_profile note
* Concatenate shims activation into installation for brevity
  (Pyenv can't be used meaningfully without shims anyway)
  Otherwise, we'd need to duplicate all the ~/.profile shenanigans in both sections
@julianfortune
Copy link

julianfortune commented May 12, 2021

source pyenv init --path
source pyenv init --shell 

What do you think?

@agoose77 This solution would work great for all of the use cases in my world! Just my 2 cents.

Add guidance blocks
Update Advanced
Update OSX instructions
Exclude Bash automated install
Formatting and prrofreading
@native-api
Copy link
Member Author

native-api commented May 12, 2021

@agoose77 @julianfortune @The-Judge

I feel that with all of this work on documenting exactly what to do for each shell, we're trying to solve the wrong problem; shell RC files are confusing, and not always consistent between shells and environments.
<...>
My rationale for dropping the per-shell documentation in pyenv itself is that it quickly explodes with the number of shells, OS default configurations (e.g. .profile sources .bashrc on Ubuntu), and login shells (e.g. I'm using GDM which explicitly sources ~/.profile despite SHELL=/usr/bin/zsh). This gets more confusing if users have non-standard configuration files already in use.

  • Not including ready-to-use invocations would erect a high barrier to entry. Even if they can (which is also not the case quite often), users don't want to have to think and waste time on it, they assume that if this is a production-ready software product, someone has already done the thinking for them!
    And rightfully so: if they have a common configuration, there must be someone who already thought of a good, turn-key solution for it, so there's absolutely no reason to reinvent the (square) wheel!
  • These "documenting exactly what to do for each shell" have been in the README for years and have been working fine so far.

I think we can get the best of both worlds here if we include both an explanation of what the effect should be, and the ready-to-use examples for common setups.
This is what I just did in the update to this PR -- go see if it's to your satisfaction! 😉

I think that we should consider dropping the shell-specific approach entirely, and instead pyenv should just expose two commands:

<..>
pyenv init --path
<...>
pyenv init --shell

This is exactly what the interface is now (just with keeping the the old name for "--shell", see below).

These are the two kinds of configuration that pyenv needs to set AFAICT. The pyenv init --shell command is an attempt to remove the visual precedence that pyenv init - has over pyenv init --path, and to make it clear that one is for shell functions, the other is for environment.

I thought about renaming - to --rc -- both when writing #1898 and now -- and ultimately decided against it.

It's true that it'll make things marginally clearer. But the drawbacks are:

  • Should we force the users to rewrite their configurations yet again, just after we've already frustrated them with this change and the broken and convoluted instructions?
    • That'll be very bad PR for the project. If we ever do that, we should rather first let the userbase calm down and forget the incident.
  • Users don't deal with pyenv init at all outside of the initial configuration step. So as long as it's clear what to put where, they don't really care how these incantations are spelled.
    • So all the frustration and the extra work that we'll be forcing onto all our users would be for nothing, really.
    • See also below for another reason why I had to retain -. Concluding from it, the moment to rename it was during Split startup logic into PATH and everything else #1898, and now the opportunity is gone.

So my best idea for this -- if this is even worth doing, as per above -- is to introduce a new name as an alias to -, and wait for a long, long time before deprecating the original.

Of course, this means that things will change for users. I don't know what the policy for API change is for this project, but if we don't want to break pyenv init - outright <...>

The change was made to fix #1649. We needed to make a breaking change to force the users to change their configurations -- otherwise, they would keep falling victim to #1649 and would keep reporting it. And we needed to retain - so that we could use the existing shell configuration to prominently notify them about what they need to do.

Supporting all shells and advice on them seems to be more related to sidekick-projects like pyenv/pyenv-installer.

Actually, pyenv init (without arguments) is intended for Pyenv-installer. Since the instructions are specific to Pyenv's version rather than Pyenv-Installer's, it's natuiral to keep them in Pyenv's codebase to avoid duplication.

@agoose77
Copy link

@native-api thanks for the clarification. I agree on all points; I'm not pushing for any breaking changes any time soon.

I like the --path --shell flags because they make it explicit that this is a two-phase initialisation. I agree that this shouldn't break anything; it should just be an alias with a long-term deprecation policy. Given that we've now made public changes to the pyenv initialisation APIs, I also concur that pyenv init - needs to remain stable.

@aiguofer
Copy link

The updated instructions seem pretty complete to me. The only issue I can see is for anyone using a shell framework like prezto or oh-my-zsh. They generally have some plugin for pyenv which may do some, or all, of the initialization. For example, prezto runs eval "$(pyenv init - --no-rehash zsh)", while oh-my-zsh attempts to do it all.

I imagine you don't need to go into detail about this, but maybe just a mention if you're using one of those frameworks you might only need the PATH changes (seems like that's all that would be needed for both of those frameworks).

@shireenrao
Copy link

Thank you @aiguofer - even after following the changes mentioned, I was seeing issues. After seeing your comment on oh-my-zsh plugins I was able to resolve this by disabling the plugin.

@native-api
Copy link
Member Author

native-api commented May 13, 2021

For example, prezto runs eval "$(pyenv init - --no-rehash zsh)", while oh-my-zsh attempts to do it all.

Those projects will need to react to Pyenv changes.

@native-api
Copy link
Member Author

I imagine you don't need to go into detail about this, but maybe just a mention if you're using one of those frameworks you might only need the PATH changes (seems like that's all that would be needed for both of those frameworks).

I feel that it's rather the responsibility of those projects to inform their users which configuration steps they're saving them from.

@aiguofer
Copy link

I imagine you don't need to go into detail about this, but maybe just a mention if you're using one of those frameworks you might only need the PATH changes (seems like that's all that would be needed for both of those frameworks).

I feel that it's rather the responsibility of those projects to inform their users which configuration steps they're saving them from.

Yeah that makes sense... Maybe just a mention to check how their framework handles pyenv then. I feel like a lot of users might unfortunately not be aware that their framework might already be doing something and thus think it's an issue with pyenv itself.

@native-api native-api changed the title [WIP] Update install instructions for Bash and Zsh Update install instructions for Bash and Zsh May 13, 2021
@native-api native-api merged commit 0d07cda into pyenv:master May 13, 2021
@native-api native-api deleted the bash_zsh_instructions branch May 13, 2021 19:58
Timothy-W-Hilton pushed a commit to Timothy-W-Hilton/dotfiles that referenced this pull request May 28, 2021
machupicchubeta added a commit to machupicchubeta/dotfiles that referenced this pull request May 30, 2021
WARNING was:
```
WARNING: `pyenv init -` no longer sets PATH.
Run `pyenv init` to see the necessary changes to make to your configuration.
```

Refs.
- pyenv/pyenv#1915
- pyenv/pyenv#1906
- pyenv/pyenv#1649
- pyenv/pyenv#1920 (comment)
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.

WARNING: pyenv init - no longer sets PATH. path manipulations need to be placed in ~/.zprofile for zsh user
6 participants