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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@ Uninstall:
.. code:: bash

rm -fr ~/.pyenv

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

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

.. code:: bash

Expand Down
61 changes: 61 additions & 0 deletions bin/pyenv-installer
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@ if [ -z "$PYENV_ROOT" ]; then
export PYENV_ROOT="${HOME}/.pyenv"
fi

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
Comment on lines +10 to +17
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#137 (comment)

Copy link
Contributor Author

@ianchen-tw ianchen-tw Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@native-api native-api Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


USER_PATH_ADDED=false

colorize() {
if [ -t 1 ]; then printf "\e[%sm%s\e[m" "$1" "$2"
else echo -n "$2"
Expand Down Expand Up @@ -50,7 +61,57 @@ checkout "${GITHUB}/pyenv/pyenv-update.git" "${PYENV_ROOT}/plugins/pyenv-upd
checkout "${GITHUB}/pyenv/pyenv-virtualenv.git" "${PYENV_ROOT}/plugins/pyenv-virtualenv" "master"
checkout "${GITHUB}/pyenv/pyenv-which-ext.git" "${PYENV_ROOT}/plugins/pyenv-which-ext" "master"

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
}

Comment on lines +64 to +78
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@native-api native-api Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

add_userpath() {
# Will detect rc files by setting variables below:
# PYENV_SHELL_DETECT
# PYENV_PROFILE_DETECT
# PYENV_RC_DETECT
eval "$(${PYENV_ROOT}/bin/pyenv init --detect-shell $shell)"

case "$PYENV_SHELL_DETECT" in
bash )
write_source $PYENV_PROFILE_DETECT
write_source $PYENV_RC_DETECT
;;
zsh )
write_source $PYENV_PROFILE_DETECT
write_source $PYENV_RC_DETECT
;;
ksh )
write_source $PYENV_PROFILE_DETECT
;;
* )
{
echo "Add userpath for $PYENV_SHELL_DETECT is not currently supported"
echo "Please set up manually"
} >&2
;;
esac
}

if ! command -v pyenv 1>/dev/null; then
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lost the context of this comment.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 criterion is based on assumptions of how pyenv-installer might be used:

  1. Most users would run pyenv-installer on a newly configured system (without pyenv installed).
  2. 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.


add_userpath
if ${USER_PATH_ADDED}; then
echo "Restart your shell to start using pyenv" >&2
exit 0
fi

{ echo
colorize 1 "WARNING"
echo ": seems you still have not added 'pyenv' to the load path."
Expand Down
59 changes: 59 additions & 0 deletions bin/pyenv-offline-installer
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@ if [ -z "$PYENV_ROOT" ]; then
PYENV_ROOT="${HOME}/.pyenv"
fi

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

USER_PATH_ADDED=false

colorize() {
if [ -t 1 ]; then printf "\e[%sm%s\e[m" "$1" "$2"
else echo -n "$2"
Expand Down Expand Up @@ -60,8 +71,56 @@ conditional_mv "$TMP_DIR/pyenv-which-ext" "${PYENV_ROOT}/plugins/pyenv-which-ex

rm -rf $TMP_DIR

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
}

add_userpath() {
# Will detect rc files by setting variables below:
# PYENV_SHELL_DETECT
# PYENV_PROFILE_DETECT
# PYENV_RC_DETECT
eval "$(${PYENV_ROOT}/bin/pyenv init --detect-shell $shell)"

case "$PYENV_SHELL_DETECT" in
bash )
write_source $PYENV_PROFILE_DETECT
write_source $PYENV_RC_DETECT
;;
zsh )
write_source $PYENV_PROFILE_DETECT
write_source $PYENV_RC_DETECT
;;
ksh )
write_source $PYENV_PROFILE_DETECT
;;
* )
{
echo "Add userpath for $PYENV_SHELL_DETECT is not currently supported"
echo "Please set up manually"
} >&2
;;
esac
}

if ! command -v pyenv 1>/dev/null; then
add_userpath
if ${USER_PATH_ADDED}; then
echo "Restart your shell to start using pyenv" >&2
exit 0
fi

{ echo
colorize 1 "WARNING"
echo ": seems you still have not added 'pyenv' to the load path."
Expand Down