doc: Fix gen-manpages to check build options

BrandonOdiwuor commented at 12:18 pm on February 20, 2024: contributor

Fixes #17506

gen-manapages.py should check enabled build options when generating docs

DrahtBot commented at 12:19 pm on February 20, 2024: contributor

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

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:

#31161 (cmake: Set top-level target output locations by hebasto)
#30986 (contrib: skip missing binaries in gen-manpages by andremralves)
#30437 (multiprocess: add bitcoin-mine test program by ryanofsky)

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.

DrahtBot added the label Docs on Feb 20, 2024

in contrib/devtools/gen-manpages.py:90 in 4c3062a2ee outdated

85+    config.read(conffile)
86+
87+    docs_to_exlude = []
88+    if (binary == 'bitcoind' or binary == 'bitcoin-qt') and not config['components'].getboolean('HAVE_SYSTEM'):
89+        docs_to_exlude += ['shutdownnotify', 'alertnotify', 'startupnotify', 'blocknotify']
90+        return parse_and_remove_subsections_from_doc(document, docs_to_exlude)

maflcko commented at 12:34 pm on February 20, 2024:

My understanding would be that the check returns an error to recommend to build with the component, when a component is missing, no?

DrahtBot added the label CI failed on Feb 20, 2024

DrahtBot commented at 2:07 pm on February 20, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/21766942690

luke-jr commented at 10:28 pm on February 20, 2024: member

Probably should be a way to override this, in case someone actually wants a custom-fit manpage?

BrandonOdiwuor force-pushed on Feb 21, 2024

in contrib/devtools/gen-manpages.py:81 in 1e1ab9054c outdated

76+
77+    if binary == 'bitcoin-wallet' and not config['components'].getboolean('ENABLE_WALLET'):
78+        print(error_message.format(component="Wallet functionality", file=file))
79+        sys.exit(1)
80+
81+    if (binary == 'bitcoind' or binary == 'bitcoin-qt') and not config['components'].getboolean('USE_UPNP'):

maflcko commented at 11:27 am on February 21, 2024:

what is the point of re-doing the check for every binary?

BrandonOdiwuor commented at 11:59 am on February 21, 2024:

fixed

BrandonOdiwuor force-pushed on Feb 21, 2024

BrandonOdiwuor marked this as ready for review on Feb 21, 2024

DrahtBot removed the label CI failed on Feb 21, 2024

BrandonOdiwuor commented at 1:21 pm on February 21, 2024: contributor

@luke-jr you mean away to override the changes or the help2man generation?

in contrib/devtools/gen-manpages.py:68 in 428b921a95 outdated

63+conffile = os.path.join(builddir, 'test/config.ini')
64+config.read(conffile)
65+
66+error_message = "Aborting generating manpages…\nError: {component} support is not enabled.\nPlease enable it and try again."
67+
68+if not config['components'].getboolean('HAVE_SYSTEM'):

fanquake commented at 1:57 pm on February 26, 2024:

This seems arbitrary and prone to breakage, giving the hardcoding of specific components. It also doesn’t cover all of the relevant components in any case, i.e at least decl_fork and natpmp (which both effect manpage output) are missing any sort of check.

maflcko commented at 7:21 am on April 24, 2024:

This seems arbitrary and prone to breakage, giving the hardcoding of specific components.

I don’t think there is a better alternative, is there?

fanquake commented at 11:38 am on April 24, 2024:

Not sure. Although I think any changes here can wait until after CMake, as this script will likely need to be (partially) rewritten after that change in any case.

maflcko commented at 12:12 pm on April 24, 2024:

changes here can wait until after CMake, as this script will likely need to be (partially) rewritten after that change in any case.

Can you explain this? config ini should be unrelated to the build system and shouldn’t be affected by it.

fanquake commented at 12:18 pm on April 24, 2024:

This is hardcoding component names / variables, and they are likely to change during the switchover.

DrahtBot added the label CI failed on Jul 13, 2024

DrahtBot removed the label CI failed on Jul 18, 2024

achow101 requested review from laanwj on Oct 15, 2024

achow101 commented at 2:36 pm on October 15, 2024: member

Are you still working on this?

laanwj commented at 2:46 pm on October 15, 2024: member

Thanks for working on this issue, however this needs update after the cmake change.. This is the new contents of test/config.ini:

 0[components]
 1# Which components are enabled. These are commented out by `configure` if they were disabled when running config.
 2ENABLE_WALLET=true
 3USE_SQLITE=true
 4#USE_BDB=true
 5ENABLE_CLI=true
 6ENABLE_BITCOIN_UTIL=true
 7ENABLE_WALLET_TOOL=true
 8ENABLE_BITCOIND=true
 9#ENABLE_FUZZ_BINARY=true
10#ENABLE_ZMQ=true
11ENABLE_EXTERNAL_SIGNER=true
12#ENABLE_USDT_TRACEPOINTS=true

i’m not sure there is a way around hardcoding component names, as they have different effects on options we can’t automatically detect here, but we may want to be more thorough: explicitly include lists of components that must be enabled, and components that we don’t care about. If any unknown components appear, also error out so that can be resolved first.

BrandonOdiwuor force-pushed on Nov 11, 2024

BrandonOdiwuor requested review from maflcko on Nov 11, 2024

BrandonOdiwuor requested review from fanquake on Nov 11, 2024

BrandonOdiwuor commented at 11:31 am on November 11, 2024: contributor

@laanwj I updated the PR to include the suggestions

in contrib/devtools/gen-manpages.py:84 in 7651dc3a4a outdated

69+    'ENABLE_CLI': 'CLI',
70+    'ENABLE_BITCOIN_UTIL': 'Bitcoin utility',
71+    'ENABLE_WALLET_TOOL': 'Wallet tool',
72+    'ENABLE_BITCOIND': 'Bitcoind',
73+    'ENABLE_EXTERNAL_SIGNER': 'External Signer',
74+}

maflcko commented at 11:37 am on November 11, 2024:

Missing HAVE_SYSTEM?

in contrib/devtools/gen-manpages.py:91 in 7651dc3a4a outdated

76+enabled_components = {
77+    'USE_BDB': 'Berkeley DB',
78+    'ENABLE_FUZZ_BINARY': 'Fuzz testing binary',
79+    'ENABLE_ZMQ': 'ZeroMQ support',
80+    'ENABLE_USDT_TRACEPOINTS': 'USDT tracepoints',
81+}

maflcko commented at 11:39 am on November 11, 2024:

Not sure about silently ignoring those. The goal of the script is to generate the manpages of the releases, and fail if it can’t.

Allowing third parties to generate their own manpages can be implemented behind an option, or not at all.

BrandonOdiwuor commented at 1:25 pm on November 11, 2024:

@maflcko, are you suggesting the default behaviour should be to also fail if the enabled components are missing?

doc: Fix gen-manpages to check build options 55b50c8dae

doc: allow missing components when genrating manapages 59cee7043a

BrandonOdiwuor force-pushed on Nov 12, 2024

BrandonOdiwuor requested review from maflcko on Nov 12, 2024

in contrib/devtools/gen-manpages.py:18 in 59cee7043a

19+'build/src/bitcoind',
20+'build/src/bitcoin-cli',
21+'build/src/bitcoin-tx',
22+'build/src/bitcoin-wallet',
23+'build/src/bitcoin-util',
24+'build/src/qt/bitcoin-qt',

maflcko commented at 5:30 pm on November 13, 2024:

You can’t just change the relative path to include an unrelated prefix. This breaks when using BUILDDIR. Please see the conflicting pull request on how to correctly do it.

maflcko commented at 5:32 pm on November 13, 2024: member

This just fails on the release binaries. Please make sure to test the code yourself before asking for review. If you run into issues, you can ask questions.

0BUILDDIR=bld ./contrib/devtools/gen-manpages.py 
1Aborting generating manpages...
2Error: 'HAVE_SYSTEM' (System component) support is not enabled.
3Please enable it and try again.

You’ll probably have to use bld/src/bitcoin-build-config.h instead of the test ini. It seems odd anyway to tie the test config to this.

DrahtBot added the label Needs rebase on Nov 27, 2024

DrahtBot commented at 6:29 pm on November 27, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

doc: Fix gen-manpages to check build options #29457

Code Coverage & Benchmarks

Reviews

Conflicts