cmake: Introduce WITH_PYTHON build option
#31669
Conversation
|
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/31669. 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. |
751d491 to
796c36a
Compare
|
Not sure. Why does this need another build option? Given that this is just for working around silent CMake warnings in the CI, couldn't this just be fixed by using explicit It also doesn't solve #31476 generically, because every time a similar case comes up, we'll have to add even more code and/or options. |
|
Yeah, I suggested this for reasons other than the CI fix, because it could be useful outside the CI as well. But no strong opinion. |
796c36a to
b42f709
Compare
|
Rebased due to the conflict with the merged #31651. |
|
NACK - based on the reasoning above. |
What "explicit |
b42f709 to
22659a3
Compare
22659a3 to
5155dbd
Compare
|
lgtm ACK 5155dbd My nit about the warning isn't a blocker. |
By default, the build system generation will fail on systems where the minimum required Python version is unavailable. This option allows users to explicitly disable the Python requirement.
There was a problem hiding this comment.
As I mentioned above, adding more build options/compexity, and forcing builders to opt out of things they don't care about, does not seem like a good, nor generic approach to fixing #31476. Especially given this is essentially a CI fix, to work around the fact that CMake doesn't have a way to turn warnings into errors?
I am not sure the current approach of silently skipping tests (with just a warning during configure, not when running the tests) is the right approach. A third alternative would be to delay the error to when they run the tests. A fourth alternative would be to just call the two python tests from the functional test runner somehow and remove all this code here? |
FWIW, something similar has been implemented in #31233. |
There was a problem hiding this comment.
Concept ACK 40d9359
(if we want to keep python check in CMAKE)
PR Adds build option to ignore / not fail on missing python. When python is missing or below required version the configuration will error out. This removes failing silently on python.
This PR will partial solve issue #31476 (strictly the Python issue)
Alternative thoughts:
Should CMAKE even test for python ? If you see CMAKE as configuration for the BUILD step and we do not use python in building should we even test for python ?
|
NACK I am with @fanquake here. The complexity increases exponentially with the number of build options. |
This was suggested in #31476 (comment):
This PR updates the build system to fail by default on systems where the minimum required Python version is unavailable. It introduces an option that allows users to explicitly disable the Python requirement.