cmake: Add user-defined APPEND_{CPP,C,CXX,LD}FLAGS (take 2)

[hebasto]

An alternative implementation of #157.

[theuni]

Context please!

Please compare/contrast the approaches at a high-level. Keep in mind that while you're holding this all in your head, it's a lot for the reviewers to keep up with as it's constantly changing.

[hebasto]

Context please!

Please compare/contrast the approaches at a high-level. Keep in mind that while you're holding this all in your head, it's a lot for the reviewers to keep up with as it's constantly changing.

The former approach in #157 works at a target properties (a CMake abstraction) level. It fails in certain cases:

linking -pthread and -fPIE -pie appear after the append flags

This approach works at a lower level. Flags are appended directly to the compiler/linker invocation strings, after processing all higher level abstractions. Please refer to CMake docs for the used variables:

• CMAKE_<LANG>_COMPILE_OBJECT
• CMAKE_<LANG>_CREATE_SHARED_LIBRARY
• CMAKE_<LANG>_LINK_EXECUTABLE

[vasild]

ACK b567a38

This works as well and the flags appear really and the end:

/usr/bin/c++ ... -MD -MT src/CMakeFiles/bitcoin_node.dir/net.cpp.o -MF src/CMakeFiles/bitcoin_node.dir/net.cpp.o.d -o src/CMakeFiles/bitcoin_node.dir/net.cpp.o -c src/net.cpp -I/appendcppflags -I/appendcxxflags

/usr/bin/c++ ... src/CMakeFiles/bitcoind.dir/bitcoind.cpp.o src/CMakeFiles/bitcoind.dir/init/bitcoind.cpp.o -o src/bitcoind ... /usr/local/lib/libsqlite3.so -L/appendldflags

[hebasto]

Rebased.

[hebasto]

Rebased to resolve a conflict with the merged #173.

[vasild]

ACK bfadb6e

[theuni]

Is it intended that depends will be updated to use these?

[theuni]

Doing a depends build with -DAPPEND_CXXFLAGS="-flto" -DAPPEND_LDFLAGS="-flto", I don't see those flags in the build summary.

[hebasto]

Doing a depends build with -DAPPEND_CXXFLAGS="-flto" -DAPPEND_LDFLAGS="-flto", I don't see those flags in the build summary.

The configure summary is to be reworked in #93.

[hebasto]

Is it intended that depends will be updated to use these?

No, at this moment. Should it be?

[theuni]

Is it intended that depends will be updated to use these?

No, at this moment. Should it be?

I just mean generally. I'm unsure how to test this as:

• It's not hooked up to depends
• It's not hooked up to c-i
• It doesn't display in the summary

That said, using VERBOSE=1, lto does seem to be working as intended.

[hebasto]

@theuni

I just mean generally. I'm unsure how to test this ...

To observe raw build logs using the --verbose command-line option?

... as:

• It doesn't display in the summary

I've added a commit for that.

[hebasto]

During the recent call, @theuni requested a CI stuff based on this PR.

Please consider bitcoin#29790.

[hebasto]

During the recent call, @theuni requested a CI stuff based on this PR.

Please consider bitcoin#29790.

Cross posting from bitcoin#29790 (comment):

Personally I like the append approach, because it specifies that everything is the "default" plus the given flags appended. Otherwise there would be ambiguity and hard-to-debug issues which default flags are omitted (or kept) by setting CXXFLAGS. (c.f. https://github.com/bitcoin/bitcoin/pull/29205/files#diff-8b9cb286f7af766e7d4615428ff76c84947644ed42032c54e97e0aa4aeee751e)

Other than that, I think the CI system should not receive more thought. I think the primary concern should be that it is clear for end-users. The CI can always be adjusted to fit in.

[vasild]

ACK a9dc2c1

[TheCharlatan]

ACK

[hebasto]

During the recent call, @theuni requested a CI stuff based on this PR.

Please consider bitcoin#29790.

As it was requested during the call yesterday, I've updated the CI demo branch in bitcoin#29790 to use APPEND_*FLAGS only as the last resort. As expected, they are still required when we want to apply -U_FORTIFY_SOURCE while having hardening enabled.

[hebasto]

Considering that 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, I'm going to merge this PR shortly.