ci: Migrate CI scripts to CMake #142
Conversation
|
@maflcko Want to look into this PR? |
|
#142 (comment) has been addressed. See Cirrus CI logs in bitcoin#29790 -- https://cirrus-ci.com/build/4887272447803392. |
|
Rebased. |
| fi | ||
|
|
||
| ccache --zero-stats | ||
| PRINT_CCACHE_STATISTICS="ccache --version | head -n 1 && ccache --show-stats" | ||
|
|
||
| mkdir -p "${BASE_BUILD_DIR}" |
There was a problem hiding this comment.
nit: Would be nice to keep the HOST or CONTAINER_NAME in the build dir, just as a redundancy for some more context?
There was a problem hiding this comment.
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
force-pushed
the
240405-cmake-DG
branch
from
e3c0071
to
bd3f34c
Compare
hebasto
force-pushed
the
240405-cmake-DG
branch
from
bd3f34c
to
4fe49f6
Compare
hebasto
force-pushed
the
cmake-staging
branch
2 times, most recently
from
f918ccb
to
22270ee
Compare
Restore `ci.yml`
|
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:
@maflcko Do you mind having another look at this PR please? |
|
lgtm, I guess. Considering that |
|
@fanquake requested offline to:
Done. This PR and bitcoin#29790 have been updated simultaneously. |
|
Addressed @theuni's #142 (comment). This PR and bitcoin#29790 have been updated simultaneously. |
| @@ -17,10 +17,11 @@ else | |||
| fi | |||
|
|
|||
| 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}" | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
| @@ -18,7 +18,7 @@ export PACKAGES="ninja-build" | |||
| export DEP_OPTS="DEBUG=1 NO_BDB=1 NO_QT=1 CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'" | |||
| 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'" | |||
There was a problem hiding this comment.
Are we using the APPEND_*FLAGS here just because setting -DCMAKE_CXX_FLAGS would clobber the CXXFLAGS?
There was a problem hiding this comment.
No.
-DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE' is required to override -D_FORTIFY_SOURCE=3 from
Lines 505 to 507 in 0578462
There was a problem hiding this comment.
FWIW, there was bitcoin#30140 in the main repository.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That seems like a problem in it's own right, but I guess can be followed up with separately.
There was a problem hiding this comment.
Isn't this the exact use case we added the APPEND flags for, though?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
On second glance,
-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODEis going intoDCMAKE_CXX_FLAGSin most other places. If nothing else, we should be consistent with where we stick vars to avoid confusion.
Done.
| @@ -99,3 +99,7 @@ target_link_libraries(leveldb PRIVATE | |||
| nowarn_leveldb_interface | |||
| crc32c | |||
| ) | |||
|
|
|||
| set_target_properties(leveldb PROPERTIES | |||
| EXPORT_COMPILE_COMMANDS OFF | |||
There was a problem hiding this comment.
Why do we need to set this for some of our targets?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
How would a user work around this if they want to analyze code from subtrees?
There was a problem hiding this comment.
Isn't it supposed to be done in upstream repositories?
|
Changes seem sane to me, but given this changes much of the CI I might be missing something. |
|
I'm going to merge this PR shortly. |
| 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'" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Wait, we do add -Wdocumentation though. So why doesn't this have to be in APPEND_CXXFLAGS?
There was a problem hiding this comment.
We have to use APPEND_CXXFLAGS for -Wno-documentation.
-Wno-error=documentation works with CMAKE_CXX_FLAGS just fine.
There was a problem hiding this comment.
Ah, ok. I would've thought that order matters there. But that makes sense.
|
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. |
|
@theuni's #142 (comment) has been addressed. |
bitcoin#29790 will continue to run the CI for the |
For the same changes in the master branch with CI logs, please see bitcoin#29790.
Still no Android-related changes.