contrib: skip missing binaries in gen-manpages #30986

1. 
   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.

   closes #30985

2. 
   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

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	willcl-ark

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

3. DrahtBot added the label Scripts and tools on Sep 26, 2024
4. DrahtBot added the label CI failed on Sep 29, 2024
5. DrahtBot removed the label CI failed on Sep 29, 2024
6. 
   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.

7. contrib: skip missing binaries in gen-manpages

   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.

   65c0a98126
8. gen-manpages: Update default build path f52a385fd7
9. gen-manpages: Prompt error if no binaries are found ab1c541e39
10. andremralves force-pushed on Oct 2, 2024
11. 
    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

Contributors
andremralves DrahtBot willcl-ark

Labels
Scripts and tools