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

use base python installation (not virtual environment's Python) during python interpreter resolution #233

Closed
cs01 opened this issue Aug 6, 2019 · 25 comments

Comments

@cs01
Copy link
Contributor

cs01 commented Aug 6, 2019

Describe the bug

Current logic to determine which python interperter to use will use whichever is on the current PATH, which can be a virtual environment's python (venv or virtualenv) or a system installation.

This can be problematic when creating new virtual environments, as pipx and probably others like pipenv must do.

How to reproduce

Run pipx's unit tests. Clone pipx, then run nox.

>> nox
nox > Running session unittests-3.6
nox > Creating virtualenv using python3.6 in .nox/unittests-3-6
nox > pip install .
nox > python -m unittest discover
E
======================================================================
ERROR: tests.test_pipx (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_pipx
Traceback (most recent call last):
  File "/usr/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
    __import__(name)
  File "/home/csmith/git/pipx/tests/test_pipx.py", line 17, in <module>
    assert not hasattr(sys, "real_prefix"), "Tests cannot run under virtualenv"
AssertionError: Tests cannot run under virtualenv


----------------------------------------------------------------------
Ran 1 test in 0.000s

FAILED (errors=1)

Expected behavior

The base python interpreter should be resolved rather than the virtual environment's python interpreter.

Example implementations for interpreter resolution:

Note: #199 suggests using venv will fix this. I think this is necessary but not sufficient to create a new venv from within a virtual environment. (cc @pfmoore)

@omry
Copy link
Contributor

omry commented Aug 24, 2019

While a recent fix improved the situation under windows, things are still a big flaky.
for example, it's unclear how to test python 2.7 (we can't have it in the path as Python because then nox would not run).

Also,

(hydra36) PS>nox -s "test_plugin-3.6(fairtask, pip install -e)"
nox > Running session test_plugin-3.6(fairtask, pip install -e)
nox > Creating virtual environment (virtualenv) using python.EXE in .nox\test_plugin-3-6-fairtask-pip-install-e
nox > pip install --upgrade setuptools pip
nox > python C:\Users\Administrator\checkout\hydra\plugins\fairtask\setup.py --classifiers
]..7> Session test_plugin-3.6(fairtask, pip install -e) skipped: Not testing fairtask on Python 3.6, supports [3.6

VS this on the same environment:

(hydra36) PS>nox -s "test_core-3.6(pip install -e)"
...
nox > Session test_core-3.6(pip install -e) was successful.

So things are still flaky in this department.

One way to make this better is to allow the user to actually indicate which python interpreter to use more directly while still maintaining compatibility with the existing logic.

@nox.session(python='2.7')
def tests(session):
    pass

can be made to also support:

@nox.session(python='2.7', python_path='c:/python2/bin/python.exe')
def tests(session):
    pass

Or possibly, to make it easier to control from an environment variable:

@nox.session(python='2.7=c:/python2/bin/python.exe')
def tests(session):
    pass

Thoughts?

@theacodes
Copy link
Collaborator

theacodes commented Aug 26, 2019 via email

@omry
Copy link
Contributor

omry commented Aug 27, 2019

Not into the noxfile, Into an environment variable.

that was a simplified example to explain the syntax, what I actually had in mind was something like:

DEFAULT_PYTHON_VERSIONS = ["2.7", "3.5", "3.6", "3.7"]

PYTHON_VERSIONS = os.environ.get(
    "NOX_PYTHON_VERSIONS", ",".join(DEFAULT_PYTHON_VERSIONS)
).split(",")


@nox.session(python=PYTHON_VERSIONS)
def test_core(session):
    pass

And then:

export NOX_PYTHON_VERSIONS=2.7=c:/python2/bin/python.exe,3.5,3.6
nox

@omry
Copy link
Contributor

omry commented Aug 27, 2019

A possibly cleaner approach would be a nox specific environment variables:

NOX_PYTHON_OVERRIDE_PY27=c:/python2/bin/python.exe

User code and logic are unchanged, but now nox would resolve python 2.7 with this environment variable if it's present.

@omry
Copy link
Contributor

omry commented Aug 30, 2019

@theacodes, do you think this approach would be preferred?
right now I am not quiet able to test python 2.7 on Windows, so workarounds would be appreciated.
I tried several tricks but was not able to get nox to detect the right python2.7 interpreter.

@theacodes
Copy link
Collaborator

theacodes commented Sep 4, 2019 via email

@omry
Copy link
Contributor

omry commented Sep 4, 2019

Thanks!

Regardless of this, I think a method to specify what Python interpreter to use for a given Python version can be a helpful way to short cut through imperfect heuristics in the rare cases they don't do what I want - but your call :).

@theacodes
Copy link
Collaborator

theacodes commented Sep 4, 2019 via email

@omry
Copy link
Contributor

omry commented Sep 4, 2019

Do you have any reservations about my second suggestion to use a per-version optional environment variable?

NOX_PYTHON_OVERRIDE_PY27=c:/python2/bin/python.exe

@theacodes
Copy link
Collaborator

theacodes commented Sep 4, 2019 via email

@chrahunt
Copy link

In case we needed another example where the current checks fail us: logs

The problem is that in the GitHub Actions runner there is a version of Python 2.7 installed in C:\ProgramData\Chocolatey\bin\python2.7.exe which resolves to the one installed in the mingw folder. When we resolve to this one the virtualenv creation step fails.

As a workaround, deleting that python2.7.exe causes nox to resolve to something else (probably via py -2.7), which works as expected. See also pypa/get-pip#48.

I'm not sure how well an environment variable would work in this case - the base directory of the installation is available with ${{env.pythonLocation}}, but if we set that unconditionally then it would be wrong for non-Python 2.7 runs (but it also wouldn't break anything). The installed directory contains the patch version number, so hard-coding it would be pretty fragile (see here).

@omry
Copy link
Contributor

omry commented Sep 17, 2019

@chrahunt, if you know the location of the python2.7 in advance, you would have added to your circle config for python 2.7 runs:

NOX_PYTHON_OVERRIDE_PY27=path_python_2.7_you_want

For me at least, I have a separate circle job for each python version (and each OS version as well) so having a - per OS+python ver logic is acceptable.

@theacodes
Copy link
Collaborator

Let's see what @dhermes @lukesneeringer @stsewd and @crwilcox think?

I think for the issue described by @chrahunt in #233 (comment) we could fix that by preferring the py.exe interpreter over whats in PATH on Windows. Could be a case of just re-arranging the resolution logic.

For other cases, I'm not sure how we should handle ambiguous resolution. Nox currently early-outs when it finds an interpreter that looks good. Should we keep going and then somehow figure out how to pick one out of many possible interpreters?

And then there's the question of letting users somehow specify the exact location for an interpreter. I'm not super keen on environment variables but I'm also not keen on having a dozen more command-line options to handle it.

@chrahunt
Copy link

Yes, giving priority to py.exe on Windows would fix my issue. I think it would be a pretty nice approach too, since py itself lets you specify which Python to resolve to in the registry, and presumably installers will do that by default (or would be open to PRs/feature requests to do it).

@dhermes
Copy link
Collaborator

dhermes commented Sep 18, 2019

Is it safe to summarize that this is a strictly Windows problem? If yes, then isn't the py launcher absolutely the preferred first check on Windows (i.e. as @chrahunt mentions it actually checks the winreg) and wouldn't that clear up most of the other issues if we put the check via py ahead of a %PATH% check?

@omry
Copy link
Contributor

omry commented Sep 18, 2019

I think we have two issues here:

  1. Windows specific resolution issue (at least one, possibly more than one).
  2. Implementing a solution to allows specifying individual python binaries explicitly as a general escape hatch for all sorts of non standard cases.

Those two things are not in conflict, and while implementing 1 alleviates the pressure to implement 2, we WILL have additional python resolution issues in the future.

@cs01
Copy link
Contributor Author

cs01 commented Sep 18, 2019

The intent of the issue when I originally created it was for nox to find and use system Python installations when working from within a virtual environment, rather than using the Python installation from within a virtual environment (new virtual environments can only be created with Python executables outside of the virtual environment). Fixing this would allow projects like pipx and pipenv to use nox for testing. I should have been more clear in the original issue title. Perhaps new, more targeted issues can be created for the other topics that have come up. I will rename this issue to be more precise as well.

@cs01 cs01 changed the title improve python interpreter resolution use base python installation (not virtual environment's Python) during python interpreter resolution Sep 18, 2019
@chrahunt
Copy link

Thanks @cs01, moved over to #250.

@stsewd
Copy link
Collaborator

stsewd commented Sep 21, 2019

Sorry for the late reply, I don't have experience on windows, so I can't have an opinion there. For other cases maybe an option like NOX_EXTRA_PYTHON_PATHS?

@crwilcox
Copy link
Collaborator

Also sorry for delay getting around to this I agree that preferring py.exe on windows is likely the best path forward. It will keep version resolution out of nox. It would be odd to add logic that didn't respect the path and I imagine would be confusing. I think we can close this and move discussion to #250.

@cs01 does this resolve pipx/pipenv issues?

@cs01
Copy link
Contributor Author

cs01 commented Oct 20, 2019

Also sorry for delay getting around to this I agree that preferring py.exe on windows is likely the best path forward. It will keep version resolution out of nox. It would be odd to add logic that didn't respect the path and I imagine would be confusing. I think we can close this and move discussion to #250.

@cs01 does this resolve pipx/pipenv issues?

I think using py.exe to find interpreters on windows would fix it since that presumably always points users to base interpreters (disclosure: I do not use windows and have never used py.exe). It wouldn't solve anything on mac/linux, but I have a fix for that I can add to nox's code.

@cs01
Copy link
Contributor Author

cs01 commented Oct 21, 2019

@gaborbernat says virtualenv will soon have code that can do this, so we should give that a shot once it's landed. #231 (comment)

@theacodes theacodes added this to the Near-term milestone Mar 7, 2021
@cjolowicz
Copy link
Collaborator

Hey @omry

  1. Implementing a solution to allows specifying individual python binaries explicitly as a general escape hatch for all sorts of non standard cases.

As of Nox 2020.12.31, this can be achieved by passing the full interpreter path via --extra-python, for example:

nox --extra-python=/usr/local/bin/python3

This will still resolve the other interpreters normally. To restrict the run to a specific interpreter, you'd need to pass the path to --python as well, like this:

nox --python=/usr/local/bin/python3 --extra-python=/usr/local/bin/python3

(We may introduce a shorthand for this eventually.)

Would this technique work for your use case?

@cs01
Copy link
Contributor Author

cs01 commented May 31, 2021

Just for the record this is no longer a record for me, as the pipx tests now work with nox. I am not sure exactly what changed, but I suspect the venv module has been modified. In any case, I am fine if this issue is closed.

@cjolowicz
Copy link
Collaborator

Thanks @cs01 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants