Prevent using system Python despite venv check#407
Prevent using system Python despite venv check#407Mikolaj-A-Kowalski wants to merge 1 commit intoCambridge-ICCS:mainfrom
Conversation
Mikolaj-A-Kowalski
added
bug
Previously a system Python could be used despite the protection if one: 1) Tried to run the `cmake` without venv 2) Activated venv after seeing the error message 3) Re-run the cmake without cleaning cache Now check for venv is done before any search for Python so nothing will be cached. Change VIRTUALENV option to ONLY to prevent using system python in any case.
Mikolaj-A-Kowalski
force-pushed
the
safe-virtualenv-error
branch
from
5ed8979 to
ab8dfbb
Compare
| # environment variable exists | ||
| # Must be done before `find_package` in to prevent system Python getting | ||
| # cached by CMake | ||
| if(NOT DEFINED ENV{VIRTUAL_ENV}) |
There was a problem hiding this comment.
We should probably make it work with conda environment as well?
But for that we will need to increase minimum CMake version 3.17 since this is when FindPython FIND_VIRTUALENV got support for conda.
There was a problem hiding this comment.
I'd be fine with bumping the minimum version to 3.17. We could also plausibly increase the maximum version given how many more releases there have been since 3.31 https://cmake.org/cmake/help/latest/release/index.html
There was a problem hiding this comment.
I think that's fine, but can we ideally have a separate commit and definitely a Changelog entry for it please.
Looking at the breaking changes for 4.0 we should be unaffected.
|
I was thinking that perhaps the cleanest and safest check for the virtual environment would be to set the |
joewallwork
left a comment
joewallwork
left a comment
There was a problem hiding this comment.
This makes sense, thanks @Mikolaj-A-Kowalski! Please could you tweak the CMake versions as mentioned?
| # environment variable exists | ||
| # Must be done before `find_package` in to prevent system Python getting | ||
| # cached by CMake | ||
| if(NOT DEFINED ENV{VIRTUAL_ENV}) |
There was a problem hiding this comment.
I'd be fine with bumping the minimum version to 3.17. We could also plausibly increase the maximum version given how many more releases there have been since 3.31 https://cmake.org/cmake/help/latest/release/index.html
jatkinson1000
left a comment
jatkinson1000
left a comment
There was a problem hiding this comment.
Yes, looks sensible to me.
Just ensure changelog is updated with the CMake version bump please.
| # environment variable exists | ||
| # Must be done before `find_package` in to prevent system Python getting | ||
| # cached by CMake | ||
| if(NOT DEFINED ENV{VIRTUAL_ENV}) |
There was a problem hiding this comment.
I think that's fine, but can we ideally have a separate commit and definitely a Changelog entry for it please.
Looking at the breaking changes for 4.0 we should be unaffected.
|
@Mikolaj-A-Kowalski just looked at the conflict and it looks like a conda check has been added in which might circumvent the need to bump and simplify your PR if you want to rebase on main and reassess. |
|
@Mikolaj-A-Kowalski any chance you might have a moment to look at this as a small hanging PR. |
3 participants
Previously a system Python could be used despite the protection if one:
cmakewithout venvNow check for venv is done before any search for Python so nothing will be cached.
I have also used this opportunity to change the
Python_FIND_VIRTUALENVtoONLYwhich is I believe the behaviour we want (never pick-up the system interpreter in any case?)