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

Support venv detection on Windows with mingw Python #12544

Closed
lazka opened this issue Jun 28, 2024 · 5 comments · Fixed by #12545
Closed

Support venv detection on Windows with mingw Python #12544

lazka opened this issue Jun 28, 2024 · 5 comments · Fixed by #12545
Assignees
Labels
good first issue easy issue that is friendly to new contributor platform: mingw type: bug problem that needs to be addressed

Comments

@lazka
Copy link

lazka commented Jun 28, 2024

What's the problem this feature will solve?

Currently pytest checks platform depended paths in a directory to see if it might be a venv:

pytest/src/_pytest/main.py

Lines 370 to 387 in 0ed2d79

def _in_venv(path: Path) -> bool:
"""Attempt to detect if ``path`` is the root of a Virtual Environment by
checking for the existence of the appropriate activate script."""
bindir = path.joinpath("Scripts" if sys.platform.startswith("win") else "bin")
try:
if not bindir.is_dir():
return False
except OSError:
return False
activates = (
"activate",
"activate.csh",
"activate.fish",
"Activate",
"Activate.bat",
"Activate.ps1",
)
return any(fname.name in activates for fname in bindir.iterdir())

This works fine with the official CPython releases, but the mingw Python on Windows uses the Unix layout, which looks like this, despite being on Windows:

$ tree -L 1 .venv
.venv
├── bin
├── include
├── lib
└── pyvenv.cfg

This results in pytest trying to collect tests in venv files by default.

Describe the solution you'd like

I can see three solutions to this:

  1. Check for both bin/Scripts

  2. First check for the existence of "pyvenv.cfg", which should be in every venv on every platform

  3. Replace the current logic and just check for "pyvenv.cfg". "pyvenv.cfg" was introduced in Python 3.3 with the venv module, so it might be enough to assume it exists in every venv. Maybe I'm missing something though.

Note: I'm happy to create a PR for any of the above solutions.

lazka added a commit to msys2-contrib/distutils that referenced this issue Jun 28, 2024
…rted

distutils currently doesn't support pytest collection that doesn't
start at least at the distutils dir or above (and not distutils/tests)
since it requires the local distutils being imported before the tests are run,
otherwise the stdlib distutils takes precedence.

Adjust the pytest call to not pass a path to work around this.

Since pytest currently fails to skip collecting venvs with mingw python
(see pytest-dev/pytest#12544) move the venv
to /tmp instead.
@RonnyPfannschmidt
Copy link
Member

the initial code was added in b32cfc8 about 7 years ago when python 2.6 was still supported

so as far as i'm aware there is no reason not to switch to this much simpler test - a pr would be appreciated

@RonnyPfannschmidt RonnyPfannschmidt added type: bug problem that needs to be addressed platform: mingw good first issue easy issue that is friendly to new contributor labels Jun 28, 2024
@bluetech
Copy link
Member

Agreed, as far as I know, checking for pyvenv.cfg is sufficient.

@zachsnicker
Copy link
Contributor

Hello!
I'm new to contributing to pytest. Can I take this up?

@webknjaz
Copy link
Member

@zachsnicker I think it's non-controversial enough, I'll assign you. Make sure to include test coverage for the patch in your pull request.

@lazka
Copy link
Author

lazka commented Jun 28, 2024

Just to be safe I did some additional research, https://peps.python.org/pep-0405/ defines venvs and states "Thus, a Python virtual environment in its simplest form would consist of nothing more than a copy or symlink of the Python binary accompanied by a pyvenv.cfg file and a site-packages directory.". So it's safe to assume that every venv following the spec will contain a pyvenv.cfg.

For the external virtualenv package pyvenv.cfg was added in v20, released 4+ years ago, which is also in Ubuntu 20.04 for example.

So I think this is fine.

I also had a look at the code already, so I'm happy to review if wanted.

jaraco pushed a commit to pypa/distutils that referenced this issue Jun 28, 2024
…rted

distutils currently doesn't support pytest collection that doesn't
start at least at the distutils dir or above (and not distutils/tests)
since it requires the local distutils being imported before the tests are run,
otherwise the stdlib distutils takes precedence.

Adjust the pytest call to not pass a path to work around this.

Since pytest currently fails to skip collecting venvs with mingw python
(see pytest-dev/pytest#12544) move the venv
to /tmp instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor platform: mingw type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants