Add env var to set DEFAULT_PYTHON (add Pyenv support) #448

wren · 2020-07-13T00:45:17Z

Fixes #113.

This keeps sys.executable as the default, but will use the PIPX_DEFAULT_PYTHON environment variable, if defined. This mean no change in functionality for any users until they define the variable.

I'm suggesting the var PIPX_DEFAULT_PYTHON (no PYENV even though the original issue asked for pyenv support), because this approach will work with any python manager, and I think it might be confusing to enshrine one manager in the var name over others.

Also, it didn't look like the constants are tested, so I didn't update any tests. But let me know if I missed something and I'll be happy to add.

cs01 · 2020-07-13T02:17:43Z

I am not a pyenv user (maybe one of the other pipx maintainers is?), so I don't really know how to test this or show that it fixes anything.

Could you run some commands that show this fixes a problem a pyenv user would run into, and copy+paste them and their output here so I can see what that looks like?

Assuming this change is useful and we want to merge, the help text will also need to document this environment variable (see main.py).

wren · 2020-07-13T04:24:05Z

Generally (for any user): the new env var should let you override the default python used by pipx to install packages. So, if you export PIPX_DEFAULT_PYTHON=python3.6, then pipx will use python3.6 to install by default (but is still override-able by --python).

Specifically (for Pyenv users): this works for with export PIPX_DEFAULT_PYTHON="$PYENV_ROOT/shims/python" since Pyenv uses that shim to always point to the correct Python (note: I think PYENV_ROOT defaults to ~/.pyenv).

So, this is testable without Pyenv simply by changing the env var and verifying that Pipx is using the correct Python to install packages.

Pyenv Tests

I tested this by running the latest pipx release side-by-side with this the source from this PR. The main thing to note is that the pipx release installed with python 3.8 both times, but the pipx source installed using the pyenv python (3.8 the first time, then 3.6 the second).

https://asciinema.org/a/347120

$ pyenv global 3.8.3

$ python --version
Python 3.8.3

$ env | grep -i pipx_default_python
PIPX_DEFAULT_PYTHON=/Users/jonathan/.cache/pyenv/shims/python

$ python src/pipx install jrnl
  installed package jrnl 2.4.3, Python 3.8.3
  These apps are now globally available
    - jrnl
done! ✨ 🌟 ✨

$ python src/pipx uninstall jrnl
uninstalled jrnl! ✨ 🌟 ✨

$ pipx install jrnl
  installed package jrnl 2.4.3, Python 3.8.3
  These apps are now globally available
    - jrnl
done! ✨ 🌟 ✨

$ pipx uninstall jrnl
uninstalled jrnl! ✨ 🌟 ✨

$ pyenv global 3.6.10

$ python --version
Python 3.6.10

$ python src/pipx install jrnl
  installed package jrnl 2.4.3, Python 3.6.10
  These apps are now globally available
    - jrnl
done! ✨ 🌟 ✨

$ python src/pipx uninstall jrnl
uninstalled jrnl! ✨ 🌟 ✨

$ pipx install jrnl
  installed package jrnl 2.4.3, Python 3.8.3
  These apps are now globally available
    - jrnl
done! ✨ 🌟 ✨

$ pipx uninstall jrnl
uninstalled jrnl! ✨ 🌟 ✨

uranusjr · 2020-07-13T06:06:18Z

I needed to do something similar in #244 (comment). This would also be a good workaround for #395 as well. In general, sys.executable is not the best value in many repackaging situations, and this would be a welcoming change. And if it happens to also help pyenv users, more wonderful 😄

cs01 · 2020-07-14T18:12:23Z

Looks good to me, thanks for listing out that test case. That was helpful.

I think we should add a unit test before landing. Since the tests on CI generally only have one python version installed, we can set PIPX_DEFAULT_PYTHON to be a path that doesn't exist, then test that pipx raises an exception saying the Python interpreter doesn't exist.

wren · 2020-07-15T06:54:38Z

I'm not super familiar with this project, so I'm not sure if I did what you asked, but I think this is ready now.

uranusjr · 2020-07-25T09:13:22Z

It just occured to me, would it be worthwhile to incorporate shutil.which() into the resolution so a user can specify (say) python3 instead of /usr/bin/python3? It’s probably a good idea to add some error reporting around this code as well, since the exception may be quite cryptic if the user sets DEFAULT_PYTHON to an invalid value.

cs01 · 2020-07-26T18:26:24Z

tests/test_constants.py

+
+
+def test_default_python(monkeypatch):
+    monkeypatch.setattr(constants, "DEFAULT_PYTHON", "bad_python")


Should this be PIPX_DEFAULT_PYTHON?

I'm not sure, but I don't think so. The way constants are set up, environment variables are read before the tests are run, so patching the environment variable itself (PIPX_DEFAULT_PYTHON) wouldn't do anything here. So, we need to patch the python variable that holds the environment variable instead.

And I'm fairly new to your code base, so I might be wrong, but it seems to me this is the same way the other constants are handled in the test suite (like patching LOCAL_BIN_DIR instead of PIPX_BIN_DIR here).

I think we could do it with importlib.reload(pipx.constants) but I'm not sure if there would be side-effects.

cs01 · 2020-07-26T18:26:56Z

It just occured to me, would it be worthwhile to incorporate shutil.which() into the resolution so a user can specify (say) python3 instead of /usr/bin/python3? It’s probably a good idea to add some error reporting around this code as well, since the exception may be quite cryptic if the user sets DEFAULT_PYTHON to an invalid value.

Agreed. We should raise a PipxError() with a clear error message and suggested action for the user if the python interpreter can't be found.

wren · 2020-07-26T19:49:59Z

Sounds good. I can add some better error output.

For the use of shutil.which(): I'm happy to add if that's what y'all want, but I personally don't think it's desirable behavior. It can lead to some unpredictable results as people move in and out of virtual envs, subshells, etc. I think a full path gives users more control. And if a user really wants this functionality, they can add already do something like export PIPX_DEFAULT_PYTHON="$(which python)" to get the same result, so I'm not sure it would add anything other than a larger maintenance burden.

itsayellow · 2020-07-26T19:59:31Z

For the use of shutil.which(): I'm happy to add if that's what y'all want, but I personally don't think it's desirable behavior. It can lead to some unpredictable results as people move in and out of virtual envs, subshells, etc. I think a full path gives users more control. And if a user really wants this functionality, they can add already do something like export PIPX_DEFAULT_PYTHON="$(which python)" to get the same result, so I'm not sure it would add anything other than a larger maintenance burden.

I agree with this. I feel like being explicit in environment config is good. For our commands in an interactive shell it is nice that --python doesn't need a full path, but I don't feel the same way about env variables, and there is possibility of unpredictable results as @wren says.

uranusjr · 2020-07-27T01:52:42Z

Contrary to instincts, a shutil.which() may actually make things more explicit. Something like DEFAULT_PYTHON=python3 pipx install <package> would still work without shutil.which() since we eventually pass the value to subprocess, which does PATH resolution on the command anyway. But that resolution logic and behaviour can be different from what users expect, such as bpo-38905. shutil.which(), on the other hand, always behaves the same, and matches the expectation in the majority of cases.

Alternatively, We can ban that by doing Path.resolve(strict=True) with an explicit error message.

itsayellow · 2020-07-27T06:04:55Z

If we do use shutil.which I would also suggest a logging.info() statement showing the full resolved path that shutil.which returns, so in pipx verbose mode the user (and/or pipx debugging team) knows which python is getting used.

wren · 2020-08-06T22:31:59Z

Hi, just checking back on this.

I just need a final decision on whether or not to include shutil, and I can wrap this PR up.

itsayellow · 2020-08-07T01:28:10Z

I'm ok with it, and would like the logging.info() statement to show that we found PIPX_DEFAULT_PYTHON, are using it, and what the full resolved path is.

uranusjr · 2020-08-07T03:22:19Z

The shutil discussion and improvement can be made separatedly. But we’ll probably need to drop the resolve() part in that case, since that would covert python3 to $PWD/python3 which would be very confusing to the user.

cs01 · 2020-08-16T19:06:40Z

@itsayellow @uranusjr do you guys want to request changes or are you good merging like this?

uranusjr · 2020-08-17T02:47:27Z

I suggest improving treatment to relative paths, either by using resolve() to turn them absolute, or rejecting outright based on is_absolute().

itsayellow · 2020-08-17T03:54:16Z

I suggest adding a logging.info() statement inside of main.py setup() after statement logging.info(f"pipx version is {__version__}").
Something like:

logging.info(f"Default python interpreter: {constants.DEFAULT_PYTHON}")

In general this will make debugging much easier in the future.

wren · 2020-08-18T00:41:11Z

Sounds good. I'll do the requested updates this week. Thanks for the feedback!

gaborbernat · 2020-08-26T08:10:11Z

@wren any progress on this, you still plan to do it?

wren · 2020-08-26T14:40:42Z

Ah, yes. Time got away from me in the last week, sorry! I'll make some time today for it.

itsayellow · 2020-08-26T19:38:39Z

FYI black just made a new release hours ago, which is likely why your Lint test is failing.

I'm going to see if I can make a merge that will fix things soon.

itsayellow · 2020-08-26T20:32:53Z

OK, if you merge pipx master now the tests should work again.

wren · 2020-08-30T20:16:30Z

I just came back to check on this and realized I didn't hit submit on my comment. Sorry!

--

So, I've made the requested changes. I also added a call to shutil.which because it was the simplest way to deal with the relative path handling requirement.

The only test failing is related to typing (since the variable can now be None in addition to a string). I can fix this either by updating the type hints in the relevant functions, or by forcing a string. Either way is fine with me. Do y'all have a preference?

This keeps sys.executable as the default, but will use the PIPX_DEFAULT_PYTHON environment variable, if defined.

This avoids needing to update any type checking where DEFAULT_PYTHON is used.

wren · 2020-09-10T00:44:29Z

Hi, this is updated now. I believe this meets all of the previously established criteria:

DEFAULT_PYTHON is always a string (so no updating of type hints is necessary)
Supports the new Windows embeddable python code (Support running with the Windows embeddable Python distribution #465)
Any env var setting is validated with shutil.which
Sys executable (including the new windows one) is not run through which (since it's redundant)
A PipxError is raised when which fails
DEFAULT_PYTHON is updated with an absolute path if validation succeeds
logging statement in place for help debugging
The env var can still be overridden by passing --python on cli

Anything I've left out?

uranusjr

One minor nitpick, otherwise this looks good to me!

src/pipx/interpreter.py

itsayellow

Looks mostly good to me, just one little nit.

Also one more thing: Could you please list PIPX_DEFAULT_PYTHON and a help blurb for it in main.py around line 50, inside PIPX_DESCRIPTION, under "Optional Environment Variables".

src/pipx/interpreter.py

itsayellow · 2020-09-28T03:25:58Z

Hey @wren , just in case it got lost, could you please list PIPX_DEFAULT_PYTHON and a help blurb for it in main.py around line 50, inside PIPX_DESCRIPTION, under "Optional Environment Variables"? Then I'll merge this.

wren · 2020-09-29T18:56:30Z

@itsayellow Yup, sorry. I'm in the middle of moving right now, so it'll be a few days before I can get to this.

wren · 2020-10-04T19:34:03Z

@itsayellow All ready for merging. Thanks for your patience!

itsayellow · 2020-10-04T20:57:03Z

@wren, thanks for your persistence as well!

gaborbernat

…ompat-113

itsayellow · 2020-10-04T21:01:36Z

Just one more thing and I'll do it...adding changelog entry.

wren changed the title ~~Add env var to set DEFAULT_PYTHON~~ Add env var to set DEFAULT_PYTHON (add Pyenv support) Jul 13, 2020

wren force-pushed the add-pyenv-compat-113 branch 2 times, most recently from 0d378b9 to 801ddee Compare July 13, 2020 01:29

wren force-pushed the add-pyenv-compat-113 branch from 77f3aec to 22df9f3 Compare July 15, 2020 06:21

gaborbernat approved these changes Jul 24, 2020

View reviewed changes

cs01 reviewed Jul 26, 2020

View reviewed changes

wren force-pushed the add-pyenv-compat-113 branch from 6cd2530 to d31b1e4 Compare August 26, 2020 20:50

uranusjr mentioned this pull request Sep 7, 2020

installation is broken on MacOS ? #461

Closed

wren added 8 commits September 9, 2020 16:13

Add env var to set DEFAULT_PYTHON

349b43d

This keeps sys.executable as the default, but will use the PIPX_DEFAULT_PYTHON environment variable, if defined.

update help screen to include new env var PIPX_DEFAULT_PYTHON

7500374

add test for DEFAULT_PYTHON constant being set to bad executable

1e7894a

change test to better match project style

b30a5c2

add logging statement, which call, pipx error per feedback

2ecacfe

remove redundant function call

17ea16b

update formatting

f13a91e

Move DEFAULT_PYTHON validation to main.py

3ebc248

This avoids needing to update any type checking where DEFAULT_PYTHON is used.

wren force-pushed the add-pyenv-compat-113 branch from d31b1e4 to 3ebc248 Compare September 9, 2020 23:34

wren added 2 commits September 9, 2020 16:47

fix lint error

b03bd24

Move code around to fix conflict, include PR feedback

2f07e5c

uranusjr reviewed Sep 10, 2020

View reviewed changes

src/pipx/interpreter.py Outdated Show resolved Hide resolved

wren added 2 commits September 10, 2020 13:22

take out unused code

a540c01

add repr per PR feedback

0e76877

uranusjr approved these changes Sep 12, 2020

View reviewed changes

itsayellow requested changes Sep 19, 2020

View reviewed changes

src/pipx/interpreter.py Show resolved Hide resolved

update help screen per PR feedback

7e98d65

itsayellow approved these changes Oct 4, 2020

View reviewed changes

gaborbernat approved these changes Oct 4, 2020

View reviewed changes

Merge branch 'master' of github.com:pipxproject/pipx into add-pyenv-c…

57ed58e

…ompat-113

Add changelog entry.

ad051ba

itsayellow merged commit 1ec520f into pypa:master Oct 4, 2020

itsayellow mentioned this pull request Nov 19, 2020

[Q] Default Python interpreter #395

Closed



		def test_default_python(monkeypatch):
		monkeypatch.setattr(constants, "DEFAULT_PYTHON", "bad_python")

Add env var to set DEFAULT_PYTHON (add Pyenv support) #448

Add env var to set DEFAULT_PYTHON (add Pyenv support) #448

Conversation

wren commented Jul 13, 2020 • edited

cs01 commented Jul 13, 2020 • edited

wren commented Jul 13, 2020 • edited

Pyenv Tests

uranusjr commented Jul 13, 2020

cs01 commented Jul 14, 2020 • edited

wren commented Jul 15, 2020

uranusjr commented Jul 25, 2020

cs01 Jul 26, 2020

Choose a reason for hiding this comment

wren Jul 26, 2020

Choose a reason for hiding this comment

itsayellow Aug 7, 2020

Choose a reason for hiding this comment

cs01 commented Jul 26, 2020

wren commented Jul 26, 2020

itsayellow commented Jul 26, 2020

uranusjr commented Jul 27, 2020 • edited

itsayellow commented Jul 27, 2020

wren commented Aug 6, 2020

itsayellow commented Aug 7, 2020

uranusjr commented Aug 7, 2020

cs01 commented Aug 16, 2020

uranusjr commented Aug 17, 2020

itsayellow commented Aug 17, 2020 • edited

wren commented Aug 18, 2020

gaborbernat commented Aug 26, 2020

wren commented Aug 26, 2020

itsayellow commented Aug 26, 2020

itsayellow commented Aug 26, 2020

wren commented Aug 30, 2020

wren commented Sep 10, 2020

uranusjr left a comment

Choose a reason for hiding this comment

itsayellow left a comment • edited

Choose a reason for hiding this comment

itsayellow commented Sep 28, 2020

wren commented Sep 29, 2020

wren commented Oct 4, 2020

itsayellow commented Oct 4, 2020

gaborbernat left a comment

Choose a reason for hiding this comment

itsayellow commented Oct 4, 2020 • edited

wren commented Jul 13, 2020 •

edited

cs01 commented Jul 13, 2020 •

edited

wren commented Jul 13, 2020 •

edited

cs01 commented Jul 14, 2020 •

edited

uranusjr commented Jul 27, 2020 •

edited

itsayellow commented Aug 17, 2020 •

edited

itsayellow left a comment •

edited

itsayellow commented Oct 4, 2020 •

edited