cmake: add optional source files to bitcoin

purpleKarrot commented at 11:22 am on November 11, 2024: contributor

Avoid having many static libraries by adding the optional sources to the target bitcoin_crypto directly. Set the necessary compile options at the source file level, rather than the target level.

fixes: #31697

DrahtBot commented at 11:22 am on November 11, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31268.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, s373nZ, TheCharlatan
Concept ACK	sipa, theuni
Approach ACK	BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31884 (cmake: Make implicit libbitcoinkernel dependencies explicit by hebasto)
#31880 (cmake: Add optional source files to libraries directly by hebasto)
#31394 ([POC] cmake: Introduce LLVM’s Source-based Code Coverage reports by hebasto)
#31161 (cmake: Set top-level target output locations by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Build system on Nov 11, 2024

in src/crypto/CMakeLists.txt:57 in 7cdae6d7e0 outdated

72 if(HAVE_ARM_SHANI)
73-  add_library(bitcoin_crypto_arm_shani STATIC EXCLUDE_FROM_ALL
74-    sha256_arm_shani.cpp
75+  target_compile_definitions(bitcoin_crypto PRIVATE ENABLE_ARM_SHANI)
76+  target_sources(bitcoin_crypto PRIVATE sha256_arm_shani.cpp)
77+  set_property(SOURCE sha256_x86_shani.cpp PROPERTY

willcl-ark commented at 11:26 am on November 11, 2024:

I think you have a copy-paste error here. x86 should be arm?

purpleKarrot force-pushed on Nov 11, 2024

DrahtBot added the label CI failed on Nov 11, 2024

DrahtBot commented at 11:29 am on November 11, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32802162894

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

purpleKarrot force-pushed on Nov 11, 2024

DrahtBot removed the label CI failed on Nov 11, 2024

fanquake commented at 2:17 pm on November 11, 2024: member

Avoid having many static libraries

Can you expand the motivation.

purpleKarrot commented at 2:44 pm on November 11, 2024: contributor

There are four alternative implementations for sha256, each in its own source file which has to be compiled with dedicated compiler options. The current approach is to build a static library per source file, set the necessary compiler options for that library, and eventually link bitcoin_crypto against those libraries. This is needless complexity.

My proposed approach is to compile the source file with its dedicated options, but link the object files into bitcoin_crypto directly.

sipa commented at 3:28 pm on November 11, 2024: member

Concept ACK. If it’s possible to have differently-compiled compilation units without needing separate libraries for each, then this does seem like a cleaner solution. I suspect we perhaps ended up in this state because in autotools separate libraries were the only way to achieve this?

BrandonOdiwuor commented at 10:59 am on November 14, 2024: contributor

Approach ACK. Great catch!

Nice simplification! Adding the optional source files directly to bitcoin_crypto eliminating the need for intermediate libraries like bitcoin_crypto_sse41, bitcoin_crypto_avx2, and bitcoin_crypto_x86_shani

hebasto commented at 8:01 pm on November 16, 2024: member

Avoid having many static libraries by adding the optional sources to the target bitcoin_crypto directly. Set the necessary compile options at the source file level, rather than the target level.

While working on the CMake staging branch, this was done on purpose (see https://github.com/hebasto/bitcoin/pull/172 and similar https://github.com/hebasto/bitcoin/pull/171). However, the goal PR has since been replaced.

Additionally, from the Professional CMake: A Practical Guide 19th Edition:

Developers should be aware of an implementation detail which may present a strong deterrent to their use in some situations. For some CMake generators (notably the Unix Makefiles generator), the dependencies between sources and source properties are stronger than one might expect. If source properties are used to modify the compiler flags for specific source files rather than for a whole target, changing the source’s compiler flags will still result in all of the target’s sources being rebuilt, not just the affected source file. This is a limitation of how the dependency details are handled in the Makefile, where testing whether each source file’s compiler flags have changed brings with it a prohibitively big performance hit. The relevant Makefile dependencies were implemented at the target level instead to avoid that problem.

purpleKarrot commented at 7:18 am on December 1, 2024: contributor

If source properties are used to modify the compiler flags for specific source files rather than for a whole target, changing the source’s compiler flags will still result in all of the target’s sources being rebuilt, not just the affected source file.

I don’t think that applies here. The source’s compile flags are never changed between builds: sha256_avx2.cpp is either compiled with ${AVX2_CXXFLAGS}, or it is not compiled at all. And AVX2_CXXFLAGS is only ever set to -mavx -mavx2. The issue that all of the libraries source files get recompiled never occurs. @hebasto, can you elaborate why you think the source-specific compile options are hard to reason about?

hebasto commented at 10:41 am on December 1, 2024: member

@hebasto, can you elaborate why you think the source-specific compile options are hard to reason about?

Sure. Consider this minimal example:

0cmake_minimum_required(VERSION 3.22)
1project(test CXX)
2
3add_library(interface INTERFACE)
4target_compile_options(interface INTERFACE -Wno-mock-interface)
5
6add_executable(test main.cpp)
7set_property(SOURCE main.cpp PROPERTY COMPILE_OPTIONS -Wno-mock-source)
8target_compile_options(test PRIVATE -Wno-mock-target)
9target_link_libraries(test PRIVATE interface)

It is not straightforward to determine the actual order of compiler flags passed to the compiler when the COMPILE_OPTIONS target property is used.

0-Wno-mock-target -Wno-mock-interface -Wno-mock-source

purpleKarrot commented at 11:31 am on December 1, 2024: contributor

Why is that hard to reason about? If I add

0add_compile_options(-Wno-mock-global)

the resulting order will be -Wno-mock-global -Wno-mock-target -Wno-mock-interface -Wno-mock-source, so the flags are ordered from most generic to most specific.

That aside, I don’t think the order of arguments is relevant here.

hebasto commented at 11:38 am on December 1, 2024: member

Why is that hard to reason about?

People unfamiliar with such peculiarities might not realise that compiler options specified as a source’s property take precedence over other compiler options.

purpleKarrot commented at 12:30 pm on December 1, 2024: contributor

Yeah, they might also not realize that compiler options specified at the target level might take precedence over those specified globally…

theuni commented at 7:01 pm on December 5, 2024: member

I also find the reasons against doing this do be pretty uncompelling.

I consider the current approach to be a leftover autootools artifact. This is a nice cleanup and simplification, and the (stated) downsides seem pretty minimal. It looks like the crc32c lib would benefit from this approach as well.

Unless there’s some actual use case or generator that this breaks, +1 from me.

in contrib/devtools/check-deps.sh:11 in ed2cd60878 outdated

 7@@ -8,7 +8,7 @@ declare -A LIBS
 8 LIBS[cli]="libbitcoin_cli.a"
 9 LIBS[common]="libbitcoin_common.a"
10 LIBS[consensus]="libbitcoin_consensus.a"
11-LIBS[crypto]="crypto/libbitcoin_crypto.a crypto/libbitcoin_crypto_x86_shani.a crypto/libbitcoin_crypto_sse41.a crypto/libbitcoin_crypto_avx2.a"
12+LIBS[crypto]="crypto/libbitcoin_crypto.a"

hebasto commented at 8:53 am on January 23, 2025:

This change resolves #31697.

hebasto commented at 9:02 am on January 23, 2025: member

I consider the current approach to be a leftover autootools artifact.

I agree. Concept ACK.

It looks like the crc32c lib would benefit from this approach as well.

Could we extend this PR to include the crc32c library as well?

purpleKarrot commented at 9:42 am on January 23, 2025: contributor

Could we extend this PR to include the crc32c library as well?

Since crc32c is a subtree, I have submitted a cleanup upstream: https://github.com/google/crc32c/pull/66

theuni commented at 5:19 pm on January 23, 2025: member

Could we extend this PR to include the crc32c library as well?

Since crc32c is a subtree, I have submitted a cleanup upstream: google/crc32c#66

FYI we don’t use the upstream CMake buildsystem, rather we handle the sources ourselves. See cmake/crc32c.cmake.

purpleKarrot force-pushed on Jan 23, 2025

purpleKarrot requested review from hebasto on Jan 23, 2025

in cmake/crc32c.cmake:85 in 7a53ce7422 outdated

82-target_compile_definitions(crc32c_common INTERFACE
83+add_library(crc32c STATIC EXCLUDE_FROM_ALL
84+  ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c.cc
85+  ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_portable.cc
86+)
87+target_compile_definitions(crc32c PRIVATE

hebasto commented at 11:59 am on February 2, 2025:

style nit: Keep the PRIVATE keyword on its own line with proper indentation, as in all surrounding code?

purpleKarrot commented at 8:15 am on February 7, 2025:

I just reused the existing style and just renamed the target and the keyword. Reformatting that line has more ripple effects in the lines that follow.

in cmake/crc32c.cmake:107 in 7a53ce7422 outdated

127-  )
128-  target_compile_options(crc32c_arm64 PRIVATE ${ARM64_CRC_CXXFLAGS})
129-  target_link_libraries(crc32c_arm64 PRIVATE crc32c_common)
130-  set_target_properties(crc32c_arm64 PROPERTIES EXPORT_COMPILE_COMMANDS OFF)
131-  target_link_libraries(crc32c PRIVATE crc32c_arm64)
132+  set(srcs ${PROJECT_SOURCE_DIR}/src/crc32c/src/crc32c_arm64.cc)

hebasto commented at 12:02 pm on February 2, 2025:

Unset the srcs variable at the end to avoid polluting the global namespace?

purpleKarrot commented at 8:14 am on February 7, 2025:

Done. Also renamed the variable to _crc32_src.

hebasto approved

hebasto commented at 12:06 pm on February 2, 2025: member

ACK 7a53ce742207c5dfaeda7858a098049627b03d55, tested on Ubuntu 24.04, both x86_64 and aarch64.

These changes effectively revert https://github.com/hebasto/bitcoin/pull/171 and https://github.com/hebasto/bitcoin/pull/172.

Could you please mention the changes to cmake/crc32c.cmake in the PR title and description, and separate them into their own commit?

Additionally, it seems reasonable to mention this in the PR description.

I think that APPENDing to the COMPILE_OPTIONS property should be preferred over overriding it.

DrahtBot requested review from BrandonOdiwuor on Feb 2, 2025

DrahtBot requested review from sipa on Feb 2, 2025

cmake: add optional source files to bitcoin_crypto directly

fixes: #31268

9c7823c5b5

cmake: add optional source files to crc32c directly 9cf746d663

purpleKarrot force-pushed on Feb 7, 2025

purpleKarrot commented at 8:17 am on February 7, 2025: contributor

I think that APPENDing to the COMPILE_OPTIONS property should be preferred over overriding it.

The command line flags that are passed to the compiler are constructed by several variables and properties that are global, target specific, and file specific. Since this is the only file specific option, APPENDing has no different effect as setting.

purpleKarrot renamed this:
~~cmake: add optional source files to bitcoin_crypto directly~~
cmake: add optional source files to bitcoin_crypto and crc32c directly
on Feb 7, 2025

hebasto approved

hebasto commented at 12:16 pm on February 10, 2025: member

re-ACK 9cf746d6631739df9c9f80accd5812b319efcfec.

s373nZ commented at 4:12 pm on February 14, 2025: none

ACK 9cf746d6631739df9c9f80accd5812b319efcfec

Tested compilation on Ubuntu 24.04 x86_64. Learned a lot looking over this PR. Nice work!

fanquake added this to the milestone 29.0 on Feb 14, 2025

fanquake commented at 4:15 pm on February 14, 2025: member

@theuni could you circle back here?

theuni commented at 8:41 pm on February 14, 2025: member

Agree with @hebasto that it’d be more straightforward to use target_compile_options(), if only for the sake of avoiding future footguns.

utACK otherwise.

hebasto commented at 12:52 pm on February 16, 2025: member

#31880 provides an extended alternative to this PR.

Since this one has already been reviewed, I’m OK with merging it as is.

purpleKarrot commented at 7:56 pm on February 16, 2025: contributor

Agree with @hebasto that it’d be more straightforward to use target_compile_options(), if only for the sake of avoiding future footguns.

Very generally in software development, future footguns are avoided by narrow contracts.

I understand the desire of narrowing the contract between the authors and CMake. One may want to avoid advanced features like setting properties on individual source files. Applying this approach consistently, one should also avoid commands like target_compile_options() and just call add_compile_definitions(), add_compile_options(), and include_directories() once in the top-level CMakeLists.txt. Keep it simple, stupid. The generated build system however, will have a wide contract: All targets are affected by all compile flags.

The approach that I am advocating on the other hand, is to use advanced cmake features in order to strive for a narrow contract in the generated build system. I would define an object library if it has to be consumed by multiple other targets. If it is consumed by a single target only, adding the source file to the consuming target directly narrows the contract and therefore better avoids possible footguns.

TheCharlatan approved

TheCharlatan commented at 9:37 am on February 18, 2025: contributor

ACK 9cf746d6631739df9c9f80accd5812b319efcfec

DrahtBot requested review from theuni on Feb 18, 2025

fanquake closed this on Feb 18, 2025

fanquake merged this on Feb 18, 2025

cmake: add optional source files to bitcoin_crypto and crc32c directly #31268

Code Coverage & Benchmarks

Reviews

Conflicts