doc: fixed inconsistencies in documentation between autotools to cmake change #30875

kevkevinpal commented at 12:02 AM on September 12, 2024: contributor

A bit of a followup from #30840

In this change the documentation where we refer to the ./configure script which is now gone and have converted the configure params to use the cmake equivalent.

DrahtBot commented at 12:02 AM on September 12, 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
ACK	maflcko, jonatack, jarolrod, tdb3, pablomartin4btc
Stale ACK	davidgumberg

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:

#30510 (multiprocess: Add IPC wrapper for Mining interface by ryanofsky)

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 Docs on Sep 12, 2024

maflcko commented at 5:12 AM on September 12, 2024: member

It would be nice to fix all remaining instances in one go. See also #30664 (comment) and #30664 (comment)

DrahtBot added the label CI failed on Sep 12, 2024

kevkevinpal force-pushed on Sep 12, 2024

pablomartin4btc commented at 10:06 AM on September 13, 2024: member

ACK dd0941cc6de6852c8ee1347c7ccfcc887cd07e4b

nit: since you are there at doc/zmq.md, I think there's a typo close to the end, look for "chain--as".

maflcko commented at 10:17 AM on September 13, 2024: member

I think you are still missing:

test/lint/lint-spelling.py:FILES_ARGS = ['git', 'ls-files', '--', ":(exclude)build-aux/m4/", ":(exclude)contrib/seeds/*.txt", ":(exclude)depends/", ":(exclude)doc/release-notes/", ":(exclude)src/qt/locale/", ":(exclude)src/qt/*.qrc", ":(exclude)contrib/guix/patches"]

Also, the pull title should be updated?

in doc/multiprocess.md:26 in dd0941cc6d outdated

  22 | @@ -23,9 +23,9 @@ src/bitcoin-node -regtest -printtoconsole -debug=ipc
  23 |  BITCOIND=bitcoin-node test/functional/test_runner.py
  24 |  ```
  25 |  
  26 | -The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `--enable-multiprocess` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option).

fanquake commented at 11:19 AM on September 13, 2024:

A few lines above here. This

cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure
make
src/bitcoin-node -regtest -printtoconsole -debug=ipc
BITCOIND=bitcoin-node test/functional/test_runner.py

should be replaced with:

cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
cmake -B build --toolchain=depends/x86_64-pc-linux-gnu/toolchain.cmake
cmake --build build
build/src/bitcoin-node -regtest -printtoconsole -debug=ipc
BITCOIND=build/src/bitcoin-node build/test/functional/test_runner.py

Although, it also looks like that is currently broken:

BITCOIND=build/src/bitcoin-node build/test/functional/test_runner.py
Temporary test directory at /tmp/test_runner_₿_🏃_20240913_111920
2024-09-13T11:19:20.313000Z TestFramework (INFO): PRNG seed is: 4821438991961925939
2024-09-13T11:19:20.315000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20240913_111920/cache
2024-09-13T11:19:20.316000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/root/ci_scratch/test/functional/test_framework/test_framework.py", line 131, in main
    self.setup()
  File "/root/ci_scratch/test/functional/test_framework/test_framework.py", line 301, in setup
    self.setup_chain()
  File "/root/ci_scratch/test/functional/test_framework/test_framework.py", line 392, in setup_chain
    self._initialize_chain()
  File "/root/ci_scratch/test/functional/test_framework/test_framework.py", line 853, in _initialize_chain
    self.start_node(CACHE_NODE_ID)
  File "/root/ci_scratch/test/functional/test_framework/test_framework.py", line 553, in start_node
    node.start(*args, **kwargs)
  File "/root/ci_scratch/test/functional/test_framework/test_node.py", line 256, in start
    self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.12/subprocess.py", line 1955, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'build/src/bitcoin-node'
2024-09-13T11:19:20.367000Z TestFramework (INFO): Stopping nodes
2024-09-13T11:19:20.367000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20240913_111920/cache
2024-09-13T11:19:20.367000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20240913_111920/cache/test_framework.log
2024-09-13T11:19:20.367000Z TestFramework (ERROR): 
2024-09-13T11:19:20.368000Z TestFramework (ERROR): Hint: Call /root/ci_scratch/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20240913_111920/cache' to consolidate all logs
2024-09-13T11:19:20.368000Z TestFramework (ERROR): 
2024-09-13T11:19:20.368000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-09-13T11:19:20.368000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-09-13T11:19:20.368000Z TestFramework (ERROR): 
Traceback (most recent call last):
  File "/root/ci_scratch/build/test/functional/test_runner.py", line 950, in <module>
    main()
  File "/root/ci_scratch/build/test/functional/test_runner.py", line 571, in main
    run_tests(
  File "/root/ci_scratch/build/test/functional/test_runner.py", line 624, in run_tests
    subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + ["--tmpdir=%s/cache" % tmpdir])
  File "/usr/lib/python3.12/subprocess.py", line 466, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/usr/bin/python3', '/root/ci_scratch/build/test/functional/create_cache.py', '--cachedir=/root/ci_scratch/build/test/cache', '--configfile=/root/ci_scratch/build/test/functional/../config.ini', '--tmpdir=/tmp/test_runner_₿_🏃_20240913_111920/cache']' returned non-zero exit status 1.

kevkevinpal commented at 1:12 PM on September 13, 2024:

thanks for the feedback!

I updated this to

cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
cmake -B build --toolchain=depends/x86_64-pc-linux-gnu/toolchain.cmake
cmake --build build
build/src/bitcoind -regtest -printtoconsole -debug=ipc
BITCOIND=$(pwd)/build/src/bitcoind build/test/functional/test_runner.py

Which does start running the tests please let me know if this looks ok

tdb3 commented at 6:20 PM on September 14, 2024:

Wouldn't we want to keep bitcoin-node (part of multiprocess) rather than change to bitcoind? Am I missing something?

Thoughts @ryanofsky ?

ryanofsky commented at 4:21 PM on September 16, 2024:

re: #30875 (review)

Thanks for the ping! Yes, this should say bitcoin-node not bitcoind to test the multiprocess binary. Either command line will work, but bitcoin-node will make the test more meaningful, especially with changes from #10102 merged, because bitcoind will run tests with the node and wallet in the same process, while bitcoin-node will spawn separate bitcoin-wallet processes

kevkevinpal commented at 5:23 PM on September 16, 2024:

I went ahead and changed it back from bitcoind to bitcoin-node in dc2e223

in doc/developer-notes.md:1065 in dd0941cc6d outdated

1062 | @@ -1063,7 +1063,7 @@ bool Chainstate::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex)
1063 |  
1064 |  - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential
1065 |    deadlocks are introduced. As of 0.12, this is defined by default when

tdb3 commented at 11:25 AM on September 13, 2024:

nit: IMHO, could get rid of the "As of 0.12" part, since we're using CMake now and and 0.12 didn't use it.

kevkevinpal commented at 1:13 PM on September 13, 2024:

thanks! I removed that in 5a85022

in doc/multiprocess.md:26 in dd0941cc6d outdated

  22 | @@ -23,9 +23,9 @@ src/bitcoin-node -regtest -printtoconsole -debug=ipc
  23 |  BITCOIND=bitcoin-node test/functional/test_runner.py
  24 |  ```
  25 |  
  26 | -The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `--enable-multiprocess` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option).
  27 | +The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `-DWITH_MULTIPROCESS=ON` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option).

tdb3 commented at 11:30 AM on September 13, 2024:

nit: "configure script" is an Autotools thing, right?

kevkevinpal commented at 1:14 PM on September 13, 2024:

thanks! I updated this to cmake build instead of configure script in 5a85022

let me know if you'd prefer different wording!

in doc/multiprocess.md:28 in dd0941cc6d outdated

  22 | @@ -23,9 +23,9 @@ src/bitcoin-node -regtest -printtoconsole -debug=ipc
  23 |  BITCOIND=bitcoin-node test/functional/test_runner.py
  24 |  ```
  25 |  
  26 | -The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `--enable-multiprocess` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option).
  27 | +The configure script will pick up settings and library locations from the depends directory, so there is no need to pass `-DWITH_MULTIPROCESS=ON` as a separate flag when using the depends system (it's controlled by the `MULTIPROCESS=1` option).
  28 |  
  29 | -Alternately, you can install [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) packages on your system, and just run `./configure --enable-multiprocess` without using the depends system. The configure script will be able to locate the installed packages via [pkg-config](https://www.freedesktop.org/wiki/Software/pkg-config/). See [Installation](https://github.com/chaincodelabs/libmultiprocess/blob/master/doc/install.md) section of the libmultiprocess readme for install steps. See [build-unix.md](build-unix.md) and [build-osx.md](build-osx.md) for information about installing dependencies in general.
  30 | +Alternately, you can install [Cap'n Proto](https://capnproto.org/) and [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) packages on your system, and just run `cmake -B build -DWITH_MULTIPROCESS=ON` without using the depends system. The configure script will be able to locate the installed packages via [pkg-config](https://www.freedesktop.org/wiki/Software/pkg-config/). See [Installation](https://github.com/chaincodelabs/libmultiprocess/blob/master/doc/install.md) section of the libmultiprocess readme for install steps. See [build-unix.md](build-unix.md) and [build-osx.md](build-osx.md) for information about installing dependencies in general.

tdb3 commented at 11:31 AM on September 13, 2024:

Same here ("configure script")

kevkevinpal commented at 1:14 PM on September 13, 2024:

thanks! I updated this to cmake build instead of configure script in 5a85022

let me know if you'd prefer different wording!

tdb3 commented at 11:32 AM on September 13, 2024: contributor

Approach ACK

Thanks for the doc cleanup.

kevkevinpal renamed this:
~~doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes~~
doc: fixed inconsistencies in documentation between autotools to cmake change
on Sep 13, 2024

kevkevinpal force-pushed on Sep 13, 2024

kevkevinpal commented at 1:29 PM on September 13, 2024: contributor

It would be nice to fix all remaining instances in one go. See also #30664 (comment) and #30664 (comment)

There are still some changes I need to address from these comments

fanquake commented at 1:41 PM on September 13, 2024: member

Also in test_deterministic_coverage.sh: Run \"./configure --enable-lcov\"

DrahtBot removed the label CI failed on Sep 13, 2024

in doc/multiprocess.md:7 in 5a85022cc6 outdated

   3 | @@ -4,7 +4,7 @@ _This document describes usage of the multiprocess feature. For design informati
   4 |  
   5 |  ## Build Option
   6 |  
   7 | -On unix systems, the `--enable-multiprocess` build option can be passed to `./configure` to build new `bitcoin-node`, `bitcoin-wallet`, and `bitcoin-gui` executables alongside existing `bitcoind` and `bitcoin-qt` executables.
   8 | +On unix systems, the `-DWITH_MULTIPROCESS=ON` build option can be passed to `cmake -B build` to build new `bitcoin-node`, `bitcoin-wallet`, and `bitcoin-gui` executables alongside existing `bitcoind` and `bitcoin-qt` executables.

jonatack commented at 5:52 PM on September 13, 2024:

On Unix systems, the `-DWITH_MULTIPROCESS=ON` build option can be passed to build the supplemental `bitcoin-node` and `bitcoin-gui` multiprocess executables.

kevkevinpal commented at 5:21 PM on September 14, 2024:

thank you! Updated in 4f72eca

jonatack commented at 4:29 PM on September 16, 2024:

@ryanofsky bitcoin-wallet is built, if enabled, independently of the multiprocess flag (if I'm not confused); should doc/multiprocess.md describe this differently?

Configure summary
=================
Executables:
  bitcoind ............................ ON
  bitcoin-node (multiprocess) ......... OFF
  bitcoin-qt (GUI) .................... ON
  bitcoin-gui (GUI, multiprocess) ..... OFF
  bitcoin-cli ......................... ON
  bitcoin-tx .......................... ON
  bitcoin-util ........................ ON
  bitcoin-wallet ...................... ON
  bitcoin-chainstate (experimental) ... ON
  libbitcoinkernel (experimental) ..... ON

ryanofsky commented at 5:05 PM on September 16, 2024:

@ryanofsky bitcoin-wallet is built, if enabled, independently of the multiprocess flag (if I'm not confused); should doc/multiprocess.md describe this differently?

Probably previous description was a little confusing but current version c3d68e11191e036e154916f27f21200d2bf16756 which doesn't mention bitcoin-wallet looks good to me.

This readme was written as part of #10102 before bitcoin-wallet tool existed, so --enable-multiprocess option really did build a new binary that didn't exist before. But now #10102 just adds multiprocess functionality to the existing binary. In the master branch currently though multiproces option has no effect on bitcoin-wallet so it is best not to mention it.

jonatack commented at 6:14 PM on September 16, 2024:

That confirms what I thought when suggesting #30875 (review). Thanks @ryanofsky for the quick reply.

in doc/zmq.md:166 in 5a85022cc6 outdated

 162 | @@ -163,7 +163,7 @@ Note that for `*block` topics, when the block chain tip changes,
 163 |  a reorganisation may occur and just the tip will be notified.
 164 |  It is up to the subscriber to retrieve the chain from the last known
 165 |  block to the new tip. Also note that no notification will occur if the tip
 166 | -was in the active chain--as would be the case after calling invalidateblock RPC.
 167 | +was in the active chain, as would be the case after calling invalidateblock RPC.

jonatack commented at 5:56 PM on September 13, 2024:

was in the active chain, as would be the case after calling the `invalidateblock` RPC.

kevkevinpal commented at 5:21 PM on September 14, 2024:

thank you! Updated in 4f72eca

in src/qt/README.md:102 in 5a85022cc6 outdated

  98 | @@ -99,7 +99,7 @@ sudo apt-get install qtcreator
  99 |  #### Setup Qt Creator
 100 |  
 101 |  1. Make sure you've installed all dependencies specified in your systems build instructions
 102 | -2. Follow the compile instructions for your system, run `./configure` with the `--enable-debug` flag
 103 | +2. Follow the compile instructions for your system, run `cmake -B build` with the `-DCMAKE_BUILD_TYPE=Debug` flag

jonatack commented at 5:57 PM on September 13, 2024:

2. Follow the compile instructions for your system, adding the `-DCMAKE_BUILD_TYPE=Debug` build flag

kevkevinpal commented at 5:21 PM on September 14, 2024:

thank you! Updated in 4f72eca

in test/functional/test_runner.py:491 in 5a85022cc6 outdated

 487 | @@ -488,7 +488,7 @@ def main():
 488 |  
 489 |      if not enable_bitcoind:
 490 |          print("No functional tests to run.")
 491 | -        print("Rerun ./configure with --with-daemon and then make")
 492 | +        print("Rerun cmake -B build with -DBUILD_DAEMON=ON and then make")

jonatack commented at 5:58 PM on September 13, 2024:

        print("Re-compile with the -DBUILD_DAEMON=ON build option")

kevkevinpal commented at 5:21 PM on September 14, 2024:

thank you! Updated in 4f72eca

jonatack commented at 5:59 PM on September 13, 2024: member

Concept ACK

pablomartin4btc commented at 2:49 PM on September 14, 2024: member

Since you are here, you could update the translate instructions as well (I've tested them), if @hebasto agrees with the changes:

doc/translation_process.md

@@ -18,8 +18,8 @@ We use automated scripts to help extract translations in both Qt, and non-Qt sou

 To automatically regenerate the `bitcoin_en.ts` file, run the following commands:
 ```sh
-cd src/
-make translate
+cmake -B build -DWITH_BDB=ON -DBUILD_GUI=ON
+cmake --build build --target translate

**Example Qt translation**

doc/translation_strings_policy.md

@@ -97,4 +97,4 @@ The second example reduces the number of pluralized words that translators have

 During a string freeze (often before a major release), no translation strings are to be added, modified or removed.

-This can be checked by executing `make translate` in the `src` directory, then verifying that `bitcoin_en.ts` remains unchanged.
+This can be checked by building `translate` package with `cmake` ([instructions](/doc/translate_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.

kevkevinpal force-pushed on Sep 14, 2024

kevkevinpal commented at 5:21 PM on September 14, 2024: contributor

Also in test_deterministic_coverage.sh: Run \"./configure --enable-lcov\"

thank you! Updated in 4f72eca

kevkevinpal commented at 5:22 PM on September 14, 2024: contributor

Since you are here, you could update the translate instructions as well (I've tested them), if @hebasto agrees with the changes:

* `doc/translation_process.md`
  ```diff
  @@ -18,8 +18,8 @@ We use automated scripts to help extract translations in both Qt, and non-Qt sou
   
   To automatically regenerate the `bitcoin_en.ts` file, run the following commands:
   ```sh
  -cd src/
  -make translate
  +cmake -B build -DWITH_BDB=ON -DBUILD_GUI=ON
  +cmake --build build --target translate
   
  **Example Qt translation**
  ```

* `doc/translation_strings_policy.md`
  ```diff
  @@ -97,4 +97,4 @@ The second example reduces the number of pluralized words that translators have
   
   During a string freeze (often before a major release), no translation strings are to be added, modified or removed.
   
  -This can be checked by executing `make translate` in the `src` directory, then verifying that `bitcoin_en.ts` remains unchanged.
  +This can be checked by building `translate` package with `cmake` ([instructions](/doc/translate_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.
  ```

thank you! Updated in 4f72eca @hebasto if you disagree I can change them back!

kevkevinpal renamed this:
~~doc: fixed inconsistencies in documentation between autotools to cmake change~~
doc: fixed inconsistencies in documentation between autotools to cmake change and removed libtools from CI
on Sep 14, 2024

kevkevinpal commented at 5:27 PM on September 14, 2024: contributor

From these two comments these are the two that have not been addressed yet in this PR #30664 (comment) #30664 (comment)

test/get_previous_releases.py: './configure {}'.format(config_flags),
contrib/devtools/check-deps.sh:# Output makefile targets, converting library .a paths to libtool .la targets

kevkevinpal force-pushed on Sep 14, 2024

DrahtBot added the label CI failed on Sep 14, 2024

DrahtBot commented at 5:30 PM on September 14, 2024: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/30149215873</sub>

<details><summary>Hints</summary>

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

kevkevinpal force-pushed on Sep 14, 2024

hebasto commented at 7:13 PM on September 14, 2024: member

@kevkevinpal

I didn't notice that #30902 overlaps with this PR. Feel free to pick all changes from #30902 to let me close it.

UPD. Please separate CI changes into a dedicated commit with the ci: prefix.

kevkevinpal force-pushed on Sep 16, 2024

kevkevinpal commented at 12:57 PM on September 16, 2024: contributor

@kevkevinpal

I didn't notice that #30902 overlaps with this PR. Feel free to pick all changes from #30902 to let me close it.

UPD. Please separate CI changes into a dedicated commit with the ci: prefix.

We can keep your PR for now and I just removed any overlapping changes in 9e4f9c9

DrahtBot removed the label CI failed on Sep 16, 2024

in depends/packages.md:155 in 9e4f9c98e3 outdated

 151 | @@ -152,8 +152,7 @@ Most autotools projects can be properly staged using:
 152 |  
 153 |  ## Build outputs:
 154 |  
 155 | -In general, the output of a depends package should not contain any libtool

fanquake commented at 3:56 PM on September 16, 2024:

This line can remain: #30902 (review).

kevkevinpal commented at 3:59 PM on September 16, 2024:

no problem removed in 03fd466 03fd466 03fd466

kevkevinpal force-pushed on Sep 16, 2024

fanquake commented at 4:05 PM on September 16, 2024: member

https://github.com/bitcoin/bitcoin/blob/37679b856ce183ec256c12a680b93ad53ed94da6/doc/design/libraries.md?plain=1#L11

kevkevinpal force-pushed on Sep 16, 2024

kevkevinpal commented at 4:30 PM on September 16, 2024: contributor

https://github.com/bitcoin/bitcoin/blob/37679b856ce183ec256c12a680b93ad53ed94da6/doc/design/libraries.md?plain=1#L11

Thanks! updated in c3d68e1

kevkevinpal force-pushed on Sep 16, 2024

kevkevinpal renamed this:
~~doc: fixed inconsistencies in documentation between autotools to cmake change and removed libtools from CI~~
doc: fixed inconsistencies in documentation between autotools to cmake change
on Sep 16, 2024

in doc/developer-notes.md:434 in dc2e223103 outdated

 428 | @@ -429,9 +429,9 @@ Defining `DEBUG_LOCKCONTENTION` adds a "lock" logging category to the logging
 429 |  RPC that, when enabled, logs the location and duration of each lock contention
 430 |  to the `debug.log` file.
 431 |  
 432 | -The `--enable-debug` configure option adds `-DDEBUG_LOCKCONTENTION` to the
 433 | +The `-DCMAKE_BUILD_TYPE=Debug` build option adds `-DDEBUG_LOCKCONTENTION` to the
 434 |  compiler flags. You may also enable it manually for a non-debug build by running
 435 | -configure with `-DDEBUG_LOCKCONTENTION` added to your CPPFLAGS,
 436 | +build with `-DDEBUG_LOCKCONTENTION` added to your CPPFLAGS,

jonatack commented at 8:48 PM on September 16, 2024:

s/for a non-debug build by running build/by building/

kevkevinpal commented at 12:04 AM on September 18, 2024:

thank you! Addressed this in bc4a929

in doc/translation_strings_policy.md:96 in dc2e223103 outdated

  92 | @@ -97,4 +93,4 @@ The second example reduces the number of pluralized words that translators have
  93 |  
  94 |  During a string freeze (often before a major release), no translation strings are to be added, modified or removed.
  95 |  
  96 | -This can be checked by executing `make translate` in the `src` directory, then verifying that `bitcoin_en.ts` remains unchanged.
  97 | +This can be checked by building `translate` package with `cmake` ([instructions](translation_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.

jonatack commented at 8:48 PM on September 16, 2024:

This can be checked by building the `translate` package with `cmake` ([instructions](translation_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.

(nano-nit, "cmake" is written without the backticks in other documents; could be consistent, also regarding referring to it as "cmake" or "CMake", but feel free to ignore)

kevkevinpal commented at 12:05 AM on September 18, 2024:

thank you! Addressed this in bc4a929

I will take a look at the backtick nit, to see if I can find already existing instances of cmake with no backticks

in test/functional/test_runner.py:491 in dc2e223103 outdated

 487 | @@ -488,7 +488,7 @@ def main():
 488 |  
 489 |      if not enable_bitcoind:
 490 |          print("No functional tests to run.")
 491 | -        print("Rerun ./configure with --with-daemon and then make")
 492 | +        print("Re-compile  with the -DBUILD_DAEMON=ON build option")

jonatack commented at 8:49 PM on September 16, 2024:

        print("Re-compile with the -DBUILD_DAEMON=ON build option")

kevkevinpal commented at 12:05 AM on September 18, 2024:

thank you! Addressed this in bc4a929

davidgumberg commented at 2:09 AM on September 17, 2024: contributor

Concept ACK

Nit: could also fix

--- a/test/util/test_runner.py
+++ b/test/util/test_runner.py
@@ -5,7 +5,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test framework for bitcoin utils.

-Runs automatically during `make check`.
+Runs automatically during `ctest --test-dir build/`.

 Can also be run manually."""

kevkevinpal force-pushed on Sep 18, 2024

kevkevinpal commented at 12:06 AM on September 18, 2024: contributor

Concept ACK

Nit: could also fix

--- a/test/util/test_runner.py
+++ b/test/util/test_runner.py
@@ -5,7 +5,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test framework for bitcoin utils.

-Runs automatically during `make check`.
+Runs automatically during `ctest --test-dir build/`.

 Can also be run manually."""

thank you! Addressed this in bc4a929

davidgumberg commented at 12:25 AM on September 18, 2024: contributor

ACK https://github.com/bitcoin/bitcoin/commit/bc4a929cd716cd2b412c70754749d4fda0ca2a10

pico-nit: your branch messes up the 80 column wrapping in a few spots in doc/developer-notes.md. This is not really consistent through the docs anyways, so I don't mind personally.

DrahtBot requested review from jonatack on Sep 18, 2024

DrahtBot requested review from pablomartin4btc on Sep 18, 2024

DrahtBot requested review from tdb3 on Sep 18, 2024

DrahtBot added the label CI failed on Sep 18, 2024

maflcko commented at 5:55 AM on September 18, 2024: member

CI failure looks unrelated (like an old RPC shutdown bug in 0.16.3). https://cirrus-ci.com/task/6654771535282176?logs=ci#L2818

in doc/translation_process.md:21 in bc4a929cd7 outdated

  17 | @@ -18,8 +18,8 @@ We use automated scripts to help extract translations in both Qt, and non-Qt sou
  18 |  
  19 |  To automatically regenerate the `bitcoin_en.ts` file, run the following commands:
  20 |  ```sh
  21 | -cd src/
  22 | -make translate
  23 | +cmake -B build -DWITH_BDB=ON -DBUILD_GUI=ON

maflcko commented at 7:07 AM on September 18, 2024:

This should use the preset dev-mode. Otherwise, deps may be missed.

kevkevinpal commented at 3:05 PM on September 18, 2024:

thanks! Updated in a9964c0

in doc/translation_strings_policy.md:96 in bc4a929cd7 outdated

  92 | @@ -97,4 +93,4 @@ The second example reduces the number of pluralized words that translators have
  93 |  
  94 |  During a string freeze (often before a major release), no translation strings are to be added, modified or removed.
  95 |  
  96 | -This can be checked by executing `make translate` in the `src` directory, then verifying that `bitcoin_en.ts` remains unchanged.
  97 | +This can be checked by building the `translate` package with `cmake` ([instructions](translation_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.

maflcko commented at 7:07 AM on September 18, 2024:

This can be checked by building the `translate` target with `cmake` ([instructions](translation_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.

kevkevinpal commented at 3:05 PM on September 18, 2024:

thanks! Updated in a9964c0

in src/util/fs_helpers.cpp:25 in bc4a929cd7 outdated

  21 | @@ -22,7 +22,7 @@
  22 |  #include <utility>
  23 |  
  24 |  #ifndef WIN32
  25 | -// for posix_fallocate, in configure.ac we check if it is present after this
  26 | +// for posix_fallocate, in cmake -B build we check if it is present after this

maflcko commented at 7:12 AM on September 18, 2024:

// for posix_fallocate, in cmake/introspection.cmake we check if it is present after this

kevkevinpal commented at 3:05 PM on September 18, 2024:

thanks! Updated in a9964c0

maflcko approved

maflcko commented at 7:13 AM on September 18, 2024: member

review ACK bc4a929cd716cd2b412c70754749d4fda0ca2a10

doc: Updating docs from autotools to cmake

replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes
replaced --enable-multiprocess with -DWITH_MULTIPROCESS=ON
replaced --disable-zmq with -DWITH_ZMQ=OFF

a9964c0444

kevkevinpal force-pushed on Sep 18, 2024

kevkevinpal commented at 3:06 PM on September 18, 2024: contributor

ACK bc4a929

pico-nit: your branch messes up the 80 column wrapping in a few spots in doc/developer-notes.md. This is not really consistent through the docs anyways, so I don't mind personally.

thanks for the suggestion! But I think I'll leave those for now since they don't relate to the autotools to cmake update too much and I would prefer to keep the PR more focused on those changes

maflcko commented at 3:07 PM on September 18, 2024: member

ACK a9964c04447745435747d9cc557165c43902783b

jonatack commented at 4:01 PM on September 18, 2024: member

utACK a9964c04447745435747d9cc557165c43902783b

jarolrod approved

jarolrod commented at 4:25 PM on September 18, 2024: member

ACK a9964c04447745435747d9cc557165c43902783b

Tried to find other inconsistencies, but couldn't.

DrahtBot removed the label CI failed on Sep 18, 2024

tdb3 approved

tdb3 commented at 4:57 PM on September 18, 2024: contributor

ACK a9964c04447745435747d9cc557165c43902783b

pablomartin4btc commented at 5:20 PM on September 18, 2024: member

re-ACK a9964c04447745435747d9cc557165c43902783b

fanquake merged this on Sep 18, 2024

fanquake closed this on Sep 18, 2024

fanquake referenced this in commit a9f6a57b69 on Oct 8, 2024

TheCharlatan referenced this in commit 8bb47d4c2c on Nov 2, 2024

bitcoin locked this on Sep 18, 2025