fix(windows): running tests in a virtual environment #2209
Conversation
psutil/tests/__init__.py
Outdated
| @@ -131,6 +131,8 @@ | |||
| COVERAGE = 'COVERAGE_RUN' in os.environ | |||
| # are we a 64 bit process? | |||
| IS_64BIT = sys.maxsize > 2 ** 32 | |||
| # PEP 405 virtual environment | |||
| IS_VIRTUALENV = sys.prefix != getattr(sys, "base_prefix", sys.prefix) | |||
There was a problem hiding this comment.
I'm afraid this code won't work on Python 2.7 (base_prefix was added in 3.3).
A similar check is present in the Makefile, but we do something different (I'm not sure this works on 3.X though):
Lines 40 to 41 in 2974941
I'm surprised detecting a venv is so difficult. FWIW, I bumped into this:
https://stackoverflow.com/a/1883251
...which seems to work on both Python 2 and 3.
There was a problem hiding this comment.
We only need to detect this properly on python 3.7+ on windows.
If python 2.7 does not respect PEP 405, we won't detect it's running in a virtual environment but the test outcome will be correct nonetheless.
There was a problem hiding this comment.
I removed the variable IS_VIRTUALENV to lessen confusion that this could trigger.
| else: | ||
| exe = os.path.realpath(sys.executable) | ||
| assert os.path.exists(exe), exe | ||
| return exe |
There was a problem hiding this comment.
Mmm no, you're removing too much. All this part was gradually built over the years because relying on sys.executable alone was not enough.
I suggest the approach I recommended here:
#2209 (comment)
...meaning leaving the existing code as-is, and integrate the new logic on top of it.
There was a problem hiding this comment.
All this part was gradually built over the years
At least the part mentioning GHA can be removed (it either passes or fails CI).
For the rest, it seems to be coming from mostly one commit d108baf, and without an issue or PR discussion around that, it seems mostly a preemptive action to something rather than fixing something, or maybe, it was fixing something at the time which might not be required anymore as the tests would suggest. Do you have any reference other than this commit ?
If keeping this, we might need to reintroduce 2 pythons for test_misc if you're not fine with the usage of sys.executable I'm about to push following #2211.
There was a problem hiding this comment.
For the rest, it seems to be coming from mostly one commit d108baf, and without an issue or PR discussion around that, it seems mostly a preemptive action to something rather than fixing something, or maybe, it was fixing something at the time which might not be required anymore as the tests would suggest. Do you have any reference other than this commit ?
I did that because on my virtualized OSX box the python exe was a symlink, and that created issues with tests using PYTHON_EXE (most likely they complained about missing path). I haven't turned on VirtualBox in a while, but I think the OSX workaround is still relevant today.
Note: I think you run tests via cibuildwheel and/or virtual env. I use the Makefile on all platforms (including windows via winmake.py), so some workarounds that you see in the code are because of my setup / way of doing things.
At least the part mentioning GHA can be removed (it either passes or fails CI).
If it doesn't break tests then sure, but does it? I can't remember why I added logic for PYPI and FREEBSD specifically, but most likely it was because some tests failed on GITHUB.
The part about PYPY is not used because right now we don't have a PYPY build configured.
Anyway, I'm fine with removing the GITHUB_ACTIONS part as long as it doesn't break tests.
There was a problem hiding this comment.
Thanks for the background, I will reintroduce that.
Note: I think you run tests via cibuildwheel and/or virtual env. I use the Makefile on all platforms (including windows via winmake.py), so some workarounds that you see in the code are because of my setup / way of doing things.
Indeed, I always use virtual environment, be it through cibuildwheel, nox, tox, manual setup, ...
I do really think that any development workflow shall not interfere with the system/user installation but that's just my opinion.
There was a problem hiding this comment.
I do really think that any development workflow shall not interfere with the system/user installation but that's just my opinion.
I certainly agree. It's an old bad habit I have. FWIW, also the Makefile can be used with venv:
make install test PYTHON=/path/to/your/venv
There was a problem hiding this comment.
The patch in this PR also works well in the reproduced dev workflow: https://github.com/mayeut/psutil/actions/runs/4465692219/jobs/7843003771
As mentioned in comments & in the code, PYTHON_BASE_EXE / sys._base_executable / or even os.path.realpath(sys._base_executable) won't have psutil installed, by design (it's the global python interpreter, not the venv) & that's why we need 2. In this PR, only pyrun reinjects the __PYVENV_LAUNCHER__ to get psutil importable when using PYTHON_BASE_EXE.
If we don't use the global python interpreter, we can't count processes (or we'd need a specific workaround) or rely on the pid of the launched interpreter to count connections. If we use the global interpreter, we can't import psutil.
My conclusion, getting 2 is the easiest, otherwise, we'd need 1 + an environment variable or rework the failing tests but I feel the 2 interpreters is easier. We can't get away from those issues just by modifying PYTHON_EXE.
There was a problem hiding this comment.
My conclusion, getting 2 is the easiest, otherwise, we'd need 1 + an environment variable.
This seems more viable to me. My point is the user of these constants should not think "which one should I use?". It's an implementation detail that is difficult to understand (at least I still don't fully understand it), and which only affects Windows + venv running on GITHUB_CI.
I forgot how your previous changes of 1 PYTHON_EXE + env var looked like, but I would try to rehash that approach instead and see how we can arrange it.
There was a problem hiding this comment.
I forgot how your previous changes of 1 PYTHON_EXE + env var looked like, but I would try to rehash that approach instead and see how we can arrange it.
Maybe you can do it in a separate PR so that we stop switching between approaches in the same PR.
There was a problem hiding this comment.
I forgot how your previous changes of 1 PYTHON_EXE + env var looked like, but I would try to rehash that approach instead and see how we can arrange it.
I've never pushed this one (or even did it in my fork). I will try to open a new PR with this approach this week.
There was a problem hiding this comment.
I opened 2 other PR with alternative implementations.
The 3 PRs pass the "dev like workflow" on my fork (side-note, it seems the just released pyperf 2.6.0 breaks that workflow, at least the way I implemented it in GHA, so I pinned that to 2.5.0 in my tests of the dev workflow)
On windows, starting with python 3.7, virtual environments use a venvlauncher startup process This does not play well when counting spawned processes or when relying on the pid of the spawned process to do some checks e.g. connection check per pid This commit detects this situation and uses the base python executable to spawn processes when required. Signed-off-by: mayeut <mayeut@users.noreply.github.com>
Summary
Description
On windows, starting with python 3.7, virtual environments use a venvlauncher startup process This does not play well when counting spawned processes or when relying on the pid of the spawned process to do some checks e.g. connection check per pid
This commit detects this situation and uses the base python executable to spawn processes when required.