doc: update dev note examples for CMake

fanquake commented at 2:17 pm on August 28, 2024: member

Update the examples in the developer notes to work with CMake. Also added an explicit -DCMAKE_EXPORT_COMPILE_COMMANDS=ON for clarity.

DrahtBot commented at 2:17 pm on August 28, 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	TheCharlatan, jonatack, davidgumberg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Docs on Aug 28, 2024

doc: update dev note examples for CMake 7de0c99804

fanquake force-pushed on Aug 28, 2024

TheCharlatan approved

TheCharlatan commented at 4:18 pm on August 28, 2024: contributor

ACK 7de0c99804bca98ef159b7b778e6f5b602507d2c

jonatack commented at 5:05 pm on August 28, 2024: member

Tested ACK 7de0c99804bca98ef159b7b778e6f5b602507d2c on arm64 macOS 14.6.1

(modulo local issue with the third incantation that is unrelated to this change, with my machine not finding clang-tidy-diff zsh: command not found: clang-tidy-diff)

TheCharlatan commented at 8:31 pm on August 28, 2024: contributor

Do you want to add https://github.com/TheCharlatan/bitcoin/commit/17f6ecd1470c07a3bf26a08e6d1a55ed2f8486ca? I guess we should update all scripts and examples to use the build directory for executing the binaries? Edit: Ah, nevermind I missed #30741

in doc/developer-notes.md:238 in 7de0c99804

239 
240 To run clang-tidy on the changed source lines:
241 
242 ```sh
243-git diff | ( cd ./src/ && clang-tidy-diff -p2 -j $(nproc) )
244+git diff | ( cd ./src/ && clang-tidy-diff -p2 -path ../build -j $(nproc) )

davidgumberg commented at 0:03 am on August 29, 2024:

Out-of-scope-feel-free-to-disregard:

On Fedora 40, I don’t have the binary clang-tidy-diff, but I tested that the invocation added here works if I do:

0git diff | ( cd ./src/ && /usr/share/clang/clang-tidy-diff.py -p2 -path ../build -j $(nproc) )

maflcko commented at 4:05 am on August 29, 2024:

There will always be a distro where it doesn’t work and other command may be needed. Generally, I think the commands target Debian/Ubuntu. If they happen to work on all distros, great, and I am sure developers are able to figure out their distro’s way of calling the command.

in doc/developer-notes.md:225 in 7de0c99804

224 ```sh
225-./autogen.sh && ./configure CC=clang CXX=clang++
226-make clean && bear --config src/.bear-tidy-config -- make -j $(nproc)
227+cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
228+cmake --build build -j $(nproc)
229 ```

davidgumberg commented at 0:21 am on August 29, 2024:

Feel free to disregard alternative:

0Configure with clang as the compiler to produce the `compile_commands.json` that
1`clang-tidy` needs:
2
3```sh
4cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
5cmake --build build -j $(nproc)
6```

davidgumberg commented at 0:36 am on August 29, 2024: contributor

Tested ACK https://github.com/bitcoin/bitcoin/commit/7de0c99804bca98ef159b7b778e6f5b602507d2c

Used the invocation provided here on a clean git clone of bitcoin core, and these instructions worked for generating the needed compile_commands.json for clang-tidy, and the arguments provided here help clang-tidy find the compile commands.

This bit of the docs is also helpful for anyone wanting to use their ide/editor’s lsp client with the clangd server which also requires compile_commands.json.

clang-tidy-diff

I had the same issue with clang-tidy-diff as @jonatack, but I tested that the invocation provided here works with the out-of-$PATH-for-me clang-tidy-diff.py:

0git diff | ( cd ./src/ && /usr/share/clang/clang-tidy-diff.py -p2 -path ../build -j $(nproc) )

This actually seems like it might be partly llvm’s issue, as there’s a ~10 year old FIXME comment asking for the clang-tidy-diff.py stuff to get rolled into run-clang-tidy.

tdb3 commented at 2:47 am on August 29, 2024: contributor

Initially running into issues building with Debian (12.6) default clang 14.0.6 and with 18.1.8. Fresh clone. Hope to revisit soon. No issues seen with gcc (following https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#to-build).

0cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
1cmake --build build -j 18

0-- Found Boost: /usr/include (found suitable version "1.74.0", minimum required is "1.73.0")
1...
2$ clang --version
3Debian clang version 18.1.8 (++20240731024826+3b5b5c1ec4a3-1~exp1~20240731144843.145)
4Target: x86_64-pc-linux-gnu

With 18.1.8

 0In file included from /usr/include/boost/mpl/integral_c.hpp:32:
 1/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'udt_builtin_mixture_enum' [-Wenum-constexpr-conversion]
 2   73 |     typedef AUX_WRAPPER_INST( BOOST_MPL_AUX_STATIC_CAST(AUX_WRAPPER_VALUE_TYPE, (value - 1)) ) prior;
 3      |                               ^
 4/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
 5   24 | #   define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
 6      |                                               ^
 7In file included from ../../../src/test/validation_chainstatemanager_tests.cpp:8:
 8In file included from ../../../src/node/chainstatemanager_args.h:9:
 9In file included from ../../../src/validation.h:28:
10In file included from ../../../src/txmempool.h:26:
11In file included from /usr/include/boost/multi_index/hashed_index.hpp:38:
12In file included from /usr/include/boost/multi_index/detail/node_handle.hpp:22:
13In file included from /usr/include/boost/multi_index_container_fwd.hpp:18:
14In file included from /usr/include/boost/multi_index/indexed_by.hpp:17:
15In file included from /usr/include/boost/mpl/vector.hpp:36:
16In file included from /usr/include/boost/mpl/vector/vector20.hpp:18:
17In file included from /usr/include/boost/mpl/vector/vector10.hpp:18:
18In file included from /usr/include/boost/mpl/vector/vector0.hpp:24:
19In file included from /usr/include/boost/mpl/vector/aux_/clear.hpp:18:
20In file included from /usr/include/boost/mpl/vector/aux_/vector0.hpp:22:
21In file included from /usr/include/boost/mpl/vector/aux_/iterator.hpp:19:
22In file included from /usr/include/boost/mpl/plus.hpp:19:
23In file included from /usr/include/boost/mpl/aux_/arithmetic_op.hpp:17:
24In file included from /usr/include/boost/mpl/integral_c.hpp:32:
25/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'int_float_mixture_enum' [-Wenum-constexpr-conversion]
26/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
27   24 | #   define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
28      |                                               ^
292 errors generated.

maflcko commented at 7:09 am on August 29, 2024: member

/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type ‘udt_builtin_mixture_enum’ [-Wenum-constexpr-conversion]

That is just a boost issue, unrelated to cmake. It should happen with autotools as well. I think you can fix it by upgrading your distro (or boost)

fanquake commented at 9:51 am on August 29, 2024: member

Initially running into issues building

Opened an issue to track this: #30751 , as it needs fixing in some way (also for 28.x).

fanquake merged this on Aug 29, 2024

fanquake closed this on Aug 29, 2024

fanquake deleted the branch on Aug 29, 2024

Thompson1985 commented at 11:32 am on August 29, 2024: none

Thank you 👍

Thompson1985 commented at 4:10 am on August 30, 2024: none

Thank you

doc: update dev note examples for CMake #30739

Code Coverage

Reviews