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

BrandonOdiwuor · 2024-02-20T12:18:57Z

Fixes #17506

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

DrahtBot · 2024-02-20T12:19:00Z

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.

maflcko · 2024-02-20T12:34:53Z

contrib/devtools/gen-manpages.py

+    docs_to_exlude = []
+    if (binary == 'bitcoind' or binary == 'bitcoin-qt') and not config['components'].getboolean('HAVE_SYSTEM'):
+        docs_to_exlude += ['shutdownnotify', 'alertnotify', 'startupnotify', 'blocknotify']
+        return parse_and_remove_subsections_from_doc(document, docs_to_exlude)


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

DrahtBot · 2024-02-20T14:07:09Z

🚧 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 · 2024-02-20T22:28:29Z

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

contrib/devtools/gen-manpages.py

BrandonOdiwuor · 2024-02-21T13:21:41Z

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

fanquake · 2024-02-26T13:57:51Z

contrib/devtools/gen-manpages.py

+
+error_message = "Aborting generating manpages…\nError: {component} support is not enabled.\nPlease enable it and try again."
+
+if not config['components'].getboolean('HAVE_SYSTEM'):


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.

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

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

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.

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.

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

achow101 · 2024-10-15T14:36:12Z

Are you still working on this?

laanwj · 2024-10-15T14:46:12Z

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

[components]
# Which components are enabled. These are commented out by `configure` if they were disabled when running config.
ENABLE_WALLET=true
USE_SQLITE=true
#USE_BDB=true
ENABLE_CLI=true
ENABLE_BITCOIN_UTIL=true
ENABLE_WALLET_TOOL=true
ENABLE_BITCOIND=true
#ENABLE_FUZZ_BINARY=true
#ENABLE_ZMQ=true
ENABLE_EXTERNAL_SIGNER=true
#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 · 2024-11-11T11:31:24Z

@laanwj I updated the PR to include the suggestions

contrib/devtools/gen-manpages.py

maflcko · 2024-11-11T11:39:15Z

contrib/devtools/gen-manpages.py

+    'ENABLE_FUZZ_BINARY': 'Fuzz testing binary',
+    'ENABLE_ZMQ': 'ZeroMQ support',
+    'ENABLE_USDT_TRACEPOINTS': 'USDT tracepoints',
+}


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.

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

maflcko

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.

BUILDDIR=bld ./contrib/devtools/gen-manpages.py 
Aborting generating manpages...
Error: 'HAVE_SYSTEM' (System component) support is not enabled.
Please 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.

maflcko · 2024-11-13T17:30:03Z

contrib/devtools/gen-manpages.py

+'build/src/bitcoin-tx',
+'build/src/bitcoin-wallet',
+'build/src/bitcoin-util',
+'build/src/qt/bitcoin-qt',


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.

DrahtBot · 2024-11-27T18:29:48Z

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

DrahtBot · 2025-01-22T16:22:34Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

fanquake · 2025-02-20T17:27:17Z

Drafted for now. @BrandonOdiwuor are you still working on this?

DrahtBot · 2025-05-20T01:12:49Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot added the Docs label Feb 20, 2024

maflcko reviewed Feb 20, 2024

View reviewed changes

DrahtBot added the CI failed label Feb 20, 2024

BrandonOdiwuor force-pushed the gen-manpages-build-options branch from 4c3062a to 1e1ab90 Compare February 21, 2024 11:16

maflcko reviewed Feb 21, 2024

View reviewed changes

contrib/devtools/gen-manpages.py Outdated Show resolved Hide resolved

BrandonOdiwuor force-pushed the gen-manpages-build-options branch 5 times, most recently from efb45f0 to 428b921 Compare February 21, 2024 12:02

BrandonOdiwuor marked this pull request as ready for review February 21, 2024 12:04

DrahtBot removed the CI failed label Feb 21, 2024

fanquake reviewed Feb 26, 2024

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Jul 13, 2024

maflcko mentioned this pull request Aug 29, 2024

doc: update documentation and scripts related to build directories #30741

Merged

achow101 requested a review from laanwj October 15, 2024 14:36

DrahtBot mentioned this pull request Oct 27, 2024

contrib: skip missing binaries in gen-manpages #30986

Merged

BrandonOdiwuor force-pushed the gen-manpages-build-options branch from 428b921 to 7651dc3 Compare November 11, 2024 10:37

BrandonOdiwuor requested review from maflcko and fanquake November 11, 2024 11:29

maflcko reviewed Nov 11, 2024

View reviewed changes

doc: Fix gen-manpages to check build options

55b50c8

doc: allow missing components when genrating manapages

59cee70

BrandonOdiwuor force-pushed the gen-manpages-build-options branch from 7651dc3 to 59cee70 Compare November 12, 2024 07:47

BrandonOdiwuor requested a review from maflcko November 12, 2024 07:48

maflcko reviewed Nov 13, 2024

View reviewed changes

DrahtBot mentioned this pull request Nov 16, 2024

cmake: Set top-level target output locations #31161

Merged

DrahtBot added the Needs rebase label Nov 27, 2024

DrahtBot mentioned this pull request Nov 28, 2024

ipc: add bitcoin-mine test program #30437

Open

fanquake marked this pull request as draft February 20, 2025 17:26

maflcko closed this May 20, 2025

BrandonOdiwuor mentioned this pull request Jul 29, 2025

doc: gen-manpages.py should check build options #33085

Open


		error_message = "Aborting generating manpages…\nError: {component} support is not enabled.\nPlease enable it and try again."

		if not config['components'].getboolean('HAVE_SYSTEM'):

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

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

Uh oh!

Conversation

BrandonOdiwuor commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 20, 2024

Uh oh!

luke-jr commented Feb 20, 2024

Uh oh!

Uh oh!

BrandonOdiwuor commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 15, 2024

Uh oh!

laanwj commented Oct 15, 2024

Uh oh!

BrandonOdiwuor commented Nov 11, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 27, 2024

Uh oh!

DrahtBot commented Jan 22, 2025

Uh oh!

fanquake commented Feb 20, 2025

Uh oh!

DrahtBot commented May 20, 2025

Uh oh!

Uh oh!

BrandonOdiwuor commented Feb 20, 2024 •

edited

Loading

DrahtBot commented Feb 20, 2024 •

edited

Loading