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

feat: added venv_location option to specify virtualenv location #785

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wpk-nist-gov
Copy link

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
Copy link
Collaborator

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
Copy link
Author

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

@cjolowicz
Copy link
Collaborator

Does this allow arbitrary path traversal?

@wpk-nist-gov
Copy link
Author

wpk-nist-gov commented Feb 27, 2024

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

wpk added 2 commits February 27, 2024 15:05
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
nox/registry.py Outdated
@@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should support Path too, I think.

Copy link
Author

Choose a reason for hiding this comment

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

Totally agree. Will update to os.PathLike

@henryiii
Copy link
Collaborator

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
Copy link
Author

wpk-nist-gov commented Feb 28, 2024

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
Copy link
Collaborator

cjolowicz commented Feb 28, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

henryiii commented Feb 28, 2024

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
Copy link
Author

wpk-nist-gov commented Feb 28, 2024

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
Copy link
Collaborator

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

@wpk-nist-gov
Copy link
Author

wpk-nist-gov commented Feb 28, 2024

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.

@wpk-nist-gov wpk-nist-gov reopened this Feb 28, 2024
@cjolowicz
Copy link
Collaborator

cjolowicz commented Feb 28, 2024

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
Copy link
Collaborator

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
Copy link
Author

wpk-nist-gov commented Feb 28, 2024

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
Copy link
Collaborator

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
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

3 participants