-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
pyenv init -
performance improvements; recommend using pyenv init - <shell>
#3136
pyenv init -
performance improvements; recommend using pyenv init - <shell>
#3136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! An almost twofold speedup just from specifying the shell is definitely a noteworthy discovery!
I guess the rest comes from case
and echo
?
It might be worth specifying in the README, in the advanced shell setup section, that pyenv init -
does work without an argument but specifying the shell explicitly results in an almost twofold speedup.
2fa6a52
to
462c7c9
Compare
Reverted to using: function print_completion() {
completion="${0%/*/*}/completions/pyenv.${shell}"
if [ -r "$completion" ]; then
echo "source '$completion'"
fi
} Inside The major obstacle in making any further progress is the fact that homebrew has a directory setup which is quite bad from my viewpoint. It has core functionality like libexec and completions in this path:
While shims and python versions are stored in PYENV_ROOT, which is set to PYENV_ROOT=/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23 Instead of having it set to Additonally, if you install pyenv-virtualenv via homebrew, it is installed in a directory like:
instead of
It's very confusing, since even in the README, we have this note:
Which tells us to drop plugins in PYENV_ROOT/plugins, but if you do this with homebrew, you will end up with:
And your plugins are now all over the place. Of course, it works, but is very confusing to a developer which comes from the setup you get when installing via pyenv run My conclusion is that it would be very nice to have the |
handling for case where unknown option is provided. - Error handling involves writing error message to stderr, and forcefully exiting the init script, forcing user to fix their setup.
- Local testing shows that you get a trivial 1.6x speedup in command execution time (over doing `pyenv init -`) by specifying which shell you are using.
to specify the shell they are using when adding: `eval $(pyenv init - <shell>)` to their config file.
pyenv-init. - It turns out that the code I wrote was very similar to what was already present in rbenv-init. As rbenv is referenced when I am writing a pull request, I am assuming that maintainers want code similar to what's already there. WARNING: The code will now not validate whether the specified shell is valid or not, and if you make a silly mistake like writing `--parh` instead of `--path`, the shell may be set to `--parh` and so forth. But I guess this is acceptable.
…urce' instead of 'pyenv init - | source'.
The old tests seems to have been copy-paste from some 11 year old rubyenv setup, and included a long and windy pathname. The old setup was extremely bad in my view, current test is much better. Exported variables are local to the specific test, so they will not affect other test instances. ---> Enables us to remove unnecessary `root` variable in pyenv-init.
echo is a bash built-in, and thus you don't need to launch a new process for something as simple as writing to stdout. Saves about 2-3ms.
print_shell_function(). -> Maintainers prefer single quotes instead of double quotes, since it allows us to look away from tracking what to escape.
Tell users that `pyenv init -` works, but that specifying the shell which is used is preferable, as it reduces launch time.
Homebrew installations have PYENV_ROOT set to `~/.pyenv`, while the actual completion file resides in /home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/completions/pyenv.zsh Thus, if you try to use `$PYENV_ROOT/completions/pyenv.${shell}`, the completions file will not be found, since you are pointing to: `~/.pyenv/completions/pyenv.zsh` when the actual location is: `/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/completions/pyenv.zsh` This problem is resolved by using the parent folder of the currently executing file: `/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/libexec/pyenv-init` which is `/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23` And then go to completions from there. I don't like the fact that homebrew installations store some things in `/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/libexec/pyenv-init` while stuff like `shims` and python versions are stored in `~/.pyenv`, but that's how it currently is.
In `test_helper.bash`: set PATH="${BATS_TEST_DIRNAME%/*}/libexec:$PATH" instead of PATH="${BATS_TEST_DIRNAME}../libexec:$PATH" it essentially enables us to expect `${exec_root}/completions/pyenv.bash` instead of something like `${exec_root}/test/../completions/pyenv.zsh
2dc7686
to
b1882cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only some nitpicks remaining. Good job!
…\` setup instead of `echo '` 1) If people are using bash with `set -u` (enable unset variable checking), then they will get an error if they run `pyenv` in the CLI under the current setup. To avoid any errors when no positional arguments, we use ${1:-} instead of $1. 2) We find that the script looks better when using: ```bash echo \ 'pyenv() { local command=${1:-}' ``` As opposed to: ``` echo 'pyenv() { local command=${1:-}' ```
pyenv-init
performance improvements
pyenv-init
performance improvementspyenv init -
performance improvements; recommend using pyenv init - <shell>
Make sure you have checked all steps below.
Prerequisite
Description
pyenv init -
, and replaced calls tocat
with calls toecho
.Instead of having:
We now get:
pyenv init
in their shell configuration file.Example:
Under the section: Set up shell environment for zsh:
Instead of
My computer uses 50ms on
pyenv init -
, and 30ms onpyenv init - zsh
. Simple speedup.pyenv-init
Removed unnecessary declarations of variables: (e. g.
no_rehash=""
), replaced a series of if-statements with acase
block (code essentially imported from rbenv), and changecompletions path
such that we get:(essentially
PYENV_ROOT/completions/pyenv.zsh
)instead of:
(equal to something like:
$(dirname "$(realpath pyenv-init)")/../completions/pyenv.zsh
)RESULTS:
The first command is run by
master
branch, second command is used with perf improvements.Not much to speak of, but will hopefully come with more improvements later. Interestingly, the speedup seems to be more significant when
zsh
is specified.Tests
3 tests have been modified, all inside
test/init.bats
.pyenv init - fish | source
PYENV_ROOT
is exported before runningpyenv init - <shell>
.Assumes that
BATS_TEST_DIRNAME/..
is equal toPYENV_ROOT
. Enables us to have a simpler path for sourcing of completions, as mentioned in point (3) above.