-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backport: Merge bitcoin/bitcoin#24263: doc: Fix gen-manpages, rewrite in Python #6555
Conversation
87f5406 doc: Swap gen-manpages and update RC steps in release process (laanwj) 42c2028 doc: Fix gen-manpages, rewrite in Python (laanwj) Pull request description: Rewrite the manual page generation script in Python. This: - solves '-' stripping issue (fixes bitcoin#22681) - makes that a copyright footer is generated correctly again Also change the release process to swap gen-manpages and update RC steps, so that the pages will have the correct rc and/or final version. ACKs for top commit: dongcarl: Code Review ACK 87f5406 fanquake: ACK 87f5406 - tested generating and opening the man pages locally, but didn't run through the release process. Will propose some changes to address consolidating the help / version output. Tree-SHA512: 39254721ca84e4f223a321c554f2e08c36428b15019a0f9fa3eff408b4c6f1e1d74941143f4d2927427afa3ad7a7e6f999d6ec660132d817809b640a87ae9f7d
WalkthroughThe pull request transitions the manpage generation tool from a Bash script to a Python implementation. The new Python script, Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
contrib/devtools/gen-manpages.py (6)
2-4
: Update the copyright notice to include Dash developers.Since this is a backported script in the Dash repository, consider adding Dash developers to the copyright notice.
-# Copyright (c) 2022 The Bitcoin Core developers +# Copyright (c) 2022 The Bitcoin Core developers +# Copyright (c) 2024 The Dash Core developers
10-17
: Clarify the commented-out binary entry.The
src/dash-util
binary is commented out. If it's not needed, consider removing the line entirely. If it will be needed in the future, add a comment explaining why it's commented out.-#'src/dash-util',
24-27
: Improve error handling for Git command.The Git command could fail if not in a Git repository or if Git is not installed. Consider adding proper error handling.
- r = subprocess.run([git, 'rev-parse', '--show-toplevel'], stdout=subprocess.PIPE, check=True, universal_newlines=True) - topdir = r.stdout.rstrip() + try: + r = subprocess.run([git, 'rev-parse', '--show-toplevel'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True, universal_newlines=True) + topdir = r.stdout.rstrip() + except subprocess.CalledProcessError as e: + print(f"Error: Failed to determine top directory using git: {e.stderr}", file=sys.stderr) + sys.exit(1)
45-48
: Make version string validation more robust.The version string parsing assumes a specific format. Consider making it more flexible to handle different version string formats.
- verstr = verstr.split()[-1] - assert verstr.startswith('v') + verstr = verstr.split()[-1] + if not verstr.startswith('v'): + print(f"Error: Version string '{verstr}' from {abspath} does not start with 'v'", file=sys.stderr) + sys.exit(1)
50-53
: Improve copyright extraction robustness.The copyright extraction assumes dash-qt will always output a copyright message in a specific format. Consider adding validation and error handling.
if relpath == 'src/qt/dash-qt': + if len(r.stdout.split('\n')) < 2: + print(f"Error: Could not extract copyright information from {abspath}", file=sys.stderr) + sys.exit(1) copyright = r.stdout.split('\n')[1:]
69-73
: Verify help2man availability before use.Consider checking if help2man is available before attempting to generate manual pages.
+ # Verify help2man is available + try: + subprocess.run([help2man, '--version'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True) + except (subprocess.CalledProcessError, FileNotFoundError): + print("Error: help2man is not available. Please install help2man.", file=sys.stderr) + sys.exit(1) + # Call the binaries through help2man to produce a manual page for each of them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contrib/devtools/README.md
(2 hunks)contrib/devtools/gen-manpages.py
(1 hunks)contrib/devtools/gen-manpages.sh
(0 hunks)doc/release-process.md
(1 hunks)
💤 Files with no reviewable changes (1)
- contrib/devtools/gen-manpages.sh
✅ Files skipped from review due to trivial changes (2)
- contrib/devtools/README.md
- doc/release-process.md
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build (linux64_ubsan, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_tsan, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_sqlite, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_nowallet, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_fuzz, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_cxx20, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (arm-linux, arm-linux-gnueabihf, arm-linux)
🔇 Additional comments (1)
contrib/devtools/gen-manpages.py (1)
56-60
: LGTM! Clear and helpful warning message.The warning about dirty tree builds is well-implemented and provides clear instructions for resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 69b1328; looks correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 69b1328
ff785ed docs: update man pages for 22.1 (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented #6551 ## What was done? Updated man pages by new script `contrib/devtools/gen-manpages.py` see #6555 ## How Has This Been Tested? N/A ## Breaking Changes N/A ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK ff785ed; diff looks good UdjinM6: utACK ff785ed Tree-SHA512: 60a81c880e6ff824fc661dc6c6da20d85aa662fe228f7080dcb8c47db69c88adc30a40ab84832d83994ab56a26ed1de4d800dd45cd282c61233fde2d4e2212b7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 69b1328
ff785ed docs: update man pages for 22.1 (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented dashpay#6551 ## What was done? Updated man pages by new script `contrib/devtools/gen-manpages.py` see dashpay#6555 ## How Has This Been Tested? N/A ## Breaking Changes N/A ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK ff785ed; diff looks good UdjinM6: utACK ff785ed Tree-SHA512: 60a81c880e6ff824fc661dc6c6da20d85aa662fe228f7080dcb8c47db69c88adc30a40ab84832d83994ab56a26ed1de4d800dd45cd282c61233fde2d4e2212b7
…ges, rewrite in Python 69b1328 Merge bitcoin#24263: doc: Fix gen-manpages, rewrite in Python (fanquake) Pull request description: ## Issue being fixed or feature implemented gen-pages.sh doesn't work correctly for `dash-cli`, it removes all hyphens somehow: ```diff .IP Set a whitelist to filter incoming RPC calls for a specific user. The field <whitelist> comes in the format: <USERNAME>:<rpc 1>,<rpc 2>,...,<rpc n>. If multiple whitelists are set for a given user, -they are set\-intersected. See \fB\-rpcwhitelistdefault\fR documentation +they are setintersected. See \fBrpcwhitelistdefault\fR documentation for information on default whitelist behavior. .HP ``` ## What was done? - backport bitcoin#24263 Rewrite the manual page generation script in Python. This: - solves '-' stripping issue (fixes bitcoin#22681) - makes that a copyright footer is generated correctly again Also change the release process to swap gen-manpages and update RC steps, so that the pages will have the correct rc and/or final version. ## How Has This Been Tested? Produced correct documents for Dash Core v22.1: dashpay#6554 ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 69b1328; looks correct UdjinM6: utACK 69b1328 kwvg: utACK 69b1328 Tree-SHA512: 01a7a9a8a4ba762e6ff36035a3fb554d998232d62da8c3441b12741e6a3b626c01c581c1ff3f6aa3dcac02e15d62eec7f2a7f94be9399895557cd0a7115424a7
e62b4eb Merge #6555: backport: Merge bitcoin#24263: doc: Fix gen-manpages, rewrite in Python (pasta) 53d0ff1 Merge #6552: docs: update supported versions in SECURITY.md (pasta) 4571d89 Merge #6554: docs: update man pages for 22.1 (pasta) bc91a83 Merge #6553: chore: bump assume valid / checkpoints for v22.1 (pasta) Pull request description: ## Issue being fixed or feature implemented Backports from develop to 22.1 ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK e62b4eb Tree-SHA512: 9f88c1a77ec1adc453b023b764f91760e13064c86d109c29bb7856f2588c6450e518c9479f8c3e245f7eeaf5ef6d28aa85f00b4b5e94ef5a4d80cb5b3d7fdff2
774a018 chore: set release to true (pasta) e62b4eb Merge #6555: backport: Merge bitcoin#24263: doc: Fix gen-manpages, rewrite in Python (pasta) 53d0ff1 Merge #6552: docs: update supported versions in SECURITY.md (pasta) 4571d89 Merge #6554: docs: update man pages for 22.1 (pasta) bc91a83 Merge #6553: chore: bump assume valid / checkpoints for v22.1 (pasta) c11ec40 docs: add release notes for 22.1.0 (pasta) Pull request description: ## Issue being fixed or feature implemented Manually suppressed configure.ac changes ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK fd51512 kwvg: utACK fd51512 Tree-SHA512: 41f08e1879596c0dd339209f869f2e4c7497f275df1928154ed8dfd2df1ff66c0618792afbff8313a818ac4d14ed8aa04f00e0fa96468adcc494042a7a28cc2b
Issue being fixed or feature implemented
gen-pages.sh doesn't work correctly for
dash-cli
, it removes all hyphens somehow:What was done?
Rewrite the manual page generation script in Python.
This:
Also change the release process to swap gen-manpages and update RC steps, so that the pages will have the correct rc and/or final version.
How Has This Been Tested?
Produced correct documents for Dash Core v22.1: #6554
Breaking Changes
N/A
Checklist: