-
Notifications
You must be signed in to change notification settings - Fork 36.5k
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
cmake: use python from venv if available #31411
base: master
Are you sure you want to change the base?
Conversation
First check if $VIRTUAL_ENV is set. If it is, use the python binary from this venv. Fall back to system python otherwise. This helps run various targets which use python, when system python is not being used.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31411. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
As far as I know, all venv providers set |
A quick note. This PR seems incompatible with #31233. |
Thanks hebasto, I had not seen this PR. Will compare/review. It has come to my attention that having these checks complete in a dev environment is not that useful anyway, so perhaps "making things easier for me to run locally" is not a great motivation for this change in any case... |
This one? |
Yes, and I highly recommend it! |
I've read https://github.com/astral-sh/uv/blob/main/README.md#python-management and used the following script for simplicity: cmake_minimum_required(VERSION 3.22)
project(test_py LANGUAGES NONE)
find_package(Python3 COMPONENTS Interpreter)
message("Python3_EXECUTABLE=${Python3_EXECUTABLE}") I have no problems with finding
What did I miss? |
I had the same thought. Shouldn't a venv "just work" out of the box, because they set the |
Apparently not. Here's another case where CMake just picks some other Python: # python --version
Python 3.13.1
# python3 --version
Python 3.13.1
# which python
/Users/xxx/.pyenv/shims/python
# cmake -B build
<snip>
-- Found Python3: /opt/homebrew/Frameworks/Python.framework/Versions/3.13/bin/python3.13 (found suitable version "3.13.0", minimum required is "3.10") found components: Interpreter |
I cannot reproduce this on my Sequoia 15.1.1 (Intel):
CMake picks the Python from Any specific steps are required to reproduce your issue? |
I'm not using a |
Thanks @hebasto for the double-check. It has helped me work out what was going wrong for me (I think)... Looks like I had not bumped my python version in the venv since #30527 so I was still running 3.9 in my venv. This caused the This was then combined with me manually exporting So this seems to have been my error indeed. I can confirm that using a python 3.10 venv fixes the issue for me. I think I can probably close this now, and perhaps move to an issue while we figure out what cmake is doing with @fanquake's setup? As this PR does not apparently fix that case, so seems largely useless now. NB I would note that it could be nice to give a warning when a venv is detected but not used (as it doesn't meet requirements), but seems like a nice-to-have, rather than something essential. |
My apologies. Adding |
Right, so CMake just picks a different version here for some reason? Is this a bug? Do we need to document this, so it's clear to users/developers that sometimes CMake will just ignore the shell/in-use Python version? |
The behaviour is extensively documented:
|
Sure, but none of that means anything to a (new) developer, who doesn't care about the intricacies of the build system. It's just unintuituve that CMake would pick some version of Python, other than the one their shell/sytem is (globally) configured to use. We even suggest using |
Does any of this matter in practise for running the tests? The tests should be passing for any supported python version, as long as it is above the minimum, which is what cmake already checks for. Also, the tests can be run with a python version explicitly, like Though, even for writing tests in python, the version should mostly be irrelevant now that python3 has matured for the commonly used batteries. Historically, one could accidentally write python3.6 code, when the minimum supported was 3.5, however I don't think this is relevant today when new python features aren't applicable to the tests in this repo anyway. In any case, if people think that forcing |
If CMake picks a different Python to the one you expect it to use, which is not the one you've |
OK the issue here is that most venv providers export the From the cmake docs:
As Therefore the only remaining issue here IMO is how to handle I don't think it can be solved in a cmake context without custom cmake find package logic, which I think we should avoid. Therefore my preference would be to recommend that folks move to |
When running targets that depend on a python, such as:
... it can be tricky to configure cmake to use a python venv. This change checks for the
$VIRTUAL_ENV
env var and uses python from there using the following logic: