cmake: Add user-defined APPEND_{CPP,C,CXX,LD}FLAGS

[hebasto]

This PR implements the CMake WG design decision made during the call on 2024-04-18.

$ cmake -B build -LH 2>&1 | grep -B 1 -e EXTRA_
// Extra C compiler flags.
EXTRA_CFLAGS:STRING=
--
// Extra (Objective) C/C++ preprocessor flags.
EXTRA_CPPFLAGS:STRING=
--
// Extra (Objective) C++ compiler flags.
EXTRA_CXXFLAGS:STRING=
--
// Extra linker flags.
EXTRA_LDFLAGS:STRING=

To test this PR, I suggest to observe raw build logs using the --verbose command-line option.

The related #93 is to be reworked on top of this PR.

[hebasto]

The recent push to bitcoin#29790 (the pr29790.21+cmake157 tag) demonstrates the new approach in CI scripts.

[vasild]

I tested these and they work as expected: use -DEXTRA_CPPFLAGS:STRING="-I/extracppflags" -DEXTRA_CFLAGS:STRING="-I/extracflags" -DEXTRA_CXXFLAGS:STRING="-I/extracxxflags" -DEXTRA_LDFLAGS:STRING="-L/extraldflags" and then observe the output when compiling with --verbose. Except that -fPIE -pie and -pthread are present after my extra LDFLAGS:

/usr/local/bin/clang++-devel -O0 -ftrapv -g3 -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -L/extracldflags -fPIE -pie src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o -o src/bitcoin-cli -Wl,-rpath,/usr/local/lib: src/libbitcoin_cli.a src/libbitcoin_common.a src/util/libbitcoin_util.a -pthread /usr/local/lib/libevent.so src/univalue/libunivalue.a src/libbitcoin_consensus.a src/crypto/libbitcoin_crypto.a libsecp256k1.a

These extra flags are not printed in the summary. An adapted #93 must make it.

[vasild]

Why is it necessary to parse the command line arguments manually/explicitly? Aren't the variables readily available in e.g. ${EXTRA_CXXFLAGS}? I was (naively?) hoping that this would be as simple as:

CXXFLAGS += ${EXTRA_CXXFLAGS}

or

set(CXXFLAGS "${CXXFLAGS} ${EXTRA_CXXFLAGS}")

[hebasto]

From the CMake docs, it follows:

1. The CXXFLAGS environment variable the CMAKE_CXX_FLAGS CMake variable. CMake can combine its own builtin toolchain-specific default flags (the exact flags are not documented) into the CMAKE_CXX_FLAGS variable.
2. During compiler invocation, the resulted flags are combined as follows:

(`CMAKE_CXX_FLAGS`) + (`CMAKE_CXX_FLAGS_<CONFIG>`) + (`COMPILE_OPTIONS` target properties)

To be able to override any flag, the EXTRA_CXXFLAGS/APPEND_CXXFLAGS must be a COMPILE_OPTIONS property of a target that is considered the last.

[vasild]

I think it is good to explain "Why?" (ie why are they mandatory). Something like:

They are mandatory for use in Bitcoin Core because they handle EXTRA_CPPFLAGS, EXTRA_CFLAGS, EXTRA_CXXFLAGS and EXTRA_LDFLAGS.

[fanquake]

Yea, it's not ectually clear why we need all this extra machinery to do this, and it's more non-standard stuff we'll have to maintain. I would also assume we can have a better interface/encapsulation, than a function that is always called with the same arguments, otherwise it can just be missued.

[vasild]

My understanding is that this is needed if we want to append to the CXXFLAGS when invoking CMake, in order to override only some options, but not all. E.g. if it would use internally "-Wfoo -Wbar" we don't want to drop/override this completely but we want to append "-Wno-foo" so that it becomes "-Wfoo -Wbar -Wno-foo". Am I right?

This implementation looks quite complicated to me but I couldn't come up with a simpler one. First I tried to use the global add_compile_options(), but that is already out-lawed via cmake/module/WarnAboutGlobalProperties.cmake. Then I tried to append the extra flags to core_interface by something like:

cmake_language(EVAL CODE "
  cmake_language(DEFER
    DIRECTORY ${PROJECT_SOURCE_DIR}
    CALL target_compile_options core_interface INTERFACE ${EXTRA_CPPFLAGS} $<$<COMPILE_LANGUAGE:CXX>:${EXTRA_CXXFLAGS}>
  )
")

but to my annoyance the extra flags appear before all the -Werror -W... flags 😠

[hebasto]

E.g. if it would use internally "-Wfoo -Wbar" we don't want to drop/override this completely but we want to append "-Wno-foo" so that it becomes "-Wfoo -Wbar -Wno-foo". Am I right?

Correct.

This implementation looks quite complicated to me but I couldn't come up with a simpler one. First I tried to use the global add_compile_options(), but that is already out-lawed via cmake/module/WarnAboutGlobalProperties.cmake.

It is out-lawed for a reason :)
Directory-level properties are quite error-prone and hard to reason about in large project like Bitcoin Core.

Then I tried to append the extra flags to core_interface by something like...

It is still possible to combine extra flags into the core_interface library, and use it in these invocations. But, again, this approach is a bit error-prone as it it requires an extra care about the order of appending all other flags to the core_interface library.

[vasild]

If CMake does not have a notion of CPPFLAGS can we avoid it as well? Looks like sneaking an autotool-ism into CMake based build system. I peeked at the CI changes that engage EXTRA_CPPFLAGS and I think using EXTRA_CXXFLAGS instead should work, no?

[hebasto]

We have discussed this earlier and during today's call. The rough consensus is to keep EXTRA_CPPFLAGS for convenience.

[hebasto]

Rebased to resolve conflicts.

[vasild]

Approach ACK f77ab70

Would be nice to have:

• Deduplicate the code from subtree_add_library and bitcoincore_add_library
• Elaborate the comment at cmake: Add user-defined APPEND_{CPP,C,CXX,LD}FLAGS #157 (comment)
• Rename to APPEND_ or LATE_

[hebasto]

@vasild

Thank you for your review!

Approach ACK f77ab70

Would be nice to have:

• Deduplicate the code from subtree_add_library and bitcoincore_add_library

• Elaborate the comment at cmake: Add user-defined EXTRA_{CPP,C,CXX,LD}FLAGS #157 (comment)

• Rename to APPEND_ or LATE_

Your feedback has been addressed.

[hebasto]

FWIW, the Qt 6 uses a quite similar approach.

From Professional CMake: A Practical Guide 18th Edition:

Qt 6 provides more automated handling of Qt-specific aspects of target creation. Certain additional processing needs to occur after targets have been defined, but that processing is usually deferred to the end of the scope in which the target is created. This is called target finalization. It is usually deferred so that the project has an opportunity to modify target properties which affect finalization.

Qt sets up finalization of targets through wrappers around the standard add_executable() and add_library() commands. These wrappers are qt_add_executable() and qt_add_library(), which can be used as drop-in replacements for their corresponding built-in CMake counterparts.

[hebasto]

Rebased.

[vasild]

ACK 1fc6d39

The append flags are not printed in the configure summary, #93

Works as expected, except then linking -pthread and -fPIE -pie appear after the append flags: /usr/local/bin/clang++-devel -O0 -ftrapv -g3 -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -L/appendldflags -fPIE -pie src/CMakeFiles/bitcoind.dir/bitcoind.cpp.o src/CMakeFiles/bitcoind.dir/init/bitcoind.cpp.o -o src/bitcoind -Wl,-rpath,/usr/local/lib: src/libbitcoin_node.a src/wallet/libbitcoin_wallet.a libleveldb.a libcrc32c.a libcrc32c_sse42.a libminisketch.a /usr/local/lib/libevent_pthreads.so src/libbitcoin_common.a src/util/libbitcoin_util.a -pthread /usr/local/lib/libevent.so src/libbitcoin_consensus.a src/crypto/libbitcoin_crypto.a src/crypto/libbitcoin_crypto_sse41.a src/crypto/libbitcoin_crypto_avx2.a src/crypto/libbitcoin_crypto_x86_shani.a libsecp256k1.a src/univalue/libunivalue.a /usr/local/lib/libsqlite3.so && :. Why is that? It makes it impossible to append -fno-PIE.

[hebasto]

Works as expected, except then linking -pthread ... appear after the append flags

This is expected as -pthread is a transitive link requirement of the bitcoin_util library:

bitcoin/src/util/CMakeLists.txt

Lines 45 to 50 in 2dc63ca

	target_link_libraries(bitcoin_util
	PRIVATE
	bitcoin_clientversion
	bitcoin_crypto
	Threads::Threads
	)

[fanquake]

This is expected as -pthread is a transitive link requirement of the bitcoin_util library:

I think you'll need to explain this further. The point of these options is to always have the last say, so it's not at all expected that some options would still be added after these flags.

[hebasto]

Works as expected, except then ... -fPIE -pie appear after the append flags...

Well, that's annoying.

Why is that?

This is because CMake treats Position Independent Code /Executable as abstractions with internal implementation of the logic for applying the related compile and linker flags.

It makes it impossible to append -fno-PIE.

I have no solution for that at the moment.

[hebasto]

This is expected as -pthread is a transitive link requirement of the bitcoin_util library:

I think you'll need to explain this further. The point of these options is to always have the last say, so it's not at all expected that some options would still be added after these flags.

1. An explanation:

When -pthread is provided, it is required to resolve particular symbols, which makes CMake treat it as any other -l option. It is expected that a linker fails if any symbol remains unresolved. Therefore, overriding -pthread seems has no use cases.

However,

2. I have a solution.

[vasild]

ACK 2dc63ca

I think there is no way to negate -pthread (e.g. -no-pthread), so its late appearance seems ok.

The -fPIE is indeed annoying, but IMO not a blocker for this PR.

I checked that cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=OFF has no effect and still adds -fPIE, maybe because the top level CMakeLists.txt sets it like set(CMAKE_POSITION_INDEPENDENT_CODE ON) after a check. Maybe this can be made conditional - only set it to ON if it is not already set to OFF on the command line?

[hebasto]

I checked that cmake -DCMAKE_POSITION_INDEPENDENT_CODE:BOOL=OFF has no effect and still adds -fPIE, maybe because the top level CMakeLists.txt sets it like set(CMAKE_POSITION_INDEPENDENT_CODE ON) after a check. Maybe this can be made conditional - only set it to ON if it is not already set to OFF on the command line?

This approach will work.

[fanquake]

When -pthread is provided, it is required to resolve particular symbols, which makes CMake treat it as any other -l option. It is expected that a linker fails if any symbol remains unresolved. Therefore, overriding -pthread seems has no use cases.

This doesn't explain why it's appearing after our options on the link line? Wether or not overriding it may or may not have use is mostly irrelevant. The fact it's appearing on the line after our options points to this not completely working like we intend.

[hebasto]

When -pthread is provided, it is required to resolve particular symbols, which makes CMake treat it as any other -l option. It is expected that a linker fails if any symbol remains unresolved. Therefore, overriding -pthread seems has no use cases.

This doesn't explain why it's appearing after our options on the link line?

"... which makes CMake treat it as any other -l option"

The -l option must appear after an object that uses symbols being resolved in a following library. CMake lists objects/archives after other options when invoking a linker.

Again, this can be fixed, if there are reasons for that.

[hebasto]

Works as expected, except then linking -pthread and -fPIE -pie appear after the append flags...

Please consider an alternative implementation in #184.

[hebasto]

Closing in favour of #184.