contrib: skip missing binaries in gen-manpages

andremralves commented at 9:37 pm on September 26, 2024: none

Instead of stopping the execution of gen-manpages.py when a binary is not found, continue generating manpages for the available binaries and skip the missing ones.

A new argument, --skip-missing-binaries, has been added to enable this behavior.

0➜  bitcoin git:(fix-gen-manpages) ✗ ./contrib/devtools/gen-manpages.py --help
1usage: gen-manpages.py [-h] [-s]
2
3options:
4  -h, --help            show this help message and exit
5  -s, --skip-missing-binaries
6                        skip generation for binaries that are not found

closes #30985

DrahtBot commented at 9:38 pm on September 26, 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/30986.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	willcl-ark, BrandonOdiwuor
Stale ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29457 (doc: Fix gen-manpages to check build options by BrandonOdiwuor)

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 Scripts and tools on Sep 26, 2024

DrahtBot added the label CI failed on Sep 29, 2024

DrahtBot removed the label CI failed on Sep 29, 2024

willcl-ark commented at 8:47 am on October 1, 2024: member

Concept ACK.

I wonder if it’s not easier to minimize what is being caught in the IOError try block and just remove missing bins from the BINARIES list? This list is used later on in the script too so in my opinion that is probably the better option. Otherwise we will end up referencing non-existing binaries/manpages in the existing manpages. Something like:

 0diff --git a/contrib/devtools/gen-manpages.py b/contrib/devtools/gen-manpages.py
 1index 92acd9a4037..0e0d31807bd 100755
 2--- a/contrib/devtools/gen-manpages.py
 3+++ b/contrib/devtools/gen-manpages.py
 4@@ -39,7 +39,8 @@ for relpath in BINARIES:
 5         r = subprocess.run([abspath, "--version"], stdout=subprocess.PIPE, check=True, text=True)
 6     except IOError:
 7         print(f'{abspath} not found or not an executable', file=sys.stderr)
 8-        sys.exit(1)
 9+        BINARIES.remove(relpath)
10+        continue
11     # take first line (which must contain version)
12     verstr = r.stdout.splitlines()[0]
13     # last word of line is the actual version e.g. v22.99.0-5c6b3d5b3508

I also notice running this post-cmake without specifying BUILDDIR results in a slightly incorrect error due to the new binary locations:

 0₿ ./contrib/devtools/gen-manpages.py
 1/home/will/src/bitcoin/src/bitcoind not found or not an executable. Skipping...
 2/home/will/src/bitcoin/src/bitcoin-cli not found or not an executable. Skipping...
 3/home/will/src/bitcoin/src/bitcoin-tx not found or not an executable. Skipping...
 4/home/will/src/bitcoin/src/bitcoin-wallet not found or not an executable. Skipping...
 5/home/will/src/bitcoin/src/bitcoin-util not found or not an executable. Skipping...
 6/home/will/src/bitcoin/src/qt/bitcoin-qt not found or not an executable. Skipping...
 7Traceback (most recent call last):
 8  File "/home/will/src/bitcoin/./contrib/devtools/gen-manpages.py", line 63, in <module>
 9    footer.write('\n'.join(versions[0][2]).strip())
10                           ~~~~~~~~^^^
11IndexError: list index out of range

Whilst you are in here, perhaps we could return early if versions is empty, optionally prompting folks to set BUILDDIR?

We could change the default paths, but as people may use different build dirs I’m not sure if this is worth it.

andremralves force-pushed on Oct 2, 2024

andremralves commented at 2:21 pm on October 2, 2024: none

Thanks for the feedback!

I wonder if it’s not easier to minimize what is being caught in the IOError try block and just remove missing bins from the BINARIES list? This list is used later on in the script too so in my opinion that is probably the better option. Otherwise we will end up referencing non-existing binaries/manpages in the existing manpages.

I agree and have updated the commit to reflect your suggestions. I just needed to make a copy of BINARIES so that it’s possible to remove items from it while iterating over the copied list. Thanks.

Whilst you are in here, perhaps we could return early if versions is empty, optionally prompting folks to set BUILDDIR?

I also think it’s a good idea.

We could change the default paths, but as people may use different build dirs I’m not sure if this is worth it.

I think it’s worth it since other scripts/docs have been updated to use /build as the default path, so we can do the same here.

I’ve added a few more commits addressing the latter two topics. Could you review them as well? @willcl-ark

laanwj commented at 12:01 pm on October 24, 2024: member

i’m divided on this. There’s a use-case, for sure, but this script is mainly used to generate the manpages for a release. And we’ve had exactly the opposite issue where conditional behavior resulted in incomplete manual pages. (Prompting me to open #17506) So not sure it should be the default. As an option, sure.

andremralves commented at 1:19 pm on October 24, 2024: none

The main issue this PR addresses is that currently, no man pages are generated if a single binary is missing. The goal is to skip the missing binary and generate man pages for the others, as was done in the previous shell script.

If I understand correctly, you want to ensure no information is lost in the man pages, right? In that case, I can simply remove the following line:

042          BINARIES.remove(relpath)

This way, the existing binaries will have their man pages generated, and all the other man pages will still be referenced at the bottom, even if the binaries are missing.

andremralves force-pushed on Oct 24, 2024

laanwj commented at 1:31 pm on October 24, 2024: member

Kind of. The problem is that the command will still finish successfully in case of an incomplete build (or build configuration), while the before a release it’s critical to update all the pages.

This is why i suggested to add this as an option eg --skip-missing-binaries.

This way, the existing binaries will have their man pages generated, and all the other man pages will still be referenced at the bottom, even if the binaries are missing.

That is an improvement, but doesn’t entirely address the concern.

andremralves force-pushed on Oct 27, 2024

andremralves commented at 4:27 am on October 28, 2024: none

I probably don’t have enough understanding of the release process to know exactly how this change could affect it, but I assume that having gen-manpages.py exit with an error is important somewhere during the process.

I implemented your suggestion by adding the –skip-missing-binaries option to the command.

Thanks for the feedback! What do you think of it now?

fanquake commented at 12:07 pm on October 29, 2024: member

I wonder if a better approach here would be to further integrate the manpage generation into the build system; given that the two issues of “which binaries exist” and “where do they exist” are already solved in that case. This should also remove the dep on git.

laanwj commented at 1:51 pm on October 29, 2024: member

I implemented your suggestion by adding the –skip-missing-binaries option to the command.

Yes, thanks!

I wonder if a better approach here would be to further integrate the manpage generation into the build system; given that the two issues of “which binaries exist” and “where do they exist” are already solved in that case. This should also remove the dep on git.

That would be better. The thing is, the build system isn’t allowed to execute the binaries it generated (and even if so this wouldn’t support cross compiling and be extremely ugly). So this would need an alternative way of extracting the arguments for the tools than running them with --help. Parsing C++ code is kind of nasty, so i’m not sure how.

BrandonOdiwuor commented at 2:32 pm on November 6, 2024: contributor

Concept ACK Adding --skip-missing-binaries as an optional flag seems reasonable.

in contrib/devtools/gen-manpages.py:28 in 7060d64bf7 outdated

23+parser.add_argument(
24+    "-s",
25+    "--skip-missing-binaries",
26+    action="store_true",
27+    default=False,
28+    help="skip generation for binaries that are not found",

rkrux commented at 11:55 am on November 8, 2024:

nit: for verbosity

0help="skip generation for binaries that are not found in the build path",

andremralves commented at 4:11 am on November 14, 2024:

Good suggestion. Thanks!

in contrib/devtools/gen-manpages.py:72 in f0806188dc outdated

67@@ -68,6 +68,10 @@
68 
69     versions.append((abspath, verstr, copyright))
70 
71+if not versions:
72+    print(f'No binaries found in {builddir}. Set BUILDDIR to specify the correct build path.')

rkrux commented at 12:08 pm on November 8, 2024:

0Set BUILDDIR to specify the correct build path.

Is the intent to make the user explicitly set BUILDDIR? Not aware of the usual use cases but as a user, my preference is to just run the script to generate the man pages w/o needing to set an env var. I’d be ok with this error as well.

0print(f'No binaries found in {builddir}. Please specify the correct build path.')

andremralves commented at 5:11 am on November 14, 2024:

The most common use case is running the script without setting BUILDDIR, in which case the default builddir, /build, is used. However, if no binaries are found in /build we remind the user to set BUILDDIR to specify the correct path.

rkrux commented at 8:58 am on November 14, 2024:

if no binaries are found in /build

This could also be because the user might not have built the binaries yet even in the case of default build path.

A more verbose message could be:

0'No binaries found in {builddir}. Please ensure the binaries are present in {builddir}, or set another build path using the BUILDDIR env variable.'

rkrux approved

rkrux commented at 12:13 pm on November 8, 2024: none

Coming here from #31255, putting my two cents in.

I had to generate man pages while reviewing a PR and found that the script threw error even in the default case due to the new build process that moved the binaries in /build, so a strong ACK for 0194a638d31fef7e8193c50843c11d2c2ee212a3 because IMO the script should work in the default case w/o needing to set the BUILDDDIR.

As I read from @laanwj’s comment that the main intent of this script is to generate man pages for a release, I’d assume all the binaries would be found because they are part of the release? Having said this, the new --skip-missing-binaries arg could be useful in the cases when not everyone will have all the binaries. Eg: I didn’t use to have the GUI earlier.

All in all ACK for f0806188dcde531a401bdfeaaed8c04801f4b5f0

gen-manpages: implement --skip-missing-binaries

With --skip-missing-binaries, instead of stopping the execution of
gen-manpages.py when a binary is not found, continue generating
manpages for the available binaries and skip the missing ones.

299e2220e9

gen-manpages: Update default build path c32104f544

gen-manpages: Prompt error if no binaries are found ca0d7eb8d1

andremralves force-pushed on Nov 14, 2024

contrib: skip missing binaries in gen-manpages #30986

Code Coverage & Benchmarks

Reviews

Conflicts