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

hebasto · 2025-01-23T14:08:33Z

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

Here are a few examples of the summary:

DrahtBot · 2025-01-23T14:08:38Z

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 · 2025-01-23T14:32:34Z

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

 -- Found SQLite3: /usr/include (found suitable version "3.46.1", minimum required is "3.7.17")
CMake Warning at CMakeLists.txt:195 (message):
  PIE is not supported at link time: PIE (CXX): Change Dir:
  '/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/CMakeFiles/CMakeScratch/TryCompile-.raysb'

  Run Build Command(s): /opt/ooce/bin/cmake -E env VERBOSE=1 /usr/bin/make -f
  Makefile cmTC_93ec4/fast

  /usr/bin/make -f CMakeFiles/cmTC_93ec4.dir/build.make
  CMakeFiles/cmTC_93ec4.dir/build

  Building CXX object CMakeFiles/cmTC_93ec4.dir/src.cxx.o

  /usr/bin/g++ -DCMAKE_CXX_LINK_PIE_SUPPORTED -pthread -std=c++20 -o
  CMakeFiles/cmTC_93ec4.dir/src.cxx.o -c
  /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/CMakeFiles/CMakeScratch/TryCompile-.raysb/src.cxx


  Linking CXX executable cmTC_93ec4

  /opt/ooce/bin/cmake -E cmake_link_script CMakeFiles/cmTC_93ec4.dir/link.txt
  --verbose=1

  g++: error: -pie is not supported in this configuration

  /usr/bin/g++ -pthread -fPIE -pie CMakeFiles/cmTC_93ec4.dir/src.cxx.o -o
  cmTC_93ec4

  *** Error code 1

  make: Fatal error: Command failed for target `cmTC_93ec4'

  Current working directory
  /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/build/CMakeFiles/CMakeScratch/TryCompile-.raysb


  *** Error code 1

  make: Fatal error: Command failed for target `cmTC_93ec4/fast'

-- Performing Test CXX_SUPPORTS__WERROR
-- Performing Test CXX_SUPPORTS__WERROR - Success
-- 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 · 2025-01-23T14:38:58Z

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 · 2025-01-23T14:45:01Z

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 · 2025-01-23T14:56:41Z

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 · 2025-01-23T15:07:18Z

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.

cmake/module/GetTargetInterface.cmake

hebasto · 2025-01-23T15:40:52Z

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/module/GetTargetInterface.cmake

davidgumberg · 2025-01-25T07:08:15Z

Tested ACK 2083e9c

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

On FreeBSD 14.2

# freebsd-version
14.2-RELEASE

master:

# cmake -B build
C++ 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:

# cmake -B build
C++ 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

hebasto · 2025-02-17T15:51:43Z

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?

fanquake · 2025-02-17T15:55:18Z

cmake/module/GetTargetInterface.cmake

+    # the C library implementation, the CMake FindThreads module
+    # sets the Threads::Threads target's compile options to
+    # generator expressions that evaluate to `-pthread` in this
+    # project.


generator expressions that evaluate to -pthread in this project.

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

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 · 2025-02-19T12:28:29Z

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. build: broken CMake *flags output #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.

DrahtBot · 2025-08-19T00:45:04Z

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko · 2025-08-19T20:02:31Z

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.

I think there is value in printing a configure summary to stdout, so writing it only to a file could make it harder to quickly double-check if an option or compile flag has been applied.

fanquake · 2025-11-20T17:21:15Z

I've closed #31482, as not sure making further changes to address this is worth it.

hebasto added the Build system label Jan 23, 2025

hebasto mentioned this pull request Jan 23, 2025

build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON #31665

Closed

fanquake reviewed Jan 23, 2025

View reviewed changes

cmake/module/GetTargetInterface.cmake Outdated Show resolved Hide resolved

hebasto mentioned this pull request Jan 23, 2025

cmake: Add CheckLinkerSupportsPIE module #31359

Merged

fanquake mentioned this pull request Jan 23, 2025

CMake CheckPIESupported doesn't always work #30771

Closed

cmake: Fix -pthread flags in summary

2083e9c

hebasto force-pushed the 250123-cmake-pthread branch from 300db5b to 2083e9c Compare January 23, 2025 18:40

davidgumberg reviewed Jan 25, 2025

View reviewed changes

cmake/module/GetTargetInterface.cmake Show resolved Hide resolved

davidgumberg added a commit to davidgumberg/bitcoin that referenced this pull request Jan 25, 2025

alternative approach to pr bitcoin#31724

956fb31

fanquake reviewed Feb 17, 2025

View reviewed changes

hebasto changed the title ~~cmake: Fix -pthread flags in summary~~ cmake: Fix -pthread flags presentation in summary Feb 20, 2025

hebasto closed this Nov 20, 2025

cmake: Fix -pthread flags presentation in summary #31724

cmake: Fix -pthread flags presentation in summary #31724

Uh oh!

Conversation

hebasto commented Jan 23, 2025

Uh oh!

DrahtBot commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Uh oh!

fanquake commented Jan 23, 2025

Uh oh!

hebasto commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 23, 2025

Uh oh!

fanquake commented Jan 23, 2025

Uh oh!

Uh oh!

hebasto commented Jan 23, 2025

Uh oh!

Uh oh!

davidgumberg commented Jan 25, 2025

Uh oh!

hebasto commented Feb 17, 2025

Uh oh!

fanquake Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

purpleKarrot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 19, 2025

Uh oh!

maflcko commented Aug 19, 2025

Uh oh!

fanquake commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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

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

DrahtBot commented Jan 23, 2025 •

edited

Loading

hebasto commented Jan 23, 2025 •

edited

Loading

fanquake commented Jan 23, 2025 •

edited

Loading

purpleKarrot commented Feb 19, 2025 •

edited

Loading