cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET

hebasto commented at 3:45 pm on January 15, 2025: member

(Almost?) every CMake check internally uses the try_compile() command, whose behaviour, in turn, depends on the CMAKE_TRY_COMPILE_TARGET_TYPE variable:

The default value, EXECUTABLE, enables both compiler and linker checks.

The STATIC_LIBRARY value enables only compiler checks.

To mimic Autotools’ behaviour, we disabled linker checks by setting CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY globally (perhaps not the best design). This effectively separates the entire CMake script into regions where CMAKE_TRY_COMPILE_TARGET_TYPE is:

unset

set to STATIC_LIBRARY

set to EXECUTABLE

From #31359 (comment):

This seems very fragile and unintuitive, and the fact that this could silently break at any point is not documented in any way. I don’t think other bad design decisions should lead to us having to write even more boilerplate code to fix things that should “just work” (minus the upstream bugs).

Agreed. I forgot that we set CMAKE_TRY_COMPILE_TARGET_TYPE globally. And even worse, it’s buried in a module. If that upsets CMake internal tests, I think we should undo that.

This PR ensures that CMAKE_TRY_COMPILE_TARGET_TYPE is modified only within local scopes.

hebasto added the label Build system on Jan 15, 2025

DrahtBot commented at 3:45 pm on January 15, 2025: 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/31662.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

theuni commented at 4:06 pm on January 15, 2025: member

High-level question: Do we actually need the old autotools behavior? If this is just about shaving off some time when running the linker, I’d rather skip that optimization. Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and also we would be ok with that failure?

If not, we could just skip the complexity and not modify the policy at all.

theuni commented at 4:10 pm on January 15, 2025: member

High-level question: Do we actually need the old autotools behavior? If this is just about shaving off some time when running the linker, I’d rather skip that optimization. Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and also we would be ok with that failure?

If not, we could just skip the complexity and not modify the policy at all.

~~From a quick look at the new uses of bitcoin_check_cxx_source_compiles here, only check_evhttp_connection_get_peer would be affected.~~

Edit: Actually, that one already has CMAKE_REQUIRED_LIBRARIES set so it should be ok. @hebasto Is there an obvious reason I’m missing to not just use CMake’s default behavior here rather than fighting it?

theuni commented at 4:45 pm on January 15, 2025: member

I tried making this change (simply removing the global CMAKE_TRY_COMPILE_TARGET_TYPE assignment) and ended up with an identical bitcoin-build-config.h.

It was .4 sec slower, but that’s not enough to care about :)

hebasto commented at 4:59 pm on January 15, 2025: member

High-level question: Do we actually need the old autotools behavior?

That was a strict requirement for the staging branch, and mapping the Autotools’ AC_PREPROC_IFELSE, AC_COMPILE_IFELSE and AC_LINK_IFELSE macros to CMake functions was essential.

Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and also we would be ok with that failure?

I don’t think so.

Is there an obvious reason I’m missing to not just use CMake’s default behavior here rather than fighting it?

I’d love to live with CMake’s default behavior!

It was .4 sec slower, but that’s not enough to care about :)

It might be even slower with LTO enabled, though.

theuni commented at 5:16 pm on January 15, 2025: member

High-level question: Do we actually need the old autotools behavior?

That was a strict requirement for the staging branch, and mapping the Autotools’ AC_PREPROC_IFELSE, AC_COMPILE_IFELSE and AC_LINK_IFELSE macros to CMake functions was essential.

Sure, I didn’t mean to imply that we were doing the wrong thing here. Only that we’ve learned that it causes problems and the 1:1 matching with autotools is problematic.

Is there an obvious reason I’m missing to not just use CMake’s default behavior here rather than fighting it?

I’d love to live with CMake’s default behavior!

Proposed alternative to this PR: https://github.com/theuni/bitcoin/commits/cmake-trycompile-exe/ What do you think?

It was .4 sec slower, but that’s not enough to care about :)

It might be even slower with LTO enabled, though.

Possibly, but the test stubs are so small it can’t matter much.

build: don't override CMake's default try_compile target

CMake assumes the default value internally, so overriding this causes problems.
The minimal speedup of skipping the linker isn't worth the complexity of
setting it to static.

9df8f70d96

hebasto force-pushed on Jan 15, 2025

hebasto commented at 5:59 pm on January 15, 2025: member

Proposed alternative to this PR: https://github.com/theuni/bitcoin/commits/cmake-trycompile-exe/ What do you think?

Thank you. It looks great!

I also pushed an updated branch, which has a cleaner resulting code.

However, I’m happy to switch to your branch, if you prefer it.

hebasto force-pushed on Jan 15, 2025

hebasto marked this as a draft on Jan 15, 2025

cmake: Delete `check_cxx_source_links_with_flags` macro f72e557507

hebasto force-pushed on Jan 15, 2025

DrahtBot added the label CI failed on Jan 15, 2025

hebasto marked this as ready for review on Jan 15, 2025

hebasto commented at 6:37 pm on January 15, 2025: member

A typo has been fixed (

DrahtBot removed the label CI failed on Jan 15, 2025

in CMakeLists.txt:381 in 583c5af4d8 outdated

377       #include <cstdint>
378       #include <cstddef>
379       extern \"C\" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { return 0; }
380       // No main() function.
381     " FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION
382+    CXXFLAGS ${SANITIZER_LDFLAGS}

theuni commented at 7:32 pm on January 15, 2025:

Could you add an LDFLAGS param to the function as LINK_OPTIONS and use that here instead?

Edit: or maybe just call it LINK_OPTIONS for symmetry.

hebasto commented at 7:59 pm on January 15, 2025:

LDFLAGS might make more sense because, for example, in the OSS-Fuzz environment, the SANITIZER_LDFLAGS variable can be set to -fsanitize=fuzzer or /usr/lib/libFuzzingEngine.a.

hebasto commented at 8:42 pm on January 15, 2025:

The LDFLAGS option has been added.

in cmake/module/TestAppendRequiredLibraries.cmake:31 in 2934a7124c outdated

29-  check_cxx_source_links("${check_socket_source}" IFADDR_LINKS_WITHOUT_LIBSOCKET)
30+  include(CheckCXXSourceCompiles)
31+  check_cxx_source_compiles("${check_socket_source}" IFADDR_LINKS_WITHOUT_LIBSOCKET)
32   if(NOT IFADDR_LINKS_WITHOUT_LIBSOCKET)
33-    check_cxx_source_links_with_libs(socket "${check_socket_source}" IFADDR_NEEDS_LINK_TO_LIBSOCKET)
34+    set(CMAKE_REQUIRED_LIBRARIES socket)

theuni commented at 7:33 pm on January 15, 2025:

Could you add a LINK_LIBRARIES option to check_cxx_source_compiles_with_flags and use that here instead?

hebasto commented at 8:41 pm on January 15, 2025:

Thanks! Done.

cmake: Convert `check_cxx_source_compiles_with_flags` to a function dbb91982dd

cmake: Delete `check_cxx_source_links*` macros 4149c8e0fc

scripted-diff: Rename CMake helper module

-BEGIN VERIFY SCRIPT-
 git mv cmake/module/CheckSourceCompilesAndLinks.cmake cmake/module/CheckSourceCompilesWithFlags.cmake
 sed -i 's|\<CheckSourceCompilesAndLinks\>|CheckSourceCompilesWithFlags|g' $(git grep -l 'CheckSourceCompilesAndLinks')
 -END VERIFY SCRIPT-

e3b722081b

hebasto force-pushed on Jan 15, 2025

hebasto commented at 8:40 pm on January 15, 2025: member

@theuni

Thank you for your review! Your comments have been addressed.

in cmake/module/CheckSourceCompilesAndLinks.cmake:37 in dbb91982dd outdated

39+
40+]=]
41+function(check_cxx_source_compiles_with_flags source result_var)
42+  cmake_parse_arguments(PARSE_ARGV 2 _ "" "" "CXXFLAGS;LDFLAGS;LINK_LIBRARIES")
43+  list(JOIN __CXXFLAGS " " CMAKE_REQUIRED_FLAGS)
44+  set(CMAKE_REQUIRED_LIBRARIES ${__LDFLAGS} ${__LINK_LIBRARIES})

theuni commented at 4:44 pm on January 16, 2025:

Why does CMAKE_REQUIRED_LIBRARIES get LDFLAGS rather than setting LINK_OPTIONS?

theuni commented at 4:47 pm on January 16, 2025:

Ah, I see your comment here: #31662 (review)

I think a static lib would work fine as an option though? Or -l/path/to/libfoo.a otherwise?

hebasto commented at 4:50 pm on January 16, 2025:

I think a static lib would work fine as an option though?

Correct. This is documented by CMake.

theuni commented at 7:02 pm on January 16, 2025:

I meant as LINK_OPTIONS. I’d rather have LDFLAGS forward to that unless there’s a reason not to.

hebasto commented at 8:02 pm on January 16, 2025:

Okay, I’ve checked CMake docs again.

CMAKE_REQUIRED_LINK_OPTIONS is forwarded to target_link_options(), which is designed “to add any link options”.

~~I’ll update the PR shortly.~~

hebasto commented at 8:15 pm on January 16, 2025:

I meant as LINK_OPTIONS. I’d rather have LDFLAGS forward to that unless there’s a reason not to.

In that case a static library (e.g., /usr/lib/libFuzzingEngine.a) would be placed before object files being linked, and the linker might fail.

cmake: Do not modify CMAKE_TRY_COMPILE_TARGET_TYPE globally #31662

Code Coverage & Benchmarks

Reviews

cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally #31662