doc: Fix gen-manpages, rewrite in Python #24263

laanwj · 2022-02-04T15:36:27Z

Rewrite the manual page generation script in Python.

This:

solves '-' stripping issue (fixes Manual page generation on master broken #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.

laanwj · 2022-02-04T15:54:14Z

~~Does anyone understand this mypy output?~~

contrib/devtools/gen-manpages.py:26: error: No overload variant of "run" matches argument types "List[str]", "bool", "bool", "bool"  [call-overload]
contrib/devtools/gen-manpages.py:26: note: Possible overload variants:
contrib/devtools/gen-manpages.py:26: note:     def run(args: Union[Union[bytes, str], Sequence[Union[str, bytes, PathLike[str], PathLike[bytes]]]], bufsize: int = ..., executable: Union[str, bytes, PathLike[str], PathLike[bytes], None] = ..., stdin: Union[None, int, IO[Any]] = ..., stdout: Union[None, int, IO[Any]] = ..., stderr: Union[None, int, IO[Any]] = ..., preexec_fn: Optional[Callable[[], Any]] = ..., close_fds: bool = ..., shell: bool = ..., cwd: Union[str, bytes, PathLike[str], PathLike[bytes], None] = ..., env: Union[Mapping[bytes, Union[str, bytes, PathLike[str], PathLike[bytes]]], Mapping[str, Union[str, bytes, PathLike[str], PathLike[bytes]]], None] = ..., universal_newlines: bool = ..., startupinfo: Any = ..., creationflags: int = ..., restore_signals: bool = ..., start_new_session: bool = ..., pass_fds: Any = ..., *, check: bool = ..., encoding: str, errors: Optional[str] = ..., input: Optional[str] = ..., timeout: Optional[float] = ...) -> CompletedProcess[str]
contrib/devtools/gen-manpages.py:26: note:     def run(args: Union[Union[bytes, str], Sequence[Union[str, bytes, PathLike[str], PathLike[bytes]]]], bufsize: int = ..., executable: Union[str, bytes, PathLike[str], PathLike[bytes], None] = ..., stdin: Union[None, int, IO[Any]] = ..., stdout: Union[None, int, IO[Any]] = ..., stderr: Union[None, int, IO[Any]] = ..., preexec_fn: Optional[Callable[[], Any]] = ..., close_fds: bool = ..., shell: bool = ..., cwd: Union[str, bytes, PathLike[str], PathLike[bytes], None] = ..., env: Union[Mapping[bytes, Union[str, bytes, PathLike[str], PathLike[bytes]]], Mapping[str, Union[str, bytes, PathLike[str], PathLike[bytes]]], None] = ..., universal_newlines: bool = ..., startupinfo: Any = ..., creationflags: int = ..., restore_signals: bool = ..., start_new_session: bool = ..., pass_fds: Any = ..., *, check: bool = ..., encoding: Optional[str] = ..., errors: str, input: Optional[str] = ..., timeout: Optional[float] = ...) -> CompletedProcess[str]
contrib/devtools/gen-manpages.py:26: note:     <3 more similar overloads not shown, out of 5 total overloads>
contrib/devtools/gen-manpages.py:31: error: Argument 1 to "join" has incompatible type "Optional[str]"; expected "Union[str, PathLike[str]]"  [arg-type]
contrib/devtools/gen-manpages.py:38: error: Argument 1 to "join" has incompatible type "Optional[str]"; expected "Union[str, PathLike[str]]"  [arg-type]
contrib/devtools/gen-manpages.py:40: error: No overload variant of "run" matches argument types "List[str]", "bool", "bool"  [call-overload]
contrib/devtools/gen-manpages.py:40: note: Possible overload variants:
contrib/devtools/gen-manpages.py:40: note:     def run(args: Union[Union[bytes, str], Sequence[Union[str, bytes, PathLike[str], PathLike[bytes]]]], bufsize: int = ..., executable: Union[str, bytes, PathLike[str], PathLike[bytes], None] = ..., stdin: Union[None, int, IO[Any]] = ..., stdout: Union[None, int, IO[Any]] = ..., stderr: Union[None, int, IO[Any]] = ..., preexec_fn: Optional[Callable[[], Any]] = ..., close_fds: bool = ..., shell: bool = ..., cwd: Union[str, bytes, PathLike[str], PathLike[bytes], None] = ..., env: Union[Mapping[bytes, Union[str, bytes, PathLike[str], PathLike[bytes]]], Mapping[str, Union[str, bytes, PathLike[str], PathLike[bytes]]], None] = ..., universal_newlines: bool = ..., startupinfo: Any = ..., creationflags: int = ..., restore_signals: bool = ..., start_new_session: bool = ..., pass_fds: Any = ..., *, check: bool = ..., encoding: str, errors: Optional[str] = ..., input: Optional[str] = ..., timeout: Optional[float] = ...) -> CompletedProcess[str]
contrib/devtools/gen-manpages.py:40: note:     def run(args: Union[Union[bytes, str], Sequence[Union[str, bytes, PathLike[str], PathLike[bytes]]]], bufsize: int = ..., executable: Union[str, bytes, PathLike[str], PathLike[bytes], None] = ..., stdin: Union[None, int, IO[Any]] = ..., stdout: Union[None, int, IO[Any]] = ..., stderr: Union[None, int, IO[Any]] = ..., preexec_fn: Optional[Callable[[], Any]] = ..., close_fds: bool = ..., shell: bool = ..., cwd: Union[str, bytes, PathLike[str], PathLike[bytes], None] = ..., env: Union[Mapping[bytes, Union[str, bytes, PathLike[str], PathLike[bytes]]], Mapping[str, Union[str, bytes, PathLike[str], PathLike[bytes]]], None] = ..., universal_newlines: bool = ..., startupinfo: Any = ..., creationflags: int = ..., restore_signals: bool = ..., start_new_session: bool = ..., pass_fds: Any = ..., *, check: bool = ..., encoding: Optional[str] = ..., errors: str, input: Optional[str] = ..., timeout: Optional[float] = ...) -> CompletedProcess[str]
contrib/devtools/gen-manpages.py:40: note:     <3 more similar overloads not shown, out of 5 total overloads>

I can't reproduce it locally with mypy 0.761 and python 3.8.10. I don't think I'm passing anything weird to subprocess.run or path.join.

Edit: also tried with mypy 0.931. No dice.
Edit.2: FIgured it out, I think. capture_output is new in Python 3.7 so too recent.

Rewrite the manual page generation script in Python. This: - Solves '-' stripping issue (fixes bitcoin#22681) - Makes that copyright footer is generated again

DrahtBot · 2022-02-04T20:50:52Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22235 (script: add script to generate example bitcoin.conf by josibake)

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.

ghost · 2022-02-04T21:14:51Z

Concept ACK

laanwj · 2022-02-05T08:58:52Z

Something that is weird, but outside scope of this particular PR to fix. is that the copyright message is emitted inconsistently between binaries:

binary	-help	-version
bitcoind	yes	no
bitcoin-cli	no	no
bitcoin-tx	no	no
bitcoin-wallet	no	no
bitcoin-util	no	no
bitcoin-qt	no	yes

I don't have a strong opinion on whether it should be shown with -version or -help, but it probably should be either, and consistent between binaries.

laanwj · 2022-02-14T09:52:29Z

@dongcarl Can I get a concept ACK here at least? It should exactly do what you propose in #22681 (comment)

dongcarl

Code Review ACK 87f5406

Had to remind myself of the situation but everything looks good, just a few nits that are 100% ignore-able.

dongcarl · 2022-02-14T18:27:56Z

contrib/devtools/gen-manpages.py

+        print(f'{abspath} not found or not an executable', file=sys.stderr)
+        sys.exit(1)
+    # take first line (which must contain version)
+    verstr = r.stdout.split('\n')[0]


Nit

Suggested change

verstr = r.stdout.split('\n')[0]

verstr = r.stdout.splitlines()[0]

done in #24409.

dongcarl · 2022-02-14T18:28:12Z

contrib/devtools/gen-manpages.py

+for relpath in BINARIES:
+    abspath = os.path.join(builddir, relpath)
+    try:
+        r = subprocess.run([abspath, '--version'], stdout=subprocess.PIPE, universal_newlines=True)


Any reason not to do check=True here?

The script will fail because bitcoin-wallet returns non-0.

Yes, I initially did check but as @fanquake says. I'm not sure why bitcoin-wallet does that actually.

Addresses in #24428.

theStack · 2022-02-20T19:16:30Z

Concept ACK

gruve-p · 2022-02-20T21:49:28Z

Concept ACK

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.

Co-authored-by: Carl Dong <contact@carldong.me>

…h `-version` 5a89bed contrib: address gen-manpages feedback from #24263 (fanquake) 2618fb8 Output license info when binaries are passed -version (fanquake) 4c3e3c5 refactor: shift CopyrightHolders() and LicenseInfo() to clientversion.cpp (fanquake) Pull request description: Addresses a review comment from #24263, and addresses the [comment](bitcoin/bitcoin#24263 (comment)) where it was pointed out that we are inconsistent with emitting our copyright. After this change, the copyright is always emitted with `-version`, rather than `-help`, i.e: ```bash bitcoind -version Bitcoin Core version v22.99.0-fc1f355913f6-dirty Copyright (C) 2009-2022 The Bitcoin Core developers Please contribute if you find Bitcoin Core useful. Visit <https://bitcoincore.org/> for further information about the software. The source code is available from <https://github.com/bitcoin/bitcoin>. This is experimental software. Distributed under the MIT software license, see the accompanying file COPYING or <https://opensource.org/licenses/MIT> ``` The info is also added to binaries other than `bitcoind`/`bitcoin-qt`. This change also prevents duplicate copyright info appearing in the `bitcoind` man page. ACKs for top commit: laanwj: Tested ACK 5a89bed Tree-SHA512: 0ac2a1adf9e9de0c3206f35837008e3f93eaf15b193736203d71609273f0887cca20b8a90972cb9f941ebd62b330d61a0cbb5fb1b7a7f2dbc715ed8a0c1569d9

Adjust the exit codes / functionality of bitcoin-wallet such that it's not returning a non-zero exit code if there isn't a problem. This is a followup from bitcoin#24263, and should allow us to add and additional check=True to our gen-manpages.py script.

Now that bitcoin-wallet no-longer returns a non-zero exit code when it shouldn't do, we can add check=True to our subprocess call. Follows up to the comments in bitcoin#24263 (comment).

Adjust the exit codes / functionality of bitcoin-wallet such that it's not returning a non-zero exit code if there isn't a problem. This is a followup from bitcoin#24263, and should allow us to add and additional check=True to our gen-manpages.py script.

Now that bitcoin-wallet no-longer returns a non-zero exit code when it shouldn't do, we can add check=True to our subprocess call. Follows up to the comments in bitcoin#24263 (comment).

fa2b8ae util: improve bitcoin-wallet exit codes (MacroFake) Pull request description: Refactors `bitcoin-wallet` so that it doesn't return a non-zero exit code by default, and makes the option handling more inline with the other binaries. i.e outputting `Error: too few parameters` if you don't pass any options. Fixing this means we can check the process output in `gen-manpages.py`; which addresses the remaining [review comment](bitcoin/bitcoin#24263 (comment)) from #24263. Top commit has no ACKs. Tree-SHA512: 80bd8098faefb4401ca1e4d49937ef6c960cf60ce0e7fb9dc38904fbc2fd92e319ec04570381da84943b7477845bf6be00e977f4c0451b247a6698662ce8f1bf

laanwj added the Docs label Feb 4, 2022

laanwj added this to the 23.0 milestone Feb 4, 2022

laanwj force-pushed the 2022-02-gen-manpages branch from 79da7e0 to 8b185a8 Compare February 4, 2022 15:38

laanwj added 2 commits February 4, 2022 17:03

doc: Fix gen-manpages, rewrite in Python

42c2028

Rewrite the manual page generation script in Python. This: - Solves '-' stripping issue (fixes bitcoin#22681) - Makes that copyright footer is generated again

doc: Swap gen-manpages and update RC steps in release process

87f5406

laanwj force-pushed the 2022-02-gen-manpages branch from 8b185a8 to 87f5406 Compare February 4, 2022 16:03

DrahtBot mentioned this pull request Feb 5, 2022

script: add script to generate example bitcoin.conf #22235

Merged

luke-jr mentioned this pull request Feb 7, 2022

Bugfix: gen-manpages: Handle tagged versions correctly #24287

Closed

dongcarl approved these changes Feb 14, 2022

View reviewed changes

fanquake approved these changes Feb 21, 2022

View reviewed changes

fanquake merged commit bd6b1d0 into bitcoin:master Feb 21, 2022

fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 21, 2022

contrib: address gen-manpages feedback from bitcoin#24263

cb8bea5

Co-authored-by: Carl Dong <contact@carldong.me>

fanquake mentioned this pull request Feb 21, 2022

Always output license/copyright info with -version #24409

Merged

fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 22, 2022

contrib: address gen-manpages feedback from bitcoin#24263

0ed0d5d

Co-authored-by: Carl Dong <contact@carldong.me>

fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 22, 2022

contrib: address gen-manpages feedback from bitcoin#24263

5a89bed

Co-authored-by: Carl Dong <contact@carldong.me>

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 22, 2022

Merge bitcoin#24263: doc: Fix gen-manpages, rewrite in Python

090a56e

fanquake mentioned this pull request Feb 23, 2022

util: improve bitcoin-wallet exit codes #24428

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 23, 2022

contrib: address gen-manpages feedback from bitcoin#24263

4ac249e

maflcko mentioned this pull request Sep 12, 2022

util: improve bitcoin-wallet exit codes #26067

Merged

bitcoin locked and limited conversation to collaborators Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Fix gen-manpages, rewrite in Python #24263

doc: Fix gen-manpages, rewrite in Python #24263

laanwj commented Feb 4, 2022

laanwj commented Feb 4, 2022 •

edited

Loading

DrahtBot commented Feb 4, 2022 •

edited

Loading

ghost commented Feb 4, 2022

laanwj commented Feb 5, 2022 •

edited

Loading

laanwj commented Feb 14, 2022

dongcarl left a comment

dongcarl Feb 14, 2022

fanquake Feb 21, 2022

dongcarl Feb 14, 2022

fanquake Feb 21, 2022

laanwj Feb 22, 2022

fanquake Feb 23, 2022

theStack commented Feb 20, 2022

gruve-p commented Feb 20, 2022

fanquake left a comment

	verstr = r.stdout.split('\n')[0]
	verstr = r.stdout.splitlines()[0]

doc: Fix gen-manpages, rewrite in Python #24263

doc: Fix gen-manpages, rewrite in Python #24263

Conversation

laanwj commented Feb 4, 2022

laanwj commented Feb 4, 2022 • edited Loading

DrahtBot commented Feb 4, 2022 • edited Loading

Conflicts

ghost commented Feb 4, 2022

laanwj commented Feb 5, 2022 • edited Loading

laanwj commented Feb 14, 2022

dongcarl left a comment

Choose a reason for hiding this comment

dongcarl Feb 14, 2022

Choose a reason for hiding this comment

fanquake Feb 21, 2022

Choose a reason for hiding this comment

dongcarl Feb 14, 2022

Choose a reason for hiding this comment

fanquake Feb 21, 2022

Choose a reason for hiding this comment

laanwj Feb 22, 2022

Choose a reason for hiding this comment

fanquake Feb 23, 2022

Choose a reason for hiding this comment

theStack commented Feb 20, 2022

gruve-p commented Feb 20, 2022

fanquake left a comment

Choose a reason for hiding this comment

laanwj commented Feb 4, 2022 •

edited

Loading

DrahtBot commented Feb 4, 2022 •

edited

Loading

laanwj commented Feb 5, 2022 •

edited

Loading