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

cmake: use python from venv if available #31411

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

willcl-ark
Copy link
Member

When running targets that depend on a python, such as:

cmake --build . --target test-security-check
cmake --build . --target check-symbols
cmake --build . --target check-security

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

  • First check if $VIRTUAL_ENV is set. If it is, use the python binary from this venv.
  • Fall back to system python otherwise.

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.
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 3, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31411.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #31233 (cmake: Improve robustness and usability by hebasto)

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.

@willcl-ark
Copy link
Member Author

cmake >= 3.17 does support a Python_FIND_VIRTUALENV variable, but this doesn't seem to support all venv providers (I use uv).

As far as I know, all venv providers set VIRTUAL_ENV, so I think this solution should be more universal.

@hebasto
Copy link
Member

hebasto commented Dec 3, 2024

cmake >= 3.17 does support a Python_FIND_VIRTUALENV variable, but this doesn't seem to support all venv providers (I use uv).

A quick note. This PR seems incompatible with #31233.

@willcl-ark
Copy link
Member Author

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

@hebasto
Copy link
Member

hebasto commented Dec 3, 2024

I use uv

This one?

@willcl-ark
Copy link
Member Author

Yes, and I highly recommend it!

@hebasto
Copy link
Member

hebasto commented Dec 3, 2024

... it can be tricky to configure cmake to use a python venv.

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 uv's Python on my Ubuntu 24.04:

$ python3 --version  # system's Python
Python 3.12.3
$ uv python install 3.10
Installed Python 3.10.15 in 9.33s
   cpython-3.10.15-linux-x86_64-gnu
$ uv venv --python 3.10
Using CPython 3.10.15
Creating virtual environment at: .venv
Activate with: source .venv/bin/activate
$ source .venv/bin/activate
$ cmake -B build --fresh
-- Found Python3: /home/hebasto/.venv/bin/python3 (found version "3.10.15") found components: Interpreter 
Python3_EXECUTABLE=/home/hebasto/.venv/bin/python3
-- Configuring done (0.2s)
-- Generating done (0.0s)
-- Build files have been written to: /home/hebasto/TEST_CMAKE_PYTHON/build

What did I miss?

@maflcko
Copy link
Member

maflcko commented Dec 4, 2024

I had the same thought. Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?

@fanquake
Copy link
Member

fanquake commented Dec 4, 2024

Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?

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

@hebasto
Copy link
Member

hebasto commented Dec 4, 2024

Shouldn't a venv "just work" out of the box, because they set the PATH appropriately?

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):

% python3 --version
Python 3.10.15
% which python3
/Users/xxx/.venv/bin/python3
% cmake -B build
-- Found Python3: /Users/xxx/.venv/bin/python3.10 (found version "3.10.15") found components: Interpreter
<snip>

CMake picks the Python from .venv while leaving system's and Homebrew's ones.

Any specific steps are required to reproduce your issue?

@fanquake
Copy link
Member

fanquake commented Dec 4, 2024

CMake picks the Python from .venv

I'm not using a .venv. It's Python installed with pyenv.

@willcl-ark
Copy link
Member Author

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 cmake check to (silently) skip that version during configure and fallback to a "system" version (3.13).

This was then combined with me manually exporting PYTHONPATH in this dir (to make Pyright perform better in nvim), which was causing a right mismatch of modules and python versions; with cmake launching files using one python and then importing modules from another env completely.

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.

@hebasto
Copy link
Member

hebasto commented Dec 4, 2024

CMake picks the Python from .venv

I'm not using a .venv. It's Python installed with pyenv.

My apologies.

Adding -DPython3_FIND_FRAMEWORK=LAST should work.

@fanquake
Copy link
Member

fanquake commented Dec 4, 2024

Adding -DPython3_FIND_FRAMEWORK=LAST should work.

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?

@hebasto
Copy link
Member

hebasto commented Dec 4, 2024

Adding -DPython3_FIND_FRAMEWORK=LAST should work.

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:

  1. From the FindPython3 module docs:

On macOS the Python3_FIND_FRAMEWORK variable determine the order of preference between Apple-style and unix-style package components. This variable can take same values as CMAKE_FIND_FRAMEWORK variable.

Note: Value ONLY is not supported so FIRST will be used instead.

If Python3_FIND_FRAMEWORK is not defined, CMAKE_FIND_FRAMEWORK variable will be used, if any.

  1. From the CMAKE_FIND_FRAMEWORK variable docs:

This variable affects how find_* commands choose between macOS Frameworks and unix-style package components.

@fanquake
Copy link
Member

fanquake commented Dec 4, 2024

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 pyenv in our documentation, so it makes even less sense that this workflow would be broken, or require workarounds that we haven't documented.

@maflcko
Copy link
Member

maflcko commented Dec 4, 2024

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 python3.NN ./bld-cmake/test/functional/test_runner.py.

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 Python3_FIND_FRAMEWORK to one value or the other on macOS is helpful, I wont' mind.

@fanquake
Copy link
Member

fanquake commented Dec 4, 2024

Does any of this matter in practise for running the tests?

If CMake picks a different Python to the one you expect it to use, which is not the one you've pip installed any requirements into, i.e pyzmq, aren't you now (likely unknowningly) skipping tests that otherwise should be run.

@willcl-ark
Copy link
Member Author

OK the issue here is that most venv providers export the VIRTUAL_ENV variable. I say most, because this appears to specifically exclude pyenv, for some reason.

From the cmake docs:

Python3_FIND_VIRTUALENV

Added in version 3.15.

This variable defines the handling of virtual environments managed by virtualenv or conda. It is meaningful only when a virtual environment is active (i.e. the activate script has been evaluated). In this case, it takes precedence over Python3_FIND_REGISTRY and CMAKE_FIND_FRAMEWORK variables. The Python3_FIND_VIRTUALENV variable can be set to one of the following:

    FIRST: The virtual environment is used before any other standard paths to look-up for the interpreter. This is the default.

    ONLY: Only the virtual environment is used to look-up for the interpreter.

    STANDARD: The virtual environment is not used to look-up for the interpreter but environment variable PATH is always considered. In this case, variable Python3_FIND_REGISTRY (Windows) or CMAKE_FIND_FRAMEWORK (macOS) can be set with value LAST or NEVER to select preferably the interpreter from the virtual environment.

As FIRST is the default for this, all venv providers that set VIRTUAL_ENV should just work. I believe this is the case, as when I updated my venv to the required minimum version it began working again, without the need for this patch.

Therefore the only remaining issue here IMO is how to handle pyenv. According to my research using pyenv-virtualenv as a plugin link sets this variable. But that doesn't seem particularly nice. Alternatively start writing custom activation or bashrc scripts to set VIRTUAL_ENV, but that also is not appealing.

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 uv, and we update our docs accordingly.

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

Successfully merging this pull request may close these issues.

5 participants