ci: Migrate CI scripts to CMake by hebasto · Pull Request #142 · hebasto/bitcoin

hebasto · 2024-04-05T16:19:38Z

For the same changes in the master branch with CI logs, please see bitcoin#29790.

Still no Android-related changes.

hebasto · 2024-04-05T16:21:12Z

@maflcko Want to look into this PR?

fanquake

Had a quick look over.

hebasto · 2024-04-06T12:40:10Z

#142 (comment) has been addressed.

See Cirrus CI logs in bitcoin#29790 -- https://cirrus-ci.com/build/4887272447803392.

hebasto · 2024-04-06T17:23:28Z

Rebased.

maflcko · 2024-04-09T08:18:17Z

 ccache --zero-stats
 PRINT_CCACHE_STATISTICS="ccache --version | head -n 1 && ccache --show-stats"

+mkdir -p "${BASE_BUILD_DIR}"


nit: Would be nice to keep the HOST or CONTAINER_NAME in the build dir, just as a redundancy for some more context?

Thanks! Done.

An excerpt from https://cirrus-ci.com/task/5556699505885184:

-- Build files have been written to: /ci_container_base/ci/scratch/build-i686-pc-linux-gnu

hebasto · 2024-05-17T13:03:15Z

Rebased.

The last commit is bitcoin@c06b2bb cherry-picked from the bitcoin#29790. So reviewers can inspect CI logs there.

It follows the following approach:

... we agreed on using APPEND_*FLAGS variables as the last resort only, without advertising them to users and attempting to use native CMake's variables as wide as possible...

@maflcko Do you mind having another look at this PR please?

maflcko · 2024-05-17T13:21:21Z

lgtm, I guess.

Considering that APPEND_ is a hidden option for power users only, and the CI uses it in a few places as power user, that seems fine.

hebasto · 2024-05-17T20:59:33Z

@fanquake requested offline to:

... take the compiler flags out of the CC and CXX defines.

Done. This PR and bitcoin#29790 have been updated simultaneously.

hebasto · 2024-05-18T07:44:23Z

Addressed @theuni's #142 (comment).

This PR and bitcoin#29790 have been updated simultaneously.

sedited · 2024-05-20T09:43:14Z


 export CONTAINER_NAME=ci_native_asan
-export PACKAGES="systemtap-sdt-dev clang-18 llvm-18 libclang-rt-18-dev python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libqrencode-dev libsqlite3-dev ${BPFCC_PACKAGE}"
+export PACKAGES="systemtap-sdt-dev clang-18 llvm-18 libclang-rt-18-dev python3-zmq qtbase5-dev qttools5-dev qttools5-dev-tools libevent-dev libboost-dev libdb5.3++-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev libqrencode-dev libsqlite3-dev ${BPFCC_PACKAGE}"


Looks like this redundant?

It follows the build docs:

bitcoin/doc/build-unix.md

Line 84 in ecd2365

sudo apt-get install qtbase5-dev qttools5-dev qttools5-dev-tools

Otherwise, CMakes fails to find Qt5LinguistTools.

Right, please resolve.

I guess the change could also be submitted upstream standalone, maybe along with similar changes that are unrelated to cmake? But either way seems fine.

sedited · 2024-05-20T10:31:08Z

 export GOAL="install"
 # _FORTIFY_SOURCE is not compatible with MSAN.
-export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,memory CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE -U_FORTIFY_SOURCE'"
+export BITCOIN_CONFIG="-DFUZZ=ON -DSANITIZERS=fuzzer,memory -DAPPEND_CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE -U_FORTIFY_SOURCE'"


Are we using the APPEND_*FLAGS here just because setting -DCMAKE_CXX_FLAGS would clobber the CXXFLAGS?

No.

-DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE' is required to override -D_FORTIFY_SOURCE=3 from

bitcoin/CMakeLists.txt

Lines 505 to 507 in 0578462

target_compile_options(hardening_interface INTERFACE

$<$<NOT:$<CONFIG:Debug>>:-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3>

)

FWIW, there was bitcoin#30140 in the main repository.

I guess the answer here is that given how hardening has been implemented in the CMake branch, it's impossible to override any of the flags using the proper CMake vars?

I guess the answer here is that given how hardening has been implemented in the CMake branch, it's impossible to override any of the flags using the proper CMake vars?

That's correct.

That seems like a problem in it's own right, but I guess can be followed up with separately.

Isn't this the exact use case we added the APPEND flags for, though?

We'll probably just need to document which flags happen to only work with those flags in some way, because at the moment, it's buried in CMake implementation detail.

On second glance, -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE is going into DCMAKE_CXX_FLAGS in most other places. If nothing else, we should be consistent with where we stick vars to avoid confusion.

On second glance, -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE is going into DCMAKE_CXX_FLAGS in most other places. If nothing else, we should be consistent with where we stick vars to avoid confusion.

Done.

sedited · 2024-05-20T10:48:11Z

 )
+
+set_target_properties(leveldb PROPERTIES
+  EXPORT_COMPILE_COMMANDS OFF


Why do we need to set this for some of our targets?

By default, when EXPORT_COMPILE_COMMANDS is ON, CMake generates a compile_commands.json, which is used in the "tidy" CI task. On the master branch, compile_commands.json is generated by the bear tool.

We do not want to analyze code from subtrees, so disabling EXPORT_COMPILE_COMMANDS in there.

How would a user work around this if they want to analyze code from subtrees?

Isn't it supposed to be done in upstream repositories?

sedited · 2024-05-20T11:47:36Z

Changes seem sane to me, but given this changes much of the CI I might be missing something.

hebasto · 2024-05-20T12:49:52Z

I'm going to merge this PR shortly.

theuni · 2024-05-20T17:27:49Z

-export BITCOIN_CONFIG="--enable-debug CC='clang -m32' CXX='clang++ -m32' \
-CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE' CXXFLAGS='-Wno-error=documentation'"
+export BITCOIN_CONFIG="-DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang;-m32' -DCMAKE_CXX_COMPILER='clang++;-m32' \
+-DCMAKE_CXX_FLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE -Wno-error=documentation'"


Are these not going into APPEND flags because we don't add -Wdocumentation ourselves? Meaning that the primary use-case for APPEND_CXXFLAGS is specifically when we:

Add a flag in our own buildsystem

Want that flag turned off by the user at build-time

?

If so, it would be nice to document exactly that.

Wait, we do add -Wdocumentation though. So why doesn't this have to be in APPEND_CXXFLAGS?

We have to use APPEND_CXXFLAGS for -Wno-documentation.

-Wno-error=documentation works with CMAKE_CXX_FLAGS just fine.

Ah, ok. I would've thought that order matters there. But that makes sense.

theuni · 2024-05-20T17:41:49Z

Agree with what @TheCharlatan said. I left a few nits, but this is a pretty huge change and I don't feel that I can review the whole thing and compare it to master with much confidence. I'm ok merging it (after addressing comments) and dealing with any fallout in a follow-up.

hebasto · 2024-05-20T17:51:14Z

@theuni's #142 (comment) has been addressed.

hebasto · 2024-05-20T18:01:28Z

I'm ok merging it (after addressing comments) and dealing with any fallout in a follow-up.

bitcoin#29790 will continue to run the CI for the cmake-staging branch. So we'll be able to observe any overlooked problem.

hebasto added the enhancement New feature or request label Apr 5, 2024