In the other readmes we’ve provided a default build directory instead, unified the developer-notes.md to specify it explicitly.
In the next commit I’ve used this default to go over each reference to our binaries and changed their in-source references to the build directory. Some of these changes were in example outputs - I haven’t validated that the outputs are still the same. I haven’t modified the build folders in the devtools.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pablomartin4btc, maflcko, fanquake |
| Concept ACK | theStack, 0xB10C |
| Stale ACK | hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
337@@ -338,9 +338,10 @@ Recommendations:
338
339 ### Generating Documentation
340
341-The documentation can be generated with `cmake --build <build_dir> --target docs`.
342-The resulting files are located in `<build_dir>/doc/doxygen/html`; open
343-`index.html` in that directory to view the homepage.
344+Assuming the build directory is named `build`,
345+the documentation can be generated with `cmake --build build --target docs`.
Seems to me that was the way we’ve proceded before: https://github.com/bitcoin/bitcoin/blob/6ce50fd9d0ae6850d54bf883e7a7c1bcb6912c5c/src/test/README.md?plain=1#L19
And since we need to update the build directories in the docs (at least that’s what I tried doing here), we need a sensible default.
Do you have a better suggestion?
<build_dir>/doc/doxygen/html; open …” is correct, thus no update is required, and the section can be left as-is. This means that the hunk can be dropped from the diff, so that the diff becomes smaller.
thus no update is required
But isn’t that how we’ve done it in other cases during the cmake change, i.e. assume a default build dir?
Or you mean that the changes here from /src/ to /build/src/ are all invalid?
228@@ -229,13 +229,13 @@ The output is denoised of errors from external dependencies.
229 To run clang-tidy on all source files:
230
231 ```sh
232-( cd ./src/ && run-clang-tidy -j $(nproc) )
233+( cd ./build/src/ && run-clang-tidy -j $(nproc) )
./src/.
12-'src/bitcoin-cli',
13-'src/bitcoin-tx',
14-'src/bitcoin-wallet',
15-'src/bitcoin-util',
16-'src/qt/bitcoin-qt',
17+'build/src/bitcoind',
Wont this break the usage as described in the readme:
0BUILDDIR=$PWD/build ./contrib/devtools/gen-manpages.py
1/root/ci_scratch/build/build/src/bitcoind not found or not an executable
To use this tool with out-of-tree builds set
BUILDDIR.
Does this comment still apply?
Should I change it to builddir = os.getenv('BUILDDIR', topdir) to default to the build dir instead of modifying the BINARIES paths?
gen-manpages.py changes, thanks for the comments.
473 $ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \
474- --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite
475-$ valgrind -v --leak-check=full src/bitcoind -printtoconsole
476+ --show-leak-kinds=all build/src/test/test_bitcoin --log_level=test_suite
477+$ valgrind -v --leak-check=full build/src/bitcoind -printtoconsole
478 $ ./test/functional/test_runner.py --valgrind
build here too.
Why is this a draft?
master wasn’t stable yet, ready for review!
63@@ -64,10 +64,10 @@ trap finish EXIT
64 EARLY_IBD_FLAGS="-maxtipage=9223372036854775207 -minimumchainwork=0x00"
65
66 server_rpc() {
67- ./src/bitcoin-cli -rpcport=$SERVER_RPC_PORT -datadir="$SERVER_DATADIR" "$@"
68+ ./build/src/bitcoin-cli -rpcport=$SERVER_RPC_PORT -datadir="$SERVER_DATADIR" "$@"
build, but still let the user specify a dir, similar to how the manpage generation currently works.
Especially if people are likely to be using multiple different dirs, or want to compare output between builds etc? It seems like a better approach would be to default to
build, but still let the user specify a dir
I agree with @fanquake, but until #29553 (which will remove this script entirely) gets merged (I’m testing it) and released, the script would fail (same if it get changed and the user specify another dir ok but default is build everywhere)…
85@@ -86,7 +86,7 @@ if [[ ! -e ${TEST_BITCOIN_BINARY} ]]; then
86 fi
87
88 get_file_suffix_count() {
89- find src/ -type f -name "*.$1" | wc -l
90+ find build/src/ -type f -name "*.$1" | wc -l
devtools changes. Will rebase in s separate push, if needed.
Can you fixup all the commits to have proper prefixes, commit messages etc.
Also, I don’t think putting changes into commits based on if you actually tested things on not, make that much sense. I guess that’s for your own convenience so it’s easier for you to can drop the changes if they happen to be broken? However it doesn’t really benefit contributors, or the changelog.
To correspond to the documentation style of e.g. src/test/README.md
Co-authored-by: pablomartin4btc <pablomartin4btc@gmail.com>
commits based on if you actually tested things or not
That’s also why it was a draft at first, so that I can get feedback on how to proceed with the devtools files. I’ve reverted them, it’s ready for review now, thanks for the feedback.
