Guard manpage generation by reading bitcoin-build-config.h and exiting if HAVE_SYSTEM, ENABLE_WALLET, or WITH_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 the SKIP_BUILD_OPTIONS_CHECK environment variable 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.
| Type | Reviewers |
|---|---|
| Stale ACK | maflcko |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
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.
Yeah, it seems better to just read one file instead of two. I haven’t tested this, but the initial code looks plausible
lgtm ACK 2a226ae02c1556a8626da823ccde3ba3c89e9a73
lgtm ACK 19453292cff54e768b5138236bfffb5706f54f36
I haven’t tested this, but the initial code looks plausible
HAVE_SYSTEM is never present in CMakeCache.txt, so I don’t see how this could work. Can you at least test your changes locally before pushing them to GitHub.
@151henry151 Is this LLM generated?
There is little value in doing that, because it is just adding extra hops and wastes time.
At least for this software project, the pull request author is expected to have a rock solid understanding of their own changes they are proposing. Failure of the pull request author to understand their own changes will lead to subtle missed bugs, or tedious review cycles to uncover and fix them.
For example, when I asked if the ZMQ build option can be parsed from the header (https://github.com/bitcoin/bitcoin/pull/33828#issuecomment-3774186724), you should have replied that it isn’t present in the header, and thus the check will fail. Ideally with exact steps to reproduce, or the failure output.
Given that HAVE_SYSTEM is never in the cmake cache, no version of the code ever pushed here will pass.
This means you never even tried to run any of this yourself, to confirm that it works?
This looks like purely LLM generated ai slop to me, just like #33254 (comment) and #33926#pullrequestreview-3513898487
I think I understand why my local testing failed to catch this; I was explicitly setting HAVE_SYSTEM=1 and HAVE_SYSTEM=0 at the command line which puts them in the cmake cache.
When I’m back at my laptop I’ll take a closer look at this. I would guess that it’s going to be preferable to read the header file, but we’ll have to add the ZMQ define to the build header. [edit: if we want to avoid reading two separate files and have a single source of information for this check]
And yes, I wrote and understand the code of this pull request and it was not LLM generated. I can understand your questioning that in the context of my misunderstandings and mistakes here, but would appreciate it if you will continue to take the time to review my work; I’m learning.
Thanks
I’m on lunch break from work now, so have a moment at my computer to share for reference how I was testing this locally:
0
1BUILDDIR="${BUILDDIR:-build}"
2SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
3cd "$SCRIPT_DIR"
4
5echo "Test 1: All features ON"
6cd "$BUILDDIR"
7cmake .. -DHAVE_SYSTEM=1 -DWITH_ZMQ=ON -DENABLE_WALLET=ON >/dev/null 2>&1
8cd "$SCRIPT_DIR"
9
10output=$(python3 contrib/devtools/gen-manpages.py 2>&1) || true
11if echo "$output" | grep -q "ERROR"; then
12 echo "FAIL: Script failed with features ON"
13 echo "$output" | grep -A5 "ERROR"
14 exit 1
15fi
16echo "PASS: Script passed with all features ON"
17
18echo ""
19echo "Test 2: All features OFF"
20cd "$BUILDDIR"
21cmake .. -DHAVE_SYSTEM=0 -DWITH_ZMQ=OFF -DENABLE_WALLET=OFF >/dev/null 2>&1
22cd "$SCRIPT_DIR"
23
24output=$(python3 contrib/devtools/gen-manpages.py 2>&1) || true
25if echo "$output" | grep -q "HAVE_SYSTEM\|ENABLE_WALLET\|WITH_ZMQ"; then
26 echo "PASS: Script detected missing features"
27else
28 echo "FAIL: Script should have detected missing features"
29 exit 1
30fi
31
32echo ""
33echo "All tests passed"
I will continue working on this when I get home this evening. Thanks again for your time and effort, I know reviewing slop is taking up a lot of time for bitcoin maintainers and I want to be of value to the project, not a drag on it.
@maflcko @fanquake I believe bd7c049 addresses the issue. The script now reads all three features (HAVE_SYSTEM, ENABLE_WALLET, and WITH_ZMQ) from bitcoin-build-config.h as the single source of truth, which resolves the problem that HAVE_SYSTEM is not typically present in CMakeCache.txt. I added #cmakedefine WITH_ZMQ 1 to the header template, which references the existing CMake option WITH_ZMQ (defined in CMakeLists.txt) and conditionally generates #define WITH_ZMQ 1 in the output header when ZMQ is enabled. I’ve tested locally across all combinations of these three features enabled/disabled, and the script correctly detects missing features in each case.
Thanks for any time you might take to review my work.
This looks like purely LLM generated ai slop to me, just like #33254 (comment) and #33926 (review)
Regarding #33254, that was my very first attempt at contributing to Bitcoin and I acknowledge it was not of any value.
Regarding #33926, I think that the most recent commit warrants reopening the pull request and that it is ready for review. Neither of them were “purely LLM generated”, and I think that if you look more closely at the current state of #33926 you may even consider reopening it. I did use AI to help me comprehend the issue and to help me research how to go about resolving the issue, and to debug when I made errors, but I take full responsibility for the code as it stands and it is my work, not purely LLM generated ai slop.
Thanks for any consideration.