8000 doc: fixed inconsistencies in documentation between autotools to cmake change by kevkevinpal · Pull Request #30875 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 18, 2024
Merged

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

merged 1 commit into from
Sep 18, 2024

Conversation

kevkevinpal
Copy link
Contributor
@kevkevinpal kevkevinpal commented Sep 12, 2024 •
edited
Loading

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
Copy link
Contributor
DrahtBot commented Sep 12, 2024 •
edited
Loading

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 DrahtBot added the Docs label Sep 12, 2024
@maflcko
Copy link
Member
maflcko commented Sep 12, 2024

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

@kevkevinpal kevkevinpal force-pushed the devNotesDebugBuild branch 2 times, most recently from 5c73aee to dd0941c Compare September 12, 2024 12:29
pablomartin4btc
pablomartin4btc reviewed Sep 13, 2024
View reviewed changes
Copy link
Member
@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dd0941c

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

@maflcko
Copy link
Member
maflcko commented Sep 13, 2024

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?

fanquake
fanquake reviewed Sep 13, 2024
View reviewed changes
doc/multiprocess.md
@@ -23,9 +23,9 @@ src/bitcoin-node -regtest -printtoconsole -debug=ipc
BITCOIND=bitcoin-node test/functional/test_runner.py
```

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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Thoughts @ryanofsky ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #30875 (comment)

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tdb3
tdb3 reviewed Sep 13, 2024
View reviewed changes
Copy link
Contributor
@tdb3 tdb3 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK

Thanks for the doc cleanup.

@kevkevinpal kevkevinpal changed the title doc: replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes doc: fixed inconsistencies in documentation between autotools to cmake change Sep 13, 2024
@kevkevinpal
Copy link
Contributor Author

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
Copy link
Member

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

jonatack
jonatack reviewed Sep 13, 2024
View reviewed changes
Copy link
Member
@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

This was referenced Sep 14, 2024
Merged
Open
@pablomartin4btc
Copy link
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 kevkevinpal force-pushed the devNotesDebugBuild branch 2 times, most recently from ea8ffed to 4f72eca Compare September 14, 2024 17:20
@kevkevinpal
Copy link
Contributor Author

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

thank you! Updated in 4f72eca

@kevkevinpal
Copy link
Contributor Author

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 kevkevinpal changed the title 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 Sep 14, 2024
@kevkevinpal
Copy link
Contributor Author

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

@DrahtBot
Copy link
Contributor

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

Hints

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.

@hebasto
Copy link
Member
hebasto commented Sep 14, 2024 •
edited
Loading

@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 kevkevinpal changed the title 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 Sep 16, 2024
jonatack
jonatack reviewed Sep 16, 2024
View reviewed changes
jonatack
jonatack reviewed Sep 16, 2024
View reviewed changes
doc/translation_strings_policy.md Outdated
@@ -97,4 +93,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](translation_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.
Copy link
Member
@jonatack jonatack Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This can be checked by building `translate` package with `cmake` ([instructions](translation_process.md)), then verifying that `bitcoin_en.ts` remains unchanged.
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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jonatack
jonatack reviewed Sep 16, 2024
View reviewed changes
@davidgumberg
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor
davidgumberg commented Sep 18, 2024 •
edited
Loading

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.

@maflcko
Copy link
Member
maflcko commented Sep 18, 2024

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

@maflcko maflcko mentioned this pull request Sep 18, 2024
Merged
maflcko
maflcko approved these changes Sep 18, 2024
View reviewed changes
@maflcko
Copy link
Member
maflcko commented Sep 18, 2024

review ACK bc4a929

@kevkevinpal
doc: Updating docs from autotools to cmake