cmake: make missing Python interpreter behaviour more explicit #33278
Conversation
Rather than failing, skip the deploy target (which was not explicitly requested) and explicitly warn the user.
|
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/33278. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
Adds new option (default on) to control the building of functional tests. In a future commit, allows us to FATAL_ERROR if no required Python interpreter can be found.
The functional tests have a hard Python requirement. Building them without being able to run them is unintuitive and error-prone for automated systems like CI, so instead raise a FATAL_ERROR if Python requirement is not satisfied. Note: on platforms without the minimum (currently 3.10) Python version installed, this will lead to the default configuration (`cmake -B build`) failing, instead requiring tests to be explicitly disabled (`cmake -B build -DBUILD_FUNCTIONAL_TESTS=0`).
Affected targets or usage already has explicit warnings or errors.
a0b6492 to
bad9092
Compare
|
Force-pushed to default |
|
ACK bad9092 We should not be building functional tests without the minimum python version satisfied, it seems fine to leave it as a warning for maintenance and deploy targets, since these will error later when attempting to build the target. |
| configure_file(config.ini.in config.ini USE_SOURCE_PERMISSIONS @ONLY) | ||
| endfunction() | ||
|
|
||
| if(NOT TARGET Python3::Interpreter) |
There was a problem hiding this comment.
Concept ACK
I am however unable to trigger the fatal error on Ubuntu 24.04.3 LTS with Python 3.9.23
Adding a few debug logs show that Python3::Interpreter target is set even if the minimum version is not met
There was a problem hiding this comment.
That's interesting. The https://cmake.org/cmake/help/v3.14/module/FindPython.html docs for Python::Interpreter state "Target defined if component Interpreter is found.", so I'm not sure why that's not failing for you.
Since the behaviour of relying on Python3::Interpreter is not changed in this PR, I suspect you won't get the "Minimum required Python not found." warning (at the very end of the configure step) that's removed in this PR, since that relies on the same mechanism? Correct?
I've implemented an alternative approach in https://github.com/stickies-v/bitcoin/commits/2025-09/cmake-fatal-python-found/ - does that throw the expected warning for you?
cc @hebasto since you recently updated this in bb9157d - is it important that we keep relying on checking the existence of the Python3::Interpreter target?
There was a problem hiding this comment.
Concept ACK
I am however unable to trigger the fatal error on
Ubuntu 24.04.3 LTSwithPython 3.9.23
I cannot reproduce the issue:
$ cat /etc/os-release | head -1
PRETTY_NAME="Ubuntu 24.04.3 LTS"
$ python3 --version
Python 3.9.23
$ cmake -B build
<snip>
-- Could NOT find Python3 (missing: Python3_EXECUTABLE Interpreter) (Required is at least version "3.10")
Reason given by package:
Interpreter: Wrong version for the interpreter "/home/hebasto/.pyenv/shims/python"
CMake Error at test/CMakeLists.txt:34 (message):
Minimum required Python not found. Consider configuring with
-DBUILD_FUNCTIONAL_TESTS=0.
-- Configuring incomplete, errors occurred!
If there is an issue with Python version detection, it should be reproducible on the master branch as well.
@BrandonOdiwuor could you please check the same scenario on the master branch?
|
Closing for lack of reviewer interest, it's not a critical change. |
We require Python 3.10 for multiple targets, and currently raise a general warning if it is missing, leading to:
This PR:
BUILD_FUNCTIONAL_TESTSoption (defaultON) and raisesFATAL_ERRORif Python is missing and we're building the functional testsconfigure_warningssystem (but requires further work to be completely removed)Behaviour changes:
cmake -B buildnow throws aFATAL_ERRORif minimum Python could not be found (overridden withcmake -B build -DBUILD_FUNCTIONAL_TESTS=0)deploytarget no longer created if minimum Python could not be foundAlternative to #31669 and partially to #33144 and #32865, partially addresses #31476.