Update shell config file during installation (bash & zsh) #137

ianchen-tw · 2022-11-25T14:44:21Z

Fix: #112

For users using the installation script, it is natural that the environment variables will be automatically modified

Outline

Modify user's profile based on current shell & system(bash & zsh, currently)

Discussion:

Should we add an environment variable, so that users can choose not to modify their shell config profiles?

References

native-api · 2022-11-25T17:58:13Z

bin/pyenv-installer

+ echo "Appending the following lines to $1:"
+ echo ' # pyenv'
+ echo ' export PATH="'"$PYENV_ROOT/bin:"'$PATH"'
+ echo ' eval "$(pyenv init --path)"'


The suggested default is pyenv init -

While this is true, I've had the most success across multiple OS'es using the above, or the above in conjunction with pyenv init -. Maybe that's an issue for a separate thread, though? Are there any disadvantages to including pyenv init --path?

Yes. Big disadvantage. That's not the suggested setup. 🙂
The suggested setup is not random. It was worked out based on user input about their use cases, to provide full features, in the vast majority of cases and with minimal side effects.

E.g. it provides shell completion, and it doesn't add duplicate PATH entries in a nested shell.

Sounds good. I'll see if I can replicate some of my issues with this suggested setup.

To be clear, I'm talking about having more success in general with:

eval "$(pyenv init -)" eval "$(pyenv init --path)"

Over:

eval "$(pyenv init -)"

But maybe this comes down to a debate of where pyenv shims belong in the PATH and of course that will be user-dependent.

native-api

Thanks for the submission! As this is one of the most requested features, you're eligible for a bounty from donated money when this goes live, if you're interested.

native-api · 2022-11-25T17:59:28Z

bin/pyenv-installer

+ echo ' # pyenv'
+ echo ' export PATH="'"$PYENV_ROOT/bin:"'$PATH"'
+ echo ' eval "$(pyenv init --path)"'
+ echo ' eval "$(pyenv virtualenv-init -)"'
+ echo ''


It's probably sufficient to just say which files are being updated, like RVM does.

native-api · 2022-11-25T18:06:41Z

bin/pyenv-installer

+
+add_userpath() {
+ OS="$(uname -s)"
+ CURRENT_SHELL="$(basename "$SHELL")"


Since this is a Bash script, $SHELL would probably always be Bash.
pyenv-init has logic to detect the shell of the parent process.
That said, this logic seems to rather belong in pyenv-init to avoid duplication. E.g. it likewise determines profile files for diffent shells and commands to write -- we just need to actually write them rather than just display.

@native-api,

After inspecting pyenv-init, I think we should just copy the logic of determining user shell directly from it, because:

The script(pyenv-init) is not composed to be sourced by other one, so that we can't simply source it and variables(rc, shell, profile) in our script.

bin/py-installer should be a self-containing script, which could be runned even if pyenv-init changed, so a little duplication might be acceptable.

So here's what I'll try to do next:

Copying the logic directly of determining shell & rc files from pyenv-init and put a reference to the original code.

Keep the OS variable so that it can better fit mac users.

I'll start working on this if you think it's appropriate

The idea is to add yet another mode to pyenv-init -- e.g. --install -- that would write shell configuration. Just like currently, the installer runs it without arguments and it prints installation instructions.

Apart from avoiding code duplication, we won't need to adapt Pyenv-installer if we change something in the suggested shell configuration.

I copied the shell detection mechanism from pyenv-init in pyenv-installer.

This resolves the issue of pyenv-init being unable to detect the parent of the parent process when invoked through pyenv-installer, as described in my experiment(ianchen-tw#2).

native-api · 2022-11-25T18:09:57Z

bin/pyenv-installer

+ fi
+}
+
+if ! command -v pyenv 1>/dev/null; then


What's the logic behind this criterion?

Maybe rather check that shell config files don't have any mention of "pyenv"? Or run a new shell with -i and/or --login and check if anything related to Pyenv is set?

I lost the context of this comment.

Could you check if this still being an issue after reviewing the updated code?
Thanks!

AFAICS, this line is supposed to detect whether we need to add shell configuration stuff, or if it's already present.

I can't see how this test achieves that.

Since there's an infinite variety of setups, perhaps the best way of action is to see how other language managers do this -- to see what measures have proven to be sufficient in practice.

This criterion is based on assumptions of how pyenv-installer might be used:

Most users would run pyenv-installer on a newly configured system (without pyenv installed).

The installer will update shell configuration stuff correctly, therefore pyenv would be detected after restarting the shell.

This criterion is not water-proof, especially for users that rerun the installer many times without restarting their shell. But it's probably the simplest way AFAICS.

bin/pyenv-installer

native-api · 2022-11-25T18:17:52Z

bin/pyenv-installer

+ add_userpath
+
+ if ${SRC_WRITTEN}; then
+ echo "Close and reopen your terminal to start using pyenv" >&2


Restarting the shell is enough.

native-api · 2022-11-25T18:41:01Z

README.rst

- 
-then remove these three lines from ``.bashrc``:
+
+then remove these three lines from your shell config file(``.bashrc``, ``.zshrc``, ``.profile``...):


ianchen-tw · 2022-11-28T15:03:02Z

Thanks for the submission! As this is one of the most requested features, you're eligible for a bounty from donated money when this goes live, if you're interested.

I would love to, lets continue this conversation once the PR gets accepted

ianchen-tw · 2022-12-11T10:07:00Z

I'm still working on this PR.
Anyone interested in the actual progress can refer to
ianchen-tw#2

Fix pyenv#112

ianchen-tw · 2022-12-25T11:04:11Z

Hello @native-api,

Thank you for your help with writing the pull request for the Pyenv(pyenv/pyenv#2561). I have updated this PR and am hopeful that we can get it to work.

native-api · 2023-01-04T23:25:31Z

bin/pyenv-installer

+if [ -z "$shell" ]; then
+ shell="$(ps -p "$PPID" -o 'args=' 2>/dev/null || true)"
+ shell="${shell%% *}"
+ shell="${shell##-}"
+ shell="${shell:-$SHELL}"
+ shell="${shell##*/}"
+ shell="${shell%%-*}"
+fi


Why the heck did we export --detect-shell, to be used with eval to boot, if we STILL need this?

#137 (comment)

Considering this example:
Run curl -L https://github.com/pyenv/pyenv-installer/raw/master/bin/pyenv-installer | bash in zsh.
Just to make sure this scenario would work

That just means that --detect-shell needs to do better.

Thanks to eval, it can run code in the same process, evading the "shell of parent of parent" problem.
Just make it output the shell detection code that you've copypasted.

native-api · 2023-01-04T23:30:06Z

bin/pyenv-installer

+ fi
+}
+
+if ! command -v pyenv 1>/dev/null; then


AFAICS, this line is supposed to detect whether we need to add shell configuration stuff, or if it's already present.

I can't see how this test achieves that.

Since there's an infinite variety of setups, perhaps the best way of action is to see how other language managers do this -- to see what measures have proven to be sufficient in practice.

native-api · 2023-01-04T23:39:09Z

bin/pyenv-installer

+write_source() {
+ # expand ~ to user's home
+ local target="${1/#\~/$HOME}"
+
+ echo "Update file: $1" >&2
+
+ echo "" >>$target
+ echo '# pyenv' >>$target
+ echo 'export PATH="'"$PYENV_ROOT/bin:"'$PATH"' >>$target
+ echo 'eval "$(pyenv init -)"' >>$target
+ echo 'eval "$(pyenv virtualenv-init -)"' >>$target
+
+ USER_PATH_ADDED=true
+}
+


I still heavily dislike the duplication with pyenv-init.
It's going to be a constant headache and source of errors during maintenance.

I understand the desire to keep installation logic out of pyenv-init as it's of no use after installation. But however you look at it, the bulk of it is already in Pyenv-Init anyway!

The "is-Pyenv-shell-setup-already-done" part of the logic is something that could probably be kept out of Pyenv-Init -- if it proves to be different enough from existing Pyenv-Init logic.

We could add pyenv init --profile and pyenv init --rc to output just the stuff that needs to be written, then write it here. But since those files are detected inside --detect-shell, it seems simpler and even less code in pyenv-init to just add a single pyenv-init --install.

native-api · 2023-01-04T23:58:40Z

I can try to complete the PR myself, addressing the remaining concerns (will probably have time this weekend).

ianchen-tw · 2023-01-05T09:01:59Z

Just wanted to express my thanks to @native-api for continuing to work on my PR and for spending extra time on it.
I really appreciate your dedication to this project and it's great to see any progress that you're making.
Thank you for your efforts!

ianchen-tw · 2023-05-07T17:10:19Z

@native-api any update?

If no one is interested in this pull request anymore, I'll likely close it.

ianchen-tw changed the title ~~Update shell config file after installation (bash & zsh)~~ Update shell config file during installation (bash & zsh) Nov 25, 2022

native-api reviewed Nov 25, 2022

View reviewed changes

native-api requested changes Nov 25, 2022

View reviewed changes

ianchen-tw changed the title ~~Update shell config file during installation (bash & zsh)~~ [WIP] Update shell config file during installation (bash & zsh) Dec 1, 2022

ianchen-tw mentioned this pull request Dec 1, 2022

Export detected shell environment in pyenv-init pyenv/pyenv#2540

Merged

5 tasks

native-api marked this pull request as draft December 1, 2022 18:58

Update shell config file after installation (bash & zsh)

6cad90a

Fix pyenv#112

ianchen-tw force-pushed the add-userpath branch from 0c69fb5 to 6cad90a Compare December 25, 2022 10:16

ianchen-tw changed the title ~~[WIP] Update shell config file during installation (bash & zsh)~~ Update shell config file during installation (bash & zsh) Dec 25, 2022

ianchen-tw marked this pull request as ready for review December 25, 2022 10:56

ianchen-tw requested a review from native-api December 25, 2022 10:56

native-api reviewed Jan 4, 2023

View reviewed changes

ianchen-tw closed this May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update shell config file during installation (bash & zsh) #137

Update shell config file during installation (bash & zsh) #137

ianchen-tw commented Nov 25, 2022

native-api Nov 25, 2022

sabard Nov 28, 2022

native-api Nov 28, 2022 •

edited

Loading

sabard Nov 28, 2022

ianchen-tw Dec 25, 2022

native-api left a comment

native-api Nov 25, 2022

ianchen-tw Dec 25, 2022 •

edited

Loading

native-api Nov 25, 2022

ianchen-tw Nov 28, 2022

native-api Nov 28, 2022

ianchen-tw Dec 25, 2022

native-api Nov 25, 2022

ianchen-tw Dec 25, 2022

native-api Jan 4, 2023

ianchen-tw Jan 5, 2023

native-api Nov 25, 2022

ianchen-tw Dec 25, 2022 •

edited

Loading

native-api Nov 25, 2022

ianchen-tw Dec 25, 2022 •

edited

Loading

ianchen-tw commented Nov 28, 2022

ianchen-tw commented Dec 11, 2022

ianchen-tw commented Dec 25, 2022

native-api Jan 4, 2023

ianchen-tw Jan 5, 2023

ianchen-tw Jan 5, 2023 •

edited

Loading

native-api Jan 5, 2023 •

edited

Loading

native-api Jan 4, 2023

native-api Jan 4, 2023

native-api Jan 4, 2023

native-api Jan 4, 2023 •

edited

Loading

native-api commented Jan 4, 2023

ianchen-tw commented Jan 5, 2023

ianchen-tw commented May 7, 2023 •

edited

Loading


		then remove these three lines from ``.bashrc``:

		then remove these three lines from your shell config file(``.bashrc``, ``.zshrc``, ``.profile``...):

Update shell config file during installation (bash & zsh) #137

Update shell config file during installation (bash & zsh) #137

Conversation

ianchen-tw commented Nov 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

native-api Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

native-api left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianchen-tw Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianchen-tw Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianchen-tw Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

ianchen-tw commented Nov 28, 2022

ianchen-tw commented Dec 11, 2022

ianchen-tw commented Dec 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianchen-tw Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

native-api Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

native-api Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

native-api commented Jan 4, 2023

ianchen-tw commented Jan 5, 2023

ianchen-tw commented May 7, 2023 • edited Loading

native-api Nov 28, 2022 •

edited

Loading

ianchen-tw Dec 25, 2022 •

edited

Loading

ianchen-tw Dec 25, 2022 •

edited

Loading

ianchen-tw Dec 25, 2022 •

edited

Loading

ianchen-tw Jan 5, 2023 •

edited

Loading

native-api Jan 5, 2023 •

edited

Loading

native-api Jan 4, 2023 •

edited

Loading

ianchen-tw commented May 7, 2023 •

edited

Loading