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

pull BrandonOdiwuor wants to merge 2 commits into bitcoin:master from BrandonOdiwuor:gen-manpages-build-options changing 1 files +59 −6

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.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/21766942690</sub>
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 force-pushed on Feb 21, 2024
BrandonOdiwuor force-pushed on Feb 21, 2024
BrandonOdiwuor force-pushed on Feb 21, 2024
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:
```
[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 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.
```
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.
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.
DrahtBot commented at 4:22 PM on January 22, 2025: contributor


⌛ 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 marked this as a draft on Feb 20, 2025
fanquake commented at 5:27 PM on February 20, 2025: member

Drafted for now. @BrandonOdiwuor are you still working on this?
DrahtBot commented at 1:12 AM on May 20, 2025: contributor


⌛ 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.
maflcko closed this on May 20, 2025

Contributors

Review Requested
laanwj fanquake

Labels

Docs Needs rebase

Linked (view graph)

#17506 gen-manpages output depends on build options, so needs to check them #33085 doc: gen-manpages.py should check build options

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

Code Coverage & Benchmarks

Reviews

Conflicts