feat: added venv_location option to specify virtualenv location #785

wpk-nist-gov · 2024-02-27T13:44:55Z

This PR adds the option venv_location to specify the location for virtualenv creation and a per session basis, and is a follow up to #753.

The use case is quickly defining development environments:

@nox.session(venv_location=".venv")
def dev(session):
    ....

henryiii · 2024-02-27T16:44:21Z

This could also be used by power users to have multiple sessions share a single venv. Nicely combines with better logic on venv reuse. :)

wpk-nist-gov · 2024-02-27T17:53:04Z

Yes, I was thinking the same thing! I've used a similar (not supported) feature in tox in the past. I'll play around with this possible functionality...

cjolowicz · 2024-02-27T18:05:48Z

Does this allow arbitrary path traversal?

wpk-nist-gov · 2024-02-27T19:31:48Z

Does this allow arbitrary path traversal?

Sorry @cjolowicz, I'm not sure what you mean.

If you mean can you place environments anywhere, the answer is yes. So you could do the following:

@nox.session(venv_location=os.path.expanduser("~/venvs/project-venv"))
def dev(session):
    ....

To place a dev environment at the location "~/venv/project-venv".

Just pushed a commit that wraps venv_location with os.path.expanduser to automatically expand "~/", so the above call to os.path.expanduser is unneeded...

There was an edge case where the user could pass something like `venv_locaiton="~/tmp/venv"`, but this would be place in a directory `$PWD/~/tmp/venv`. So now use `os.path.expanduser(venv_location)` to fix this.

Got rid of venv_location tests that created folder in home directory

henryiii · 2024-02-27T22:13:19Z

nox/registry.py

@@ -55,6 +56,7 @@ def session_decorator(
    name: str | None = None,
    venv_backend: Any | None = None,
    venv_params: Any | None = None,
+    venv_location: str | None = None,


These should support Path too, I think.

Totally agree. Will update to os.PathLike

henryiii · 2024-02-27T22:17:53Z

wrap venv_location with os.path.expanuser

Do we do that anywhere else? Any reason not to let a user make it explicit? Path.home() would work, for example.

wpk-nist-gov · 2024-02-28T00:15:29Z

The call to os.pathexpanduser was just to catch an edge case. Not necessarily needed. But it also will covert pathlib.Path objects to strings correctly. Most other paths in the code at some point get passed through os.path.join or similar which typically ensures they are path-like strings.

When I was working on this, I resisted the urge to convert string paths to pathlib.Path objects, as nox uses string paths and the os.path module exclusively. Maybe a future PR is in store converting to pathlib...

For now, I'll make a change allowing os.PathLike objects for venv_location

The user can pass a string or Path object to set venv_location. Note that for now, I left in `os.path.expanduser`. This coverts things like `venv_location="~/path/to/dir"` to `$HOME/path/to/dir` instead of `$PWD/~/path/to/dir`. If you really want the latter behavior, you can pass in `Path.cwd().joinpath("~", "path", "to", "dir")`

cjolowicz · 2024-02-28T07:19:18Z

If you mean can you place environments anywhere, the answer is yes.

That is what I meant, thanks. Absolute paths won't be very portable and may be surprising behavior when you run Nox on somebody else's project. Why do you think this should be possible?

How does the venv_location feature work for parameterized sessions including sessions with multiple Pythons?

cjolowicz · 2024-02-28T07:30:06Z

Are we concerned about users trying something like venv_location="tests"? They might expect this to land under .nox but it would create files within their test suite.

henryiii · 2024-02-28T15:09:06Z

How would this interact with parametrization? I could see it potentially being very useful to allow a parametrized test to reuse the same environment, but some cases that would be bad, especially if Python was parametrized over (the most common one!). Would it make sense to allow .format substitution in the path, providing the parametrized values? Edit: I didn't read the edited comment above, sorry for repeating it.

wpk-nist-gov · 2024-02-28T16:22:03Z

That is what I meant, thanks. Absolute paths won't be very portable and may be surprising behavior when you run Nox on somebody else's project. Why do you think this should be possible?

I'm fine restricting venv_location to be below the current directory, if that's the consensus? I don't have strong feelings on the matter.

How does the venv_location feature work for parameterized sessions including sessions with multiple Pythons?

Combining with the comment from @henryiii, I like the idea of using .format, so you could use something like venv_location="my-env-{python}". But I'm hesitant to add this functionality in general. In my opinion, you should use the default behavior for these cases. Maybe for now, raise an error if using venv_location with parametrize? I'm not sure how to do this...

Are we concerned about users trying something like venv_location="tests"? They might expect this to land under .nox but it would create files within their test suite.

Yes, I agree this could be an issue. How about a check like: if venv_location exists, it must contains {venv_location}/pyvenv.cfg or {venv_location}/conda-meta?

(EDIT: I'm not sure there's a general solution to testing for a venv)

henryiii · 2024-02-28T16:25:38Z

Does {venv_location} contain pyvenv.cfg on Windows? I'm not sure it does, for at least one of venv/virtualenv.

wpk-nist-gov · 2024-02-28T16:38:02Z

Does {venv_location} contain pyvenv.cfg on Windows? I'm not sure it does, for at least one of venv/virtualenv.

No clue. Is there a general way to test if a directory is venv/virtualenv/conda-env?

Sorry, accidentally closed PR. Reopened.

cjolowicz · 2024-02-28T19:24:48Z

A pyvenv.cfg with a home key is always present in a virtual environment, CPython needs it to build sys.path. It doesn't depend on the tool that creates it or the platform.

henryiii · 2024-02-28T20:31:33Z

That's what I thought, but some of the tests are disabled on Windows looking for it (and virutalenv < 20 didn't add one it seems from comments).

wpk-nist-gov · 2024-02-28T21:17:02Z

A pyvenv.cfg with a home key is always present in a virtual environment, CPython needs it to build sys.path. It doesn't depend on the tool that creates it or the platform.

In this case, should I add a check that, if venv_location exists, that it is a virtualenv?

Just an FYI, using tox devenv my-location (the partial inspiration for venv_location) will place environments anywhere (it expands "~/" to the home directory), and doesn't check the specified directory (will overwrite/delete/etc)...

* Now raises error if passing directly multiple pythons or parametrize for session with `venv_location` * With `venv_location` set, ignore (and warn) `--extra-python` * Warn that session sets `venv_location`

cjolowicz · 2024-02-29T09:06:57Z

In this case, should I add a check that, if venv_location exists, that it is a virtualenv?

Yes, I think this is a good safeguard.

I'm fine restricting venv_location to be below the current directory, if that's the consensus? I don't have strong feelings on the matter.

Either that or just requiring a relative path. I'm not opposed to relaxing this restriction later if people come up with good use cases.

Maybe for now, raise an error if using venv_location with parametrize? I'm not sure how to do this...

Yes, I'd prefer this solution, also for sessions with multiple Pythons. Let's see some real world usage of this feature before we add something like templating to it. One approach would be to check this in Manifest.make_session (similar to how we warn about backend="none" but resulting in a hard error instead of a warning). Another approach would be to error out when we create the environment. I'm not sure which is best.

wpk-nist-gov · 2024-02-29T16:43:10Z

The latest commit raises an error if trying to pass multiple pythons to a session with venv_location. It also ignores (with warning) any --extra-python or --force-python flags, although I'm thinking it should handle the case where passing a single value of --force-python.

TODO

Make venv_location always relative. And hopefully fix the bug in windows CI...
Add check for venv before overwriting directory
Proper handling of --force-python if single valued.
Possible addition. Add --devenv flag that the could be used to direct any session to a user defined venv_location

feat: added venv_location option to specify virtualenv location

8636ce3

wpk added 2 commits February 27, 2024 15:05

'chore: wrap venv_location with os.path.expanuser

ea8e444

There was an edge case where the user could pass something like `venv_locaiton="~/tmp/venv"`, but this would be place in a directory `$PWD/~/tmp/venv`. So now use `os.path.expanduser(venv_location)` to fix this.

chore: fix issue with tests

3c8949e

Got rid of venv_location tests that created folder in home directory

henryiii reviewed Feb 27, 2024

View reviewed changes

wpk-nist-gov closed this Feb 28, 2024

chore: fix error in python3.8 CI from _typing.py

754cecb

wpk-nist-gov reopened this Feb 28, 2024

chore: Safety check venv_location for multiple python or parametrize

ff03ae4

* Now raises error if passing directly multiple pythons or parametrize for session with `venv_location` * With `venv_location` set, ignore (and warn) `--extra-python` * Warn that session sets `venv_location`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added venv_location option to specify virtualenv location #785

feat: added venv_location option to specify virtualenv location #785

wpk-nist-gov commented Feb 27, 2024

henryiii commented Feb 27, 2024

wpk-nist-gov commented Feb 27, 2024 via email •

edited

cjolowicz commented Feb 27, 2024

wpk-nist-gov commented Feb 27, 2024 •

edited

henryiii Feb 27, 2024

wpk-nist-gov Feb 28, 2024

henryiii commented Feb 27, 2024

wpk-nist-gov commented Feb 28, 2024 •

edited

cjolowicz commented Feb 28, 2024 •

edited

cjolowicz commented Feb 28, 2024

henryiii commented Feb 28, 2024 •

edited

wpk-nist-gov commented Feb 28, 2024 •

edited

henryiii commented Feb 28, 2024

wpk-nist-gov commented Feb 28, 2024 •

edited

cjolowicz commented Feb 28, 2024 •

edited

henryiii commented Feb 28, 2024

wpk-nist-gov commented Feb 28, 2024 •

edited

cjolowicz commented Feb 29, 2024

wpk-nist-gov commented Feb 29, 2024

feat: added venv_location option to specify virtualenv location #785

Are you sure you want to change the base?

feat: added venv_location option to specify virtualenv location #785

Conversation

wpk-nist-gov commented Feb 27, 2024

henryiii commented Feb 27, 2024

wpk-nist-gov commented Feb 27, 2024 via email • edited

cjolowicz commented Feb 27, 2024

wpk-nist-gov commented Feb 27, 2024 • edited

henryiii Feb 27, 2024

Choose a reason for hiding this comment

wpk-nist-gov Feb 28, 2024

Choose a reason for hiding this comment

henryiii commented Feb 27, 2024

wpk-nist-gov commented Feb 28, 2024 • edited

cjolowicz commented Feb 28, 2024 • edited

cjolowicz commented Feb 28, 2024

henryiii commented Feb 28, 2024 • edited

wpk-nist-gov commented Feb 28, 2024 • edited

henryiii commented Feb 28, 2024

wpk-nist-gov commented Feb 28, 2024 • edited

cjolowicz commented Feb 28, 2024 • edited

henryiii commented Feb 28, 2024

wpk-nist-gov commented Feb 28, 2024 • edited

cjolowicz commented Feb 29, 2024

wpk-nist-gov commented Feb 29, 2024

wpk-nist-gov commented Feb 27, 2024 via email •

edited

wpk-nist-gov commented Feb 27, 2024 •

edited

wpk-nist-gov commented Feb 28, 2024 •

edited

cjolowicz commented Feb 28, 2024 •

edited

henryiii commented Feb 28, 2024 •

edited

wpk-nist-gov commented Feb 28, 2024 •

edited

wpk-nist-gov commented Feb 28, 2024 •

edited

cjolowicz commented Feb 28, 2024 •

edited

wpk-nist-gov commented Feb 28, 2024 •

edited