-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
build: Add CMake-based build system (8 of N) #18
Conversation
Friendly ping @TheCharlatan @theuni :) |
Does it make sense to encapsulate platform-specific preprocessor definitions, compiler and linker flags into an interface library, such as |
Prefixes of the parsed function arguments have been made abbreviated to improve code readability. |
add16ca
to
e947768
Compare
Dropped unneeded locale normalization in the |
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick review of the first 3 commits.
The 4th one is going to take more time and energy :)
cmake/module/TryAppendCXXFlags.cmake
Outdated
if(MSVC) | ||
set(CMAKE_REQUIRED_FLAGS "${flags} /WX") | ||
else() | ||
set(CMAKE_REQUIRED_FLAGS "${flags} -Werror") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to remain an option to be passed in, similar to our autotools usage. 2 reasons:
- The user might have passed in something stupid:
cmake .. -DCMAKE_CXX_FLAGS="-Wbadflag"
- A warning might not be an error
In the first case, every check will fail. This is not true in autotools because "-Werror" is only set after it has been tested itself. Here, it's hard-coded as working.
The second case is much more common. Imagine a compiler option that spews a deprecation message, but still works just fine. In that case, we'd want the ability to not pass in "Werror".
Edit: corrected "warning might not be an error".
|
||
if(MSVC) | ||
set(${linker_flags_var} ${flag} /WX) | ||
elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- These are specific to the linker, not the platform.
- The Darwin platform is in flux (moving to lld)
- ld64 may be used on other platforms (BSDs)
So... I'm requesting as usual: please don't hard-code platforms like these. Test the flags and apply them if they work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see "Werror" comment about making this an option. We don't always want fatal warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at our current impl, I see now you've mostly just ported over the old behavior.
Much of this seems quite janky to me, but I'll stop complaining about current behaviors. I might upstream a few changes though, and let you pick them up in a rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at our current impl, I see now you've mostly just ported over the old behavior.
Right. As we all were agree about that for the staging branch, IIRC.
Much of this seems quite janky to me, but I'll stop complaining about current behaviors. I might upstream a few changes though, and let you pick them up in a rebase.
Great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- ld64 may be used on other platforms (BSDs)
FWIW, I've successfully tested this PR on FreeBSD 13.2 and OpenBSD 7.1 using the default toolchains.
add_compile_definitions($<$<CONFIG:Debug>:ABORT_ON_FAILED_ASSUME>) | ||
# We leave assertions on. | ||
if(MSVC) | ||
remove_c_flag_from_all_configs(/DNDEBUG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a CPPFLAG (preprocessor) flag, not a cflag/cxxflag and not c/cxx specific. Surely it's possible to remove it from the list of definitions instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a CPPFLAG (preprocessor) flag, not a cflag/cxxflag and not c/cxx specific.
Agree :)
Surely it's possible to remove it from the list of definitions instead?
This is all about CMake's default platform-specific CMAKE_<LANG>_FLAGS_<CONFIG>_INIT
variables. There are no preprocessor variable among them. And CMake indeed sets initial preprocessor definitions in the mentioned above language-specific variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
# Prefer -O2 optimization level. (-O3 is CMake's default for Release for many compilers.) | ||
replace_c_flag_in_config(Release -O3 -O2) | ||
replace_cxx_flag_in_config(Release -O3 -O2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this means the user has no way to override and is stuck with "-O2" builds ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, an issue with the user's opportunities to override build system options described in details in bitcoin-core/secp256k1#1249.
if(compiler_supports_O0) | ||
string(STRIP "${debug_c_flags} -O0" debug_c_flags) | ||
endif() | ||
try_append_cxx_flags(debug_cxx_flags "-g3" RESULT_VAR compiler_supports_g3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "-g" flags should be on for Debug + RelWithDebInfo, off for MinSizeRel/Release. But it appears they're applied here unconditionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, though your multi-config output looks as I'd expect. I must be misunderstanding something. Ignore the above comment for now.
The following @theuni's comments have been addressed: |
The PR description has been updated with an cross-compiling example. |
ac7bc5b [FIXUP] Rename CCACHE_EXECUTABLE --> CCACHE_COMMAND for consistency (Hennadii Stepanov) 0476509 [FIXUP] Learn to work with recent ccache in MSVC builds (Hennadii Stepanov) 2fd67c7 [FIXUP] Do not disable `TrackFileAccess` in MSVC builds (Hennadii Stepanov) a53ae12 [FIXUP] Use Multi-ToolTask in MSVC builds by default (Hennadii Stepanov) Pull request description: The parent PR: bitcoin#25797. The previous PRs in the staging branch: #5, #6, #7, #10, #13, #15, #17, #18. This PR consists of fixups related to using [Ccache](https://ccache.dev/) in MSVC builds. Top commit has no ACKs. Tree-SHA512: dc01202b054aaeb5b46607bc93126bee0df523df3b4f2df54b566b2e2d6306d784a723fd4a999d0d84fdf31e926a72304790f94b9d57034db0c112ee9f52070e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left two style-only nits
cmake/module/TryAppendCXXFlags.cmake
Outdated
|
||
if(TACXXF_SOURCE) | ||
set(source "${TACXXF_SOURCE}") | ||
unset(${result} CACHE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this removed from the cache in case the function was previously run with a RESULT_VAR
argument and is now run with a SOURCE
argument instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://cmake.org/cmake/help/latest/module/CheckCXXSourceCompiles.html:
The check is only performed once, with the result cached in the variable named by
<resultVar>
. Every subsequent CMake run will re-use this cached value rather than performing the check again, even if the<code>
changes. In order to force the check to be re-evaluated, the variable named by<resultVar>
must be manually removed from the cache.
We are interested in performing checks whenever the tested source code is changed. This occurs when the SOURCE
argument is added, omitted after being used before, or modified.
And it has nothing about the RESULT_VAR
argument.
Reworked, because the previous branch version had a bug: skipping a check when the SOURCE
argument is omitted after being used before.
@@ -0,0 +1,126 @@ | |||
# Copyright (c) 2023 The Bitcoin Core developers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this entire file (excluding the printing) all just coping with -DNDEBUG
? If so I'm wondering if there isn't an easier way...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this entire file (excluding the printing) all just coping with
-DNDEBUG
?
This file's goal (excluding the printing) is to adjust CMake's default platform-specific flags to our needs, particularly, to drop the NDEBUG
preprocessor macro, lower optimization level from -O3
to -O2
, and so on.
If so I'm wondering if there isn't an easier way...
I'll be happy with any suggestion to implement :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can define our custom default flags, which would override CMake's internal logic. However, I'd prefer to avoid this approach.
CMakeLists.txt
Outdated
try_append_linker_flag(mingw_linker_flags "-Wl,--major-subsystem-version,6") | ||
try_append_linker_flag(mingw_linker_flags "-Wl,--minor-subsystem-version,1") | ||
separate_arguments(mingw_linker_flags) | ||
add_link_options(${mingw_linker_flags}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a regression to set global flags here. In master we set these to CORE_LDFLAGS
, which we pretty much use everywhere, but we can still opt-out.
I suspect we'll want to avoid setting any globals like this, in favor of manually applying flags where necessary.
How about making try_append_linker_flag
take an output variable, to which it appends the flag if it succeeds? Pretty much how we do it in autoconf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what I was asking in #18 (comment) :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f365f4b
to
c594ac3
Compare
Addressed @MarcoFalke's and @TheCharlatan's comments.
|
30df9fa
to
04de82d
Compare
@TheCharlatan @theuni @MarcoFalke All your comments have been addressed. Also I had to rebase due to the conflict with #19. Hoping on your final (?) review :) |
04de82d
to
a65da0d
Compare
Updated 04de82d -> a65da0d (cmake18.09 -> cmake18.10, diff):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a65da0d to keep this moving. This has been sitting for too long :(
Thanks @hebasto for making the high-level review requests. I like the sanity checks too :)
I find myself getting bogged down by the details and corner-cases, which is probably not useful for this round of review. For example, I think it's safe to just assume TryAppendCXXFlags
and friends work as expected for now.
@@ -73,6 +73,10 @@ if(NOT CMAKE_OBJCXX_COMPILER) | |||
endif() | |||
set(CMAKE_OBJCXX_FLAGS_INIT "${DEPENDS_OBJCXX_COMPILER_FLAGS} @CPPFLAGS@ @CXXFLAGS@") | |||
|
|||
if(CMAKE_SYSTEM_NAME STREQUAL "Windows") | |||
set(CMAKE_EXE_LINKER_FLAGS_INIT "-static") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this moved to depends?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using depends for Windows means the user cross-build on Linux. To be able to test binaries with Wine, they must be linked statically, no?
On the other hand, building on Windows natively does not require static linking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using depends for Windows means the user cross-build on Linux.
Yikes, please drop that assumption. I've maintained throughout review here that it makes more sense to me for MSVC to be using depends than a relying on external packagers. It's fine if that's not the default way, but please don't rule it out either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this moved to depends?
Reworked in #31.
@@ -280,3 +280,17 @@ if(configure_warnings) | |||
endforeach() | |||
message(" ******\n") | |||
endif() | |||
|
|||
# We want all build properties to be encapsulated properly. | |||
get_directory_property(global_compile_definitions COMPILE_DEFINITIONS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanity checking -- Love it!
…tifications fuzz target fab164f fuzz: Avoid signed-integer-overflow in wallet_notifications fuzz target (MarcoFalke) Pull request description: Should avoid ``` policy/feerate.cpp:29:63: runtime error: signed integer overflow: 77600710321911316 * 149 cannot be represented in type 'int64_t' (aka 'long') #0 0x563a1775ed66 in CFeeRate::GetFee(unsigned int) const src/policy/feerate.cpp:29:63 #1 0x563a15913a69 in wallet::COutput::COutput(COutPoint const&, CTxOut const&, int, int, bool, bool, bool, long, bool, std::optional<CFeeRate>) src/./wallet/coinselection.h:91:57 #2 0x563a16fa6a6d in wallet::FetchSelectedInputs(wallet::CWallet const&, wallet::CCoinControl const&, wallet::CoinSelectionParams const&) src/wallet/spend.cpp:297:17 #3 0x563a16fc4512 in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1105:33 #4 0x563a16fbec74 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1291:16 #5 0x563a16fcf6df in wallet::FundTransaction(wallet::CWallet&, CMutableTransaction&, long&, int&, bilingual_str&, bool, std::set<int, std::less<int>, std::allocator<int>> const&, wallet::CCoinControl) src/wallet/spend.cpp:1361:16 #6 0x563a1597b7b9 in wallet::(anonymous namespace)::FuzzedWallet::FundTx(FuzzedDataProvider&, CMutableTransaction) src/wallet/test/fuzz/notifications.cpp:162:15 #7 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0::operator()() const src/wallet/test/fuzz/notifications.cpp:228:23 #8 0x563a15958240 in unsigned long CallOneOf<wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1>(FuzzedDataProvider&, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1) src/./test/fuzz/util.h:43:27 #9 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/notifications.cpp:196:9 #10 0x563a15fdef0c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #11 0x563a15fdef0c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 #12 0x563a158032a4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19822a4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #13 0x563a15802999 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1981999) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #14 0x563a15804586 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983586) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #15 0x563a15804aa7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983aa7) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #16 0x563a157f21fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19711fb) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #17 0x563a1581c766 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199b766) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #18 0x7f499e17b0cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #19 0x7f499e17b188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #20 0x563a157e70c4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19660c4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:29:63 in MS: 0 ; base unit: 0000000000000000000000000000000000000000 0x3f,0x0,0x2f,0x5f,0x5f,0x5f,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0xff,0xff,0xff,0xff,0xff,0x53,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x13,0x5e,0x5f,0x5f,0x8,0x25,0x0,0x5f,0x5f,0x5f,0x5f,0x5f,0x5f,0x8,0x25,0xca,0x7f,0x5f,0x5f,0x5f,0x13,0x13,0x5f,0x5f,0x5f,0x2,0xdb,0xca,0x0,0x0,0xe7,0xe6,0x66,0x65,0x0,0x0,0x0,0x0,0x44,0x3f,0xa,0xa,0xff,0xff,0xff,0xff,0xff,0x61,0x76,0x6f,0x69,0x0,0xb5,0x15, ?\000/___}}}}}}}}}}}}}}}}}}}}\377\377\377\377\377S\377\377\377\377\377\000\000\000\000\000\000\023^__\010%\000______\010%\312\177___\023\023___\002\333\312\000\000\347\346fe\000\000\000\000D?\012\012\377\377\377\377\377avoi\000\265\025 artifact_prefix='./'; Test unit written to ./crash-4d3bac8a64d4e58b2f0943e6d28e6e1f16328d7d Base64: PwAvX19ffX19fX19fX19fX19fX19fX19fX3//////1P//////wAAAAAAABNeX18IJQBfX19fX18IJcp/X19fExNfX18C28oAAOfmZmUAAAAARD8KCv//////YXZvaQC1FQ== ACKs for top commit: dergoegge: ACK fab164f brunoerg: ACK fab164f Tree-SHA512: f416828f4394aa7303ee437f141e9bbd23c0e0f1b830e4ef3932338858249ba68a811b9837c5b7ad8c6ab871b6354996434183597c1a910a8d8e8d829693e4b2
The parent PR: bitcoin#25797.
The previous PRs in the staging branch: #5, #6, #7, #10, #13, #15, #17, #19.
What is NEW:
EXAMPLES of configuration output on Ubuntu 22.04:
A cross-project note. The
ProcessConfigurations.cmake
is based on the same module that was suggested in bitcoin-core/secp256k1#1291. So, cross-reviewing is welcome :)