cmake: Fix `-pthread` flags presentation in summary

hebasto commented at 2:08 pm on January 23, 2025: member

This PR removes the raw CMake generator expressions from the summary output.

Fixes #31482.

Here are a few examples of the summary:

hebasto added the label Build system on Jan 23, 2025

DrahtBot commented at 2:08 pm on January 23, 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/31724.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	davidgumberg

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

fanquake commented at 2:32 pm on January 23, 2025: member

on OmniOS: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/12930654711/job/36062682690#step:5:25

 0 -- Found SQLite3: /usr/include (found suitable version "3.46.1", minimum required is "3.7.17")
 1CMake Warning at CMakeLists.txt:195 (message):
 2  PIE is not supported at link time: PIE (CXX): Change Dir:
 3  '/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/CMakeFiles/CMakeScratch/TryCompile-.raysb'
 4
 5  Run Build Command(s): /opt/ooce/bin/cmake -E env VERBOSE=1 /usr/bin/make -f
 6  Makefile cmTC_93ec4/fast
 7
 8  /usr/bin/make -f CMakeFiles/cmTC_93ec4.dir/build.make
 9  CMakeFiles/cmTC_93ec4.dir/build
10
11  Building CXX object CMakeFiles/cmTC_93ec4.dir/src.cxx.o
12
13  /usr/bin/g++ -DCMAKE_CXX_LINK_PIE_SUPPORTED -pthread -std=c++20 -o
14  CMakeFiles/cmTC_93ec4.dir/src.cxx.o -c
15  /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/CMakeFiles/CMakeScratch/TryCompile-.raysb/src.cxx
16
17
18  Linking CXX executable cmTC_93ec4
19
20  /opt/ooce/bin/cmake -E cmake_link_script CMakeFiles/cmTC_93ec4.dir/link.txt
21  --verbose=1
22
23  g++: error: -pie is not supported in this configuration
24
25  /usr/bin/g++ -pthread -fPIE -pie CMakeFiles/cmTC_93ec4.dir/src.cxx.o -o
26  cmTC_93ec4
27
28  *** Error code 1
29
30  make: Fatal error: Command failed for target `cmTC_93ec4'
31
32  Current working directory
33  /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/CMakeFiles/CMakeScratch/TryCompile-.raysb
34
35
36  *** Error code 1
37
38  make: Fatal error: Command failed for target `cmTC_93ec4/fast'
39
40-- Performing Test CXX_SUPPORTS__WERROR
41-- Performing Test CXX_SUPPORTS__WERROR - Success
42-- Setting build type to "RelWithDebInfo" as none was specified

This one looks like its producing errors, but they are ignored and configuring continues? Is that a different bug?

hebasto commented at 2:38 pm on January 23, 2025: member

Is that a different bug?

https://www.illumos.org/issues/15730

UPD. This PR changes only flag presentation in the summary without actual changes in the build system.

fanquake commented at 2:45 pm on January 23, 2025: member

https://www.illumos.org/issues/15730

So is this a CMake bug then? I don’t think our configure process should output "(Fatal) error" 3 different times and then carry on as if that’s normal? I’m assuming anyone who sees it would be confused / report it as a bug. Side not, this would also make building on this platform with -DWERROR impossible, if #31665 went ahead.

hebasto commented at 2:56 pm on January 23, 2025: member

https://www.illumos.org/issues/15730

So is this a CMake bug then?

Why a bug? A toolchain does not support PIE, and the script warns the user about that condition.
This discussion seems unrelated to this PR changes and might be continued elsewhere, no?

fanquake commented at 3:07 pm on January 23, 2025: member

Why a bug?

Because CMake has a habit of trying to work around these kinds of issues internally, so when something like this pops up, it’s something that they are not accomodating (and without looking at the code, it’s unclear if that’s a bug, or a concious decision).

script warns the user about that condition.

Sure, I’m just not sure this is useful output. It’s verbose compiler output, that also says error multiple times (not warning), and doesn’t explain what anything means, or why it might/might not be a problem.

This discussion seems unrelated to this PR changes and might be continued elsewhere, no?

Sure, we can open another issue, or role it into #30771, given this is related to the same CMake functionality.

in cmake/module/GetTargetInterface.cmake:36 in 300db5b17a outdated

31@@ -32,7 +32,15 @@ endfunction()
32 function(get_target_interface var config target property)
33   get_target_property(result ${target} INTERFACE_${property})
34   if(result)
35-    evaluate_generator_expressions(result "${config}")
36+    # The CMake FindThreads module uses generator expressions to conditionally apply
37+    # the `-pthread` flag, avoiding its addition in cases such as CUDA or Swift, which

fanquake commented at 3:11 pm on January 23, 2025:

Rather than documenting CUDA or Swift, which aren’t relevant to this project, it’d be better to explain why this issue only occurs on some operating systems, like NetBSD or FreeBSD, which we do support, which would make it clearer why we need to work around this.

hebasto commented at 3:27 pm on January 23, 2025:

These systems’ libcs do not implement pthread functionality and require linking to libpthread and the -pthread flag during compilation.

A workaround is required solely to evaluate generator expressions to make them readable in the summary. For an underlying build system they are evaluated automatically.

fanquake commented at 3:29 pm on January 23, 2025:

Sure, if that’s the issue then that’s what should be documented, not something about CUDA & Swift.

hebasto commented at 6:42 pm on January 23, 2025:

The comment has been rewritten.

hebasto commented at 3:40 pm on January 23, 2025: member

script warns the user about that condition.

Sure, I’m just not sure this is useful output. It’s verbose compiler output, that also says error multiple times (not warning), and doesn’t explain what anything means, or why it might/might not be a problem.

Fixed in #31359.

cmake: Fix `-pthread` flags in summary 2083e9cf05

hebasto force-pushed on Jan 23, 2025

in cmake/module/GetTargetInterface.cmake:42 in 2083e9cf05

38+    # sets the Threads::Threads target's compile options to
39+    # generator expressions that evaluate to `-pthread` in this
40+    # project.
41+    # To improve the readability of the configuration summary,
42+    # we skip these generator expressions.
43+    if(${target} STREQUAL "Threads::Threads" AND ${property} STREQUAL "COMPILE_OPTIONS")

davidgumberg commented at 7:05 am on January 25, 2025:

not blocking: if an upstream change in CMake ever sets different COMPILE_OPTIONS for Threads::Threads (probably unlikely) we would print something wrong in the flags summary,

What do you think of this as an alternative?

0--- a/cmake/module/GetTargetInterface.cmake
1+++ b/cmake/module/GetTargetInterface.cmake
2@@ -20,6 +20,10 @@ function(evaluate_generator_expressions list config)
3       if(NOT CMAKE_MATCH_1 STREQUAL config)
4         list(APPEND result ${CMAKE_MATCH_2})
5       endif()
6+    elseif(token MATCHES "\\$<\\$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>:SHELL:[^>]+>")
7+      # do nothing
8+    elseif(token MATCHES "\\$<\\$<AND:\\$<NOT:\\$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>>,\\$<NOT:\\$<COMPILE_LANGUAGE:Swift>>>:([^>]+)>")
9+      list(APPEND result ${CMAKE_MATCH_1})

hebasto commented at 4:17 pm on January 29, 2025:

if an upstream change in CMake ever sets different COMPILE_OPTIONS for Threads::Threads (probably unlikely)…

I would argue the opposite. It’s possible that more cases could be added where the -pthread flag should be filtered out using additional generator expressions. The last modification was made two years ago.

davidgumberg commented at 6:37 pm on February 2, 2025:

I agree, but in that situation we would be back to printing generator expressions in the flags summary, which seems to me better than silently printing something that is potentially not true.

hebasto commented at 3:41 pm on February 3, 2025:

I agree, but in that situation we would be back to printing generator expressions in the flags summary…

Unfortunately, it happens not only with unknown future changes but also with older yet supported CMake versions, such as 3.25, where $<$<NOT:$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>>:-pthread> is printed.

davidgumberg commented at 7:08 am on January 25, 2025: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/31724/commits/2083e9cf05ca64c12ec2aae42aac9c0f2054cd53

Left a non-blocking suggestion of an alternative approach, but I think this is a good solution to #31482 as-is.

0# freebsd-version
114.2-RELEASE

master:

0# cmake -B build
1C++ compiler flags .................... -O2 -g -std=c++20 -fPIC -fdebug-prefix-map=/root/bitcoin-master/src=. -fmacro-prefix-map=/root/bitcoin-master/src=. $<$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>:SHELL:-Xcompiler -pthread> $<$<AND:$<NOT:$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>>,$<NOT:$<COMPILE_LANGUAGE:Swift>>>:-pthread> -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection

On this branch:

0# cmake -B build
1C++ compiler flags .................... -O2 -g -std=c++20 -fPIC -fdebug-prefix-map=/root/bitcoin-master/src=. -fmacro-prefix-map=/root/bitcoin-master/src=. -pthread -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection

davidgumberg referenced this in commit 956fb31d12 on Jan 25, 2025

hebasto commented at 3:51 pm on February 17, 2025: member

Fixes #31482.

The issue been fixed is assigned to the “29.0” milestone.

Should the milestones for an issue and a PR with its fix be aligned?

in cmake/module/GetTargetInterface.cmake:39 in 2083e9cf05

31@@ -32,7 +32,18 @@ endfunction()
32 function(get_target_interface var config target property)
33   get_target_property(result ${target} INTERFACE_${property})
34   if(result)
35-    evaluate_generator_expressions(result "${config}")
36+    # On systems where pthread functionality is not provided by
37+    # the C library implementation, the CMake FindThreads module
38+    # sets the Threads::Threads target's compile options to
39+    # generator expressions that evaluate to `-pthread` in this
40+    # project.

fanquake commented at 3:55 pm on February 17, 2025:

generator expressions that evaluate to -pthread in this project.

So couldn’t we just stop using Threads::Threads and use -pthread directly?

hebasto commented at 2:19 pm on February 20, 2025:

It is our practise to check whether flags are supported and required, isn’t it?
In the Autotools-based build system, we used the AX_PTHREAD macro for that purpose.
The FindThreads module is a CMake’s way of achieving the same.
The current issue on the master branch is not about the thread-related flags for the toolchain but rather their presentation in the summary.

purpleKarrot commented at 12:28 pm on February 19, 2025: contributor

There are two general problems with printing a build summary at configure time:

It is annoying visual clutter. The cmake configure log should contain status events and important warnings as well as errors. Detailed settings can be analyzed and modified in the cache editor such as ccmake. Currently, when using ccmake, this dreaded summary has to be dismissed with e after each configure run with c. This is because make treats the summary as a warning.
There is no way to know all compile options at configure time. #31482 is just one example for showing that the exact settings that will be used for the build are not known until generate time. Trying to replace generator expressions with hardcoded constants is an endless effort but the more severe issue is that it turns the summary into an approximation. There is no guarantee that what you see in the summary actually matches the information used by the build system.

The best alternative to printing a summary at configure time, is to generate it into a file at generate time with file(GENERATE). This supports generator expressions.

hebasto renamed this:
~~cmake: Fix `-pthread` flags in summary~~
cmake: Fix `-pthread` flags presentation in summary
on Feb 20, 2025

cmake: Fix -pthread flags presentation in summary #31724

Code Coverage & Benchmarks

Reviews

cmake: Fix `-pthread` flags presentation in summary #31724