Guard manpage generation by reading bitcoin-build-config.h and exiting if HAVE_SYSTEM, ENABLE_WALLET, or ENABLE_ZMQ are off, and provide a short hint so contributors rebuild with the right flags instead of committing incomplete manpages.
Fixes #17506
Override by using –skip-build-options-check if you really need a custom-fit manpage (see bitcoin/bitcoin#29457 (comment)).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33828.
See the guideline for information on the review process. A summary of reviews will appear here.
No conflicts as of last run.
Thanks for taking a look, fanquake.
I noticed #33085 had gone quiet for a little over three weeks with the review concerns still open, so I put this together.
In #33085 the script pulls component flags from test/config.ini (adding checks for CLI, chainstate, fuzz, USDT tracepoints, etc.), but that file only exists when tests are configured and most of those toggles don’t affect the manpages. This branch reads CMakeCache.txt, the authoritative record of the configuration, and only verifies the interfaces that actually change the generated help text.
#33085 adds a new --skip-build-options-check flag, whereas this keeps using the existing SKIP_BUILD_OPTIONS_CHECK environment override so the command-line surface stays the same. The error messaging here points directly at the missing CMake options and explains how to bypass the guard, which lines up with the release-process docs, and the helper functions make the new logic reusable instead of nesting more ad-hoc parsing in the main flow.
I appreciate the work Brandon already put in to #33085 and I looked his code over thoroughly before tackling this, but I thought this solution was more elegant and would be preferred. Wanting to follow proper etiquette, perhaps I should have commented my suggestions on his PR instead of opening my own?
This duplicates #33085. If the author doesn’t follow up there shortly, we could reopen here.
It’s been a few weeks, would it be appropriate to reopen here? I did try to reach out to @BrandonOdiwuor on the comments of #33085 and haven’t heard back from him yet.
11 import argparse
12
13+
14+def _read_cmake_cache(cache_path):
15+ options = {}
16+ with open(cache_path, encoding="utf-8") as cache:
0 with open(cache_path) as cache:
They can now be dropped, after https://github.com/bitcoin/bitcoin/pull/33702
76+cmake_cache_path = os.path.join(builddir, 'CMakeCache.txt')
77+if not os.path.exists(cmake_cache_path):
78+ print(f'ERROR: {cmake_cache_path} not found. Run this script against a CMake build directory.', file=sys.stderr)
79+ sys.exit(1)
80+
81+if _env_truthy("SKIP_BUILD_OPTIONS_CHECK"):
--ignore-warnings. Or just remove this option for now.
25+ options[key] = value
26+ return options
27+
28+
29+def _cmake_truthy(value):
30+ return value.upper() in {"ON", "TRUE", "YES"} or value == "1"
#define HAVE_SYSTEM 1 exists, or not.
@maflcko you said –ignore-warnings, but I used –skip-build-options-check since we’re skipping checks, not ignoring warnings. Should I change it to –ignore-warnings?
I switched HAVE_SYSTEM and ENABLE_WALLET to read from bitcoin-build-config.h. WITH_ZMQ isn’t in the header, so I’m still parsing CMakeCache.txt for it. Is that expected?
WITH_ZMQ isn’t in the header, so I’m still parsing CMakeCache.txt for it. Is that expected?
It is called ENABLE_ZMQ. You can read the header, to see the contents of it.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21181549467/job/60924930617
LLM reason (✨ experimental): Python linting failed (ruff) due to trailing/blank-line whitespace issues and other fixable ruff errors in the Python code.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
WITH_ZMQ isn’t in the header, so I’m still parsing CMakeCache.txt for it. Is that expected?
It is called
ENABLE_ZMQ. You can read the header, to see the contents of it.
Sorry, i was wrong. This is actually a compile definition. So I guess either the cmake parsing is required, or it has to be made a define in the build header.
… either the cmake parsing is required, or it has to be made a define in the build header.
So either we need to keep parsing CMakeCache.txt for WITH_ZMQ, or ENABLE_ZMQ needs to be added to the build header. Do you have a preference between these two approaches? I would lean towards adding it to the build header, since it’s simpler and more direct than parsing the CMake cache format. I’ve implemented that in f9ad5f0
Also, this doesn’t compile (please test locally before pushing)
Edit: As for a preference, I’d say it is probably easiest in this context to just read the cmake cache file.
I believe the current commit 442346a follows your suggestions, using header checks for HAVE_SYSTEM and ENABLE_WALLET, and still checking CMakeCache.txt for WITH_ZMQ.
Frankly, I prefer the code of 2a226ae which intentionally used the existing SKIP_BUILD_OPTIONS_CHECK env var to keep the CLI unchanged, and checked all three interfaces against CMakeCache.txt as the single source of truth. It just seems more elegant to me.
Can you clarify the suggestion to use a single argument like –ignore-warnings? I don’t see an existing –ignore-warnings option in the script. I added the argument –skip-build-options-check, but wasn’t sure if maybe the –ignore-warnings option exists elsewhere and should also skip build options checks? Thanks again for all the feedback.