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

add venv as an option for venv_backend #231

Merged
merged 7 commits into from
Aug 12, 2019
Merged

add venv as an option for venv_backend #231

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b362552
add venv as an option for venv_backend
cs01 Aug 5, 2019
0cfb341
remove python resolution changes
cs01 Aug 6, 2019
06edcd8
update tests
cs01 Aug 6, 2019
db84d10
run black
cs01 Aug 6, 2019
5d5c737
use different interpreters for venv vs virtualenv
cs01 Aug 6, 2019
70f467e
update test
cs01 Aug 10, 2019
6c5ebdc
Merge branch 'master' of github.com:theacodes/nox into cs01/use-venv
cs01 Aug 10, 2019
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
7 changes: 7 additions & 0 deletions nox/sessions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,13 @@ def _create_venv(self):
self.venv = CondaEnv(
path, interpreter=self.func.python, reuse_existing=reuse_existing
)
elif self.func.venv_backend == "venv":
self.venv = VirtualEnv(
path,
interpreter=self.func.python,
reuse_existing=reuse_existing,
venv=True,
)
else:
raise ValueError(
"Expected venv_backend one of ('virtualenv', 'conda'), but got '{}'.".format(
Expand Down
58 changes: 48 additions & 10 deletions nox/virtualenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

import os
from pathlib import Path
import platform
import re
import shutil
Expand Down Expand Up @@ -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
cs01 marked this conversation as resolved.
Show resolved Hide resolved

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

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cs01 cs01 Aug 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

interpreter = (
desired_intepreter
if desired_intepreter
else f"{sys.version_info.major}.{sys.version_info.minor}"
)
python_str = f"python{interpreter}" # i.e. python3.6

base_python_installation = Path(sys.base_prefix) / "bin" / python_str
if base_python_installation.is_file():
return str(base_python_installation)
raise InterpreterNotFound(interpreter)


class VirtualEnv(ProcessEnv):
"""Virtualenv management class.

Expand All @@ -196,12 +217,15 @@ class VirtualEnv(ProcessEnv):

is_sandboxed = True

def __init__(self, location, interpreter=None, reuse_existing=False):
def __init__(
self, location, interpreter=None, reuse_existing=False, *, venv: bool = False
):
self.location_name = location
self.location = os.path.abspath(location)
self.interpreter = interpreter
self._resolved = None
self.reuse_existing = reuse_existing
self.venv_or_virtualenv = "venv" if venv else "virtualenv"
super(VirtualEnv, self).__init__()

_clean_location = _clean_location
Expand All @@ -212,6 +236,7 @@ def _resolved_interpreter(self):

Based heavily on tox's implementation (tox/interpreters.py).
"""

# If there is no assigned interpreter, then use the same one used by
# Nox.
if isinstance(self._resolved, Exception):
Expand All @@ -220,8 +245,13 @@ def _resolved_interpreter(self):
if self._resolved is not None:
return self._resolved

currently_in_virtual_environment = sys.prefix != sys.base_prefix

if self.interpreter is None:
self._resolved = sys.executable
if currently_in_virtual_environment:
self._resolved = resolve_real_python_outside_venv(self.interpreter)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

else:
self._resolved = sys.executable
return self._resolved

# Otherwise we need to divine the path to the interpreter. This is
Expand All @@ -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":
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I didn't intentionally ignore it

self._resolved = resolve_real_python_outside_venv(self.interpreter)
return self._resolved
elif py.path.local.sysfind(cleaned_interpreter):
self._resolved = cleaned_interpreter
return self._resolved

Expand Down Expand Up @@ -275,21 +308,26 @@ def bin(self):
return os.path.join(self.location, "bin")

def create(self):
"""Create the virtualenv."""
"""Create the virtualenv or venv."""
if not self._clean_location():
logger.debug(
"Re-using existing virtualenv at {}.".format(self.location_name)
"Re-using existing virtual environment at {}.".format(
self.location_name
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

)
return False

cmd = [sys.executable, "-m", "virtualenv", self.location]

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

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

else:
cmd = [sys.executable]
cmd += ["-m", self.venv_or_virtualenv, self.location]

logger.info(
"Creating virtualenv using {} in {}".format(
os.path.basename(self._resolved_interpreter), self.location_name
"Creating virtual environment ({}) using {} in {}".format(
self.venv_or_virtualenv,
os.path.basename(self._resolved_interpreter),
self.location_name,
)
)
nox.command.run(cmd, silent=True, log=False)
Expand Down