Skip to content
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

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

Closed
wants to merge 96 commits into from

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Apr 21, 2024

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 hebasto added the enhancement New feature or request label Apr 21, 2024
@hebasto
Copy link
Owner Author

hebasto commented Apr 21, 2024

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

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +30 to +28
cmake_parse_arguments(PARSE_ARGV 1 FWD "" "" "")
add_library("${name}" ${FWD_UNPARSED_ARGUMENTS})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}")

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CMakeLists.txt Outdated Show resolved Hide resolved
src/qt/CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines 7 to 9
# Functions in this module are drop-in replacements for CMake's add_library
# and add_executable functions. They are mandatory for use in the Bitcoin
# Core project, except for imported and interface libraries.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😠

Copy link
Owner Author

@hebasto hebasto Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cmake/module/AddTargetMacros.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
@@ -94,6 +94,11 @@ cmake_dependent_option(FUZZ "Build for fuzzing. Enabling this will disable all o

option(INSTALL_MAN "Install man pages." ON)

set(EXTRA_CPPFLAGS "" CACHE STRING "Extra (Objective) C/C++ preprocessor flags.")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner Author

@hebasto hebasto Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hebasto
Copy link
Owner Author

hebasto commented Apr 25, 2024

Rebased to resolve conflicts.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK f77ab70

Would be nice to have:

cmake/module/AddTargetMacros.cmake Outdated Show resolved Hide resolved
@hebasto hebasto changed the title cmake: Add user-defined EXTRA_{CPP,C,CXX,LD}FLAGS cmake: Add user-defined APPEND_{CPP,C,CXX,LD}FLAGS Apr 26, 2024
@hebasto
Copy link
Owner Author

hebasto commented Apr 26, 2024

@vasild

Thank you for your review!

Approach ACK f77ab70

Would be nice to have:

Your feedback has been addressed.

@hebasto
Copy link
Owner Author

hebasto commented Apr 28, 2024

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 added 6 commits April 30, 2024 18:14
67f5198 fixup! cmake: Build `bitcoin_consensus` library (Hennadii Stepanov)

Pull request description:

  Separated sources have not been needed since libbitcoinconsencus was removed.

  Required for #157.

Top commit has no ACKs.

Tree-SHA512: c4f391f439a7401771152ce5efee54c4b85b2ae879af95a66bc06e902a86a254defcf83526b1d778a3f523886076906382020f9a0f024d666f13195ea52b8547
d88e03c cmake: Add `docs` build target (Hennadii Stepanov)

Pull request description:

  Same as `make docs` in the master branch.

ACKs for top commit:
  vasild:
    ACK d88e03c

Tree-SHA512: 6c8871658cd686576b41e73c0adcdedf97ef2d1ce9a25c9f0625ec94183e6cda7eb642e25e761c1bdfed45fb26fa20af39565a61e7087143636452086a416b7c
5ca0799 fixup! cmake: Build `bitcoin_crypto` library (Hennadii Stepanov)

Pull request description:

  Avoid using source-specific compile options. They hard to reason about and error-prone when combining with target-specific compile options.

  The resulted build logic effectively mirrors the master branch's one.

  Required for #157.

  Similar to #171.

ACKs for top commit:
  TheCharlatan:
    ACK 5ca0799

Tree-SHA512: 8f55c86a3ad900c8de1789a97c936e39362d9995d75d2f9ca2d0f65f5863816795623d8ec1f2cc5f4468e27d934ce99150574cf766a7d6dd1da68546f13216fb
43fbeac fixup! cmake: Build `crc32c` static library (Hennadii Stepanov)

Pull request description:

  Avoid using source-specific compile options. They are hard to reason about and error-prone when combining with target-specific compile options.

  The resulted build logic effectively mirrors the master branch's one.

  Required for #157.

ACKs for top commit:
  TheCharlatan:
    ACK 43fbeac

Tree-SHA512: 51eac99f9c7327afc9edf812575b2c0a76c899d788cac547e22ed573c7d28694ae5615fa6686d0033bc9cc85e0c350dd8f5aeaacbfde864c8a9718a18efae6f8
The content of those variables is appended to the each target flags
added by the build system.
@hebasto hebasto force-pushed the 240421-cmake-DR branch from 555798a to 1fc6d39 Compare May 1, 2024 10:52
@hebasto
Copy link
Owner Author

hebasto commented May 1, 2024

Rebased.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cmake/module/AddTargetMacros.cmake Outdated Show resolved Hide resolved
cmake/module/AddTargetMacros.cmake Outdated Show resolved Hide resolved
@hebasto hebasto force-pushed the 240421-cmake-DR branch from 1fc6d39 to 2dc63ca Compare May 2, 2024 10:45
@hebasto
Copy link
Owner Author

hebasto commented May 2, 2024

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:

target_link_libraries(bitcoin_util
PRIVATE
bitcoin_clientversion
bitcoin_crypto
Threads::Threads
)

@fanquake
Copy link

fanquake commented May 2, 2024

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
Copy link
Owner Author

hebasto commented May 2, 2024

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
Copy link
Owner Author

hebasto commented May 2, 2024

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,

  1. I have a solution.

Copy link

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner Author

hebasto commented May 2, 2024

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
Copy link

fanquake commented May 2, 2024

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
Copy link
Owner Author

hebasto commented May 2, 2024

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
Copy link
Owner Author

hebasto commented May 2, 2024

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

Please consider an alternative implementation in #184.

@hebasto
Copy link
Owner Author

hebasto commented May 9, 2024

Closing in favour of #184.

@hebasto hebasto closed this May 9, 2024
hebasto added a commit that referenced this pull request May 17, 2024
…e 2)

a9dc2c1 cmake: Dispaly `APPEND_{CPP,C,CXX,LD}FLAGS` in summary (Hennadii Stepanov)
bfadb6e cmake: Add `APPEND_{CPP,C,CXX,LD}FLAGS` cache variables (Hennadii Stepanov)

Pull request description:

  An alternative implementation of #157.

ACKs for top commit:
  vasild:
    ACK a9dc2c1

Tree-SHA512: 77fff64817c59153e4022390bed3a5db1eeb247ea916c99c582fcf18fe9e1f638aa2dd38121ebb5734339bf15d935a8b817083918020b944f2a0d72c76375e86
real-or-random added a commit to bitcoin-core/secp256k1 that referenced this pull request Jun 25, 2024
…oin Core's approach

4706be2 cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach (Hennadii Stepanov)
c2764db cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS` (Hennadii Stepanov)

Pull request description:

  This PR address this hebasto/bitcoin#239 (comment):
  > For consistency with libsecp256k1:
  >
  > > > Is this code block supposed to achieve the same as our `SECP256K1_LATE_CFLAGS` (implemented by a user-defined function `all_targets_add_compile_options`) in libsecp256k1?
  > >
  > >
  > > It is. But this approach guaranties to override even options that are abstracted by CMake, for instance [#157 (comment)](hebasto/bitcoin#157 (comment)).
  >
  >    * If we agree that appending to rule variables is superior, should we also do this in libsecp256k1?
  >
  >    * And/or should we rename the `SECP256K1_LATE_CFLAGS` variable to `APPEND_CFLAGS`?

ACKs for top commit:
  real-or-random:
    utACK 4706be2

Tree-SHA512: 24603886c4d6ab4e31836a67d5759f9855a60c6c9d34cfc6fc4023bd309cd51c15d986ac0b77a434f9fdc6d5e97dcd3b8484d8f5ef5d8f840f47dc141de18084
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants