add venv as an option for venv_backend #231

cs01 · 2019-08-05T05:55:49Z

Summary

This PR adds support for a different backend, venv. It adds a keyword argument to the VirtualEnv class, which defaults to False and is only true when the nox session has venv_backend=True.

It also updates the resolution of the Python path, since using venv alone does not fix issues when creating a venv from within a virtual environment.

The path resolution is modified to determine if a virtual environment is being used. If so, it tries to find the desired Python interpreter version in the base_prefix rather than anywhere on the system's PATH. Note that Windows was not tested, and probably will require some more changes.

Test Plan

# Create and enter virtual environment, then
cd ~/git/nox
pip install -e .
cd ~/git/pipx
# modify pipx's noxfile.py to have `venv_backend='venv'` for the unittests session
nox -s unittests-3.6  # tests pass

Additional context

prefix: A string giving the site-specific directory prefix where the platform independent Python files are installed

https://docs.python.org/3/library/sys.html#sys.prefix

base_prefix: Set during Python startup, before site.py is run, to the same value as prefix. If not running in a virtual environment, the values will stay the same; if site.py finds that a virtual environment is in use, the values of prefix and exec_prefix will be changed to point to the virtual environment, whereas base_prefix and base_exec_prefix will remain pointing to the base Python installation (the one which the virtual environment was created from).

https://docs.python.org/3/library/sys.html#sys.base_prefix

Tox's implementation for venv support is included as a plugin in an additional package: https://github.com/tox-dev/tox-venv/blob/master/src/tox_venv/hooks.py

fixes #199
cc @pfmoore

pfmoore

Some comments based on desk-checking the code, I'm not currently at a machine where I can test it.

pfmoore · 2019-08-05T08:38:12Z

nox/virtualenv.py

        if self.interpreter:
-            cmd.extend(["-p", self._resolved_interpreter])
+            cmd = [self._resolved_interpreter]


This change means that if virtualenv is used, it will now need to be installed in the resolved interpreter, whereas the current code only requires virtualenv to be installed in the current interpreter. It's not clear to me whether this would be a problem in practice, but it is a change in behaviour and if it's to be made, it should be done deliberately and not "by accident".

Whoops, that was a remnant of my hack to get pipx tests working earlier. I'll add the -p flag back in if virtualenv is being used. I'll fix it.

pfmoore · 2019-08-05T08:40:24Z

nox/virtualenv.py

-                "Re-using existing virtualenv at {}.".format(self.location_name)
+                "Re-using existing virtual environment at {}.".format(
+                    self.location_name
+                )


Is there a risk here that the existing environment used virtualenv, but the request was to use venv? If so, would this result in the caller getting the wrong type of environment? This is a real edge case, and probably isn't a major consideration, but it's worth noting.

Yes there is. Agreed this is an edge case. I suppose there could be a call made to identify whether it's a virtualenv or venv and to assert as necessary before continuing.

We also have an outstanding bug to make sure the -r checks the interpreter version as well. We might be able to just add a note to that issue that checking the backend should be part of the check as well. #123.

nox/virtualenv.py

pfmoore · 2019-08-05T08:47:57Z

nox/virtualenv.py

        if self.interpreter is None:
-            self._resolved = sys.executable
+            if currently_in_virtual_environment:
+                self._resolved = resolve_real_python_outside_venv(self.interpreter)


As it stands, this has no hope of working on Windows. At this point, we're calling resolve_real_python_outside_venv(None) (side note, it might be better to explicitly use None, rather than have the reader need to track back to the if statement to determine that self.interpreter is always None at this point). There's no check for _WINDOWS here (unlike the call below) so the fact that resolve_real_python_outside_venv doesn't (yet) support Windows will cause immediate breakage.

pfmoore · 2019-08-05T08:49:04Z

nox/virtualenv.py

@@ -238,7 +268,10 @@ def _resolved_interpreter(self):
            cleaned_interpreter = "python{}".format(xy_version)

        # If the cleaned interpreter is on the PATH, go ahead and return it.
-        if py.path.local.sysfind(cleaned_interpreter):
+        if currently_in_virtual_environment and _SYSTEM != "Windows":


You're ignoring cleaned_interpreter here, which seems weird. Why go to the trouble of calculating it and then not use it?

Good catch, I didn't intentionally ignore it

pfmoore · 2019-08-05T08:56:33Z

nox/virtualenv.py

+    See also:
+    https://docs.python.org/3/library/sys.html#sys.prefix
+    https://docs.python.org/3/library/sys.html#sys.base_prefix
+    """


This definitely won't work on Windows. There are various implementations of this type of "locate Python" algorithm on the web, including a reusable library version at https://github.com/sarugaku/pythonfinder.

Unfortunately, reusable code for this seems hard to locate (hence the reason so many people write their own) but IMO we should use a library if at all possible (pythonfinder is the one I've heard of, but I have no problem if someone recommends a better one).

pythonfinder looks like it can replace the entire existing nox function unless I'm missing something. Is that correct?

I replaced all the code with pythonfinder but it isn't able to find any installations. I'll keep trying with it, but won't be able to work on it any more until tonight. If it works it will simplify the code tremendously.

I've not used pythonfinder myself, I just knew about it from other projects. So I've no experience with it I'm afraid. I'll try to have a play with it myself, in case I get any ideas.

theacodes · 2019-08-05T17:16:47Z

I would definitely want to make the pythonfinder stuff a separate PR, if that's okay with y'all. Relatedly, I'm a bit wary about taking on new dependencies in Nox. It definitely warrants a bit of discussion.

…

On Mon, Aug 5, 2019 at 10:14 AM Chad Smith ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nox/virtualenv.py <#231 (comment)>: > @@ -174,6 +175,26 @@ def create(self): return True +def resolve_real_python_outside_venv(desired_intepreter: str) -> str: + """Return path to the real Python installation based + + See also: + https://docs.python.org/3/library/sys.html#sys.prefix + https://docs.python.org/3/library/sys.html#sys.base_prefix + """ I replaced all the code with pythonfinder but it isn't able to find any installations. I'll keep trying with it. If it works it will simplify the code tremendously. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#231>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB5I4ZLJ2EAUPFK4QNEYVDQDBNWLANCNFSM4IJHWZ4Q> .

pfmoore · 2019-08-05T17:33:01Z

I'm completely fine with the pythonfinder thing being a separate discussion. However, getting resolve_real_python_outside_venv to work without it is non-trivial (the existing code doesn't support Windows at all). I'll fish out some known-working code that does the job - both tox or virtualenv have this IIRC (and I have a suspicion I contributed the code in both cases!) Let me see what I can find.

But I do think that in the long term, duplicating this same bit of code in many projects is a bad idea...

cs01 · 2019-08-05T17:35:04Z

@pfmoore I'm pretty sure this is tox's: https://github.com/tox-dev/tox-venv/blob/master/src/tox_venv/hooks.py

theacodes · 2019-08-05T17:45:44Z

If we land the PR to switch to pythonfinder *first*, does this PR become easier/less complex?

…

On Mon, Aug 5, 2019 at 10:35 AM Chad Smith ***@***.***> wrote: @pfmoore <https://github.com/pfmoore> I'm pretty sure this is tox's: https://github.com/tox-dev/tox-venv/blob/master/src/tox_venv/hooks.py — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#231>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB5I472HOLILTZYLKOK7NLQDBQETANCNFSM4IJHWZ4Q> .

cs01 · 2019-08-05T17:48:32Z

@theacodes Assuming pythonfinder does what I think it does, yes.

This PR is really doing two things: adding the venv_backend option of venv, and updating Python resolution to make venv creation from within a virtual environment work. It makes sense to review the two changes separately; I can separate it out into two PRs.

pfmoore · 2019-08-05T18:29:30Z

I'm pretty sure this is tox's

I'm not sure that's all of it. A key part is tox_get_python_executable, which is defined in the tox.interpreters subpackage). To get the available interpreters on Windows you need to look in the registry (PEP 514).

As a simpler workaround, you can let the launcher do the work for you, and call py -X.Y -c "import sys; print(sys.executable)" in a subprocess, but you need to be careful with things like setting PYTHONIOENCODING so that you support full Unicode paths.

The reason I'm in favour of a library solution is that it's really tricky to get all the edge cases correct. Of course, users can always just supply the full executable path, so it's not like it's critical, but it's not a good UX if the software can't find a Python installation you know is there.

updating Python resolution to make venv creation from within a virtual environment work

Yeah, mixing virtualenvs and venvs is really messy. At this point I think you can safely create a virtualenv from within an active venv (see the discussions around pypa/virtualenv#1345 for the ugly details) but I don't think creating a venv from within an active virtual environment (either venv or virtualenv) is even supported. So getting this fixed would be good (for general reliability, even if there's no specific issue it's causing right now).

pfmoore · 2019-08-05T18:30:56Z

Something like the following should work for Windows, for what it's worth. Lightly tested, it works on my PC but I only have one Python version installed, so I couldn't test much...

import os
import subprocess

def get_python(major, minor=None):
    env = os.environ.copy()
    env['PYTHONIOENCODING'] = "utf-8"
    version = "-" + major
    if minor:
        version += "." + minor
    try:
        proc = subprocess.run(
            ["py", version, "-c", "import sys; print(sys.executable)"],
            env=env,
            capture_output=True
        )
    except Exception:
        # Just say we couldn't find anything if we get any sort of error
        return None
    if proc.returncode != 0:
        return None
    return proc.stdout.decode("utf-8").strip()

if __name__ == '__main__':
    import sys
    print(get_python(*sys.argv[1:]))

pfmoore · 2019-08-05T18:38:21Z

Whoa! I just looked at the list of dependencies for pythonfinder:

install_requires =
    attrs
    cached-property
    click
    crayons
    six
    packaging
    vistir[spinner]>=0.4

That's way too much for the job it's doing. Installing it pulled in 15 other packages, for a total of about 14M of data.

At this point, I'd say let's not use pythonfinder. That's just silly :-(

theacodes · 2019-08-06T05:29:02Z

nox/virtualenv.py


-        if self.interpreter:
+        if self.interpreter and self.venv_or_virtualenv == "virtualenv":


What happens if they request a different interpreter version than the one Nox is installed on?

Ah I think I see a bug.

In the case of venv we should call

[self._resolved_interpreter, 'venv', '-m', 'self.location']

rather than use sys.executable since you can't specify the python interpreter to use in venv (it just uses the Python you create the venv with).

In the case of virtualenv it will remain unchanged

[sys.executable, 'virtualenv', '-m', 'self.location', '-p', self._resolved_interpreter]

cs01 · 2019-08-06T16:37:49Z

Whoa! I just looked at the list of dependencies for pythonfinder:
install_requires =
    attrs
    cached-property
    click
    crayons
    six
    packaging
    vistir[spinner]>=0.4
That's way too much for the job it's doing. Installing it pulled in 15 other packages, for a total of about 14M of data.

At this point, I'd say let's not use pythonfinder. That's just silly :-(

Just FYI I filed an issue in pythonfinder on this: sarugaku/pythonfinder#78

cs01 · 2019-08-11T00:11:52Z

Got the tests passing 🎉 .

So to summarize, this PR adds support for venv as an alternate backend in a nox session. It does not modify the logic when resolving the interpreter, which is required to create a new venv from within a venv. That can be done in a different PR, possibly with pythonfinder, depending on how sarugaku/pythonfinder#78 evolves.

theacodes · 2019-08-12T02:01:31Z

Thank you, @cs01 and @pfmoore!

Does this need a follow-up PR for documentation as well, or do we think it's sufficiently advanced enough that most users won't need it?

cs01 · 2019-08-12T16:57:36Z

Thank you, @cs01 and @pfmoore!

Does this need a follow-up PR for documentation as well, or do we think it's sufficiently advanced enough that most users won't need it?

I don't really have a feeling for how many other people want to use this. AFAIK the only reason you'd want it is to test a program that creates virtual environments. So that would include pipx, pipenv, poetry, virtualenv, venv, and maybe some others. Because of this, I don't know if there is much value added to using the venv backend without the improved Python resolution, so I was planning on waiting until the improved Python resolution was added before documenting it.

theacodes · 2019-08-12T16:58:33Z

Sounds good. Thank you.

…

On Mon, Aug 12, 2019, 9:57 AM Chad Smith ***@***.***> wrote: Thank you, @cs01 <https://github.com/cs01> and @pfmoore <https://github.com/pfmoore>! Does this need a follow-up PR for documentation as well, or do we think it's sufficiently advanced enough that most users won't need it? I don't really have a feeling for how many other people want to use this. AFAIK the only reason you'd want it is to test a program that creates virtual environments. So that would include pipx, pipenv, poetry, virtualenv, venv, and maybe some others. Because of this, I don't know if there is much value added to using the venv backend without the improved Python resolution, so I was planning on waiting until the improved Python resolution was added before documenting it. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#231>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB5I467HHIGZ4VNMSZONSTQEGJADANCNFSM4IJHWZ4Q> .

gaborbernat · 2019-10-20T11:43:20Z

Hmmm, I've been working on making virtualenv more like venv, and add python discovery part of it - see https://github.com/gaborbernat/virtualenv/blob/rewrite/src/virtualenv/interpreters/discovery/__init__.py 🤔 probably will be released in a month or so 🤔 so this entire ticket should self solve itself then 🤔

cs01 · 2019-10-20T15:31:56Z

venv is now a valid backend for nox, so nox can create venvs, but interpreter resolution in nox still needs to be improved since it still defaults to the python in the venv.

Are you saying that in the future, nox can use virtualenv but still find the base interpreter from there (I think currently it can only do this from venv), or that the default interpreter in virtualenv will be the base interpreter and therefore able to create a new venv/virtualenv from there? (This is the PR, open issue is at #233)

gaborbernat · 2019-10-20T16:56:44Z

virtualenv's future discovery mechanism will always let you know the base interpreter too (for venv/old virtualenv/host python) 👍 furthermore new virtualenv environments will be equivalent with current venvs.

cs01 · 2019-10-20T19:29:48Z

Right now, nox has a @property called _resolved_interpreter. Just want to confirm I am understanding the new workflow: In the future we will be importing virtualenv into nox's virtualenv.py module and using a new method in _resolved_interpreter that allows us to get the base interpreter?

gaborbernat · 2019-10-21T12:14:58Z

Not sure how this will integrate into nox; but virtualenv will contain the logic of interpreter discovery (with option to also access the host interpreter, not just the immediate discovered one). tox will definitely use this and no longer implement its own version of this functionality, nox might or might not choose to do so, 🤷‍♂ it's up to @theacodes

add venv as an option for venv_backend

b362552

pfmoore suggested changes Aug 5, 2019

View reviewed changes

cs01 added 2 commits August 5, 2019 18:19

remove python resolution changes

0cfb341

update tests

06edcd8

cs01 mentioned this pull request Aug 6, 2019

Reduce/Eliminate dependencies sarugaku/pythonfinder#78

Open

run black

db84d10

theacodes reviewed Aug 6, 2019

View reviewed changes

use different interpreters for venv vs virtualenv

5d5c737

cs01 mentioned this pull request Aug 6, 2019

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

Closed

cs01 added 2 commits August 10, 2019 16:51

update test

70f467e

Merge branch 'master' of github.com:theacodes/nox into cs01/use-venv

6c5ebdc

theacodes mentioned this pull request Aug 12, 2019

reuse_existing should check the interpreter #123

Closed

theacodes approved these changes Aug 12, 2019

View reviewed changes

theacodes merged commit 9477a91 into wntrblm:master Aug 12, 2019

cs01 deleted the cs01/use-venv branch August 12, 2019 02:19

vaneseltine mentioned this pull request Sep 26, 2019

Prefer py launcher to suffixed Pythons found on PATH on Windows #250

Open

cs01 mentioned this pull request Oct 18, 2019

Travis CI and upstream nox pypa/pipx#235

Closed

itsayellow mentioned this pull request Sep 29, 2020

Can we use regular nox (and not the special native-venv nox fork?) pypa/pipx#508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add venv as an option for venv_backend #231

add venv as an option for venv_backend #231

cs01 commented Aug 5, 2019

pfmoore left a comment

pfmoore Aug 5, 2019

cs01 Aug 5, 2019

pfmoore Aug 5, 2019

cs01 Aug 6, 2019

theacodes Aug 6, 2019

pfmoore Aug 5, 2019

cs01 Aug 5, 2019

pfmoore Aug 5, 2019

cs01 Aug 5, 2019

pfmoore Aug 5, 2019

cs01 Aug 5, 2019

cs01 Aug 5, 2019 •

edited

pfmoore Aug 5, 2019

theacodes commented Aug 5, 2019 via email

pfmoore commented Aug 5, 2019

cs01 commented Aug 5, 2019

theacodes commented Aug 5, 2019 via email

cs01 commented Aug 5, 2019 •

edited

pfmoore commented Aug 5, 2019

pfmoore commented Aug 5, 2019

pfmoore commented Aug 5, 2019

theacodes Aug 6, 2019

cs01 Aug 6, 2019

cs01 commented Aug 6, 2019

cs01 commented Aug 11, 2019

theacodes commented Aug 12, 2019

cs01 commented Aug 12, 2019

theacodes commented Aug 12, 2019 via email

gaborbernat commented Oct 20, 2019

cs01 commented Oct 20, 2019

gaborbernat commented Oct 20, 2019 •

edited

cs01 commented Oct 20, 2019 •

edited

gaborbernat commented Oct 21, 2019


		if self.interpreter:
		if self.interpreter and self.venv_or_virtualenv == "virtualenv":

add venv as an option for venv_backend #231

add venv as an option for venv_backend #231

Conversation

cs01 commented Aug 5, 2019

Summary

Test Plan

Additional context

pfmoore left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cs01 Aug 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theacodes commented Aug 5, 2019 via email

pfmoore commented Aug 5, 2019

cs01 commented Aug 5, 2019

theacodes commented Aug 5, 2019 via email

cs01 commented Aug 5, 2019 • edited

pfmoore commented Aug 5, 2019

pfmoore commented Aug 5, 2019

pfmoore commented Aug 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cs01 commented Aug 6, 2019

cs01 commented Aug 11, 2019

theacodes commented Aug 12, 2019

cs01 commented Aug 12, 2019

theacodes commented Aug 12, 2019 via email

gaborbernat commented Oct 20, 2019

cs01 commented Oct 20, 2019

gaborbernat commented Oct 20, 2019 • edited

cs01 commented Oct 20, 2019 • edited

gaborbernat commented Oct 21, 2019

cs01 Aug 5, 2019 •

edited

cs01 commented Aug 5, 2019 •

edited

gaborbernat commented Oct 20, 2019 •

edited

cs01 commented Oct 20, 2019 •

edited