ci: add cmake debug output by ryanofsky · Pull Request #352 · bitcoin-core/libmultiprocess

ryanofsky · 2026-08-20T20:34:22Z

Show detailed debug information if cmake fails in CI

DrahtBot · 2026-08-20T20:34:32Z

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

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	maflcko, hebasto
Stale ACK	ViniciusCestarii

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#175 (Set cmake_minimum_required(VERSION 3.22) by maflcko)

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.

maflcko

lgtm ACK 6d719cf

maflcko · 2026-08-21T06:35:13Z

+  # If cmake failed, try it again with debug options.
+  # Could add --trace / --trace-expand here too but they are very verbose.
+  cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG)
+  cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?"


Suggested change

cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?"

cmake "$src_dir" "${cmake_args[@]}" || echo "cmake exited with $?"

nit: I know this is duplicate, but this way non-Bash people won't have to confirm this works:

# set -o xtrace false || : "command exited with $?" set +o xtrace + false + : 'command exited with 1' + set +o xtrace

re: #352 (comment)

From my perspective, the suggested change makes output more verbose, and I don't know what problem it would be solving. I'd definitely change this if it looked misleading, but it seems pretty obvious this is trying to show an error message. I also don't know why someone who didn't know bash would worry about this. It should take only few seconds to look up how this works if anyone is worried.

ViniciusCestarii

ACK 6d719cf

Looks good, just added a comment about cmake support for the flags used.

ViniciusCestarii · 2026-08-21T13:21:39Z

+if ! cmake "$src_dir" "${cmake_args[@]}"; then
+  # If cmake failed, try it again with debug options.
+  # Could add --trace / --trace-expand here too but they are very verbose.
+  cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG)


Since this project supports cmake 3.12+ it would be good to guard these flags against a minimum version otherwise the command will just fail for these lower versions.

if ver_ge "$cmake_ver" "3.16"; then cmake_args+=(--log-level=DEBUG); fi
if ver_ge "$cmake_ver" "3.17"; then cmake_args+=(--debug-find); fi

re: https://github.com/bitcoin-core/libmultiprocess/pull/352/changes#r3830510005

Since this project supports cmake 3.12+ it would be good to guard these flags against a minimum version otherwise the command will just fail for these lower versions.

Agree these would be nice changes and I'd be happy to review a followup adding them.

re: #352 (comment)

Since this project supports cmake 3.12+ it would be good to guard these flags against a minimum version otherwise the command will just fail for these lower versions.

Thanks! Added this change in latest rebase

hebasto · 2026-08-23T12:04:02Z

+  # Could add --trace / --trace-expand here too but they are very verbose.
+  cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG)
+  cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?"
+  cat CMakeFiles/CMakeConfigureLog.yaml || true


The CMakeFiles/CMakeConfigureLog.yaml is available only in CMake >=3.26.

re: #352 (comment)

The CMakeFiles/CMakeConfigureLog.yaml is available only in CMake >=3.26.

Thanks, this good to know. The || true should let the script continue if the file isn't created for any other reason as well. But it could be a good idea to add a ver_ge check like the ones ViniciusCestarii suggested above

re: #352 (comment)

The CMakeFiles/CMakeConfigureLog.yaml is available only in CMake >=3.26.

Thanks! Added guard in latest rebase

hebasto · 2026-08-23T12:12:34Z

+  # If cmake failed, try it again with debug options.
+  # Could add --trace / --trace-expand here too but they are very verbose.
+  cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG)
+  cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?"


The second cmake invocation is reusing CMakeCache.txt, skipping the checks whose results are cached. Is this the desired behaviour?

re: #352 (comment)

The second cmake invocation is reusing CMakeCache.txt, skipping the checks whose results are cached. Is this the desired behaviour?

I think there are tradeoffs. I'd presume more likely than not if if results were cached they were probably successful results, so the current change lets the script be faster and simpler and show strictly more information than it did previously. It could be a good idea to delete the cache or build directory though, and I'd happy review if someone wanted to implement this followup.

re: #352 (comment)

The second cmake invocation is reusing CMakeCache.txt, skipping the checks whose results are cached.

Decided not to implement a change here because erasing CMakeCache.txt could make failures harder to debug locally and would not be respecting the CI_CLEAN option. I think it could be reasonable to erase the cache (or entire build directory) after a failure when CI_CLEAN is true and to set CI_CLEAN in CI jobs, but erasing things by default when these scripts are run locally seems unsafe and inconvenient, and value of extra debug output that would be provided seems low.

Co-Authored-By: ViniciusCestarii <124843824+ViniciusCestarii@users.noreply.github.com> Co-Authored-By: hebasto <32963518+hebasto@users.noreply.github.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ryanofsky · 2026-08-26T03:42:51Z

Thanks for the reviews! Looks like this conflicted with #212 so I'll make all the suggested changes above in the rebase.

maflcko · 2026-08-26T14:09:56Z

Seeing Claude being added as co-author here in the force-push, makes me a bit flabbergasted, but I guess this is the thing now. lgtm ACK fbd50d2

$ git range-diff upstream/master 6d719cf7ccb1cd6d958898cc11db528a334b9d39 fbd50d2f5dd2ed5311ba457ea9de57322da9b0b2  -U0
1:  6d719cf ! 1:  fbd50d2 ci: add cmake debug output
    @@ Commit message
    +    Co-Authored-By: ViniciusCestarii <124843824+ViniciusCestarii@users.noreply.github.com>
    +    Co-Authored-By: hebasto <32963518+hebasto@users.noreply.github.com>
    +    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
    +
    @@ ci/scripts/ci.sh
    -@@ ci/scripts/ci.sh: src_dir=$PWD
    - mkdir -p "$CI_DIR"
    +@@ ci/scripts/ci.sh: mkdir -p "$CI_DIR"
    @@ ci/scripts/ci.sh: src_dir=$PWD
    + git --no-pager log -1 || true
    @@ ci/scripts/ci.sh: src_dir=$PWD
    -+  cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG)
    ++  cmake_args+=(--debug-output --debug-trycompile)
    ++  if ver_ge "$cmake_ver" "3.16"; then cmake_args+=(--log-level=DEBUG); fi
    ++  if ver_ge "$cmake_ver" "3.17"; then cmake_args+=(--debug-find); fi
    @@ ci/scripts/ci.sh: src_dir=$PWD
    -+  cat CMakeFiles/CMakeConfigureLog.yaml || true
    ++  if ver_ge "$cmake_ver" "3.26"; then cat CMakeFiles/CMakeConfigureLog.yaml || true; fi

ryanofsky

Rebased 6d719cf -> fbd50d2 (pr/cmake-debug.1 -> pr/cmake-debug.2, compare) due to conflict with #212, also implementing review suggestions.

re: #352 (comment)

Seeing Claude being added as co-author here in the force-push, makes me a bit flabbergasted, but I guess this is the thing now.

Sorry about that and I sympathize but I instantly muted 35794 as soon as I saw the title because I thought it would be too distracting for me, so I am not caught up right now. I do plan to read the discussion at some point and would like to talk about it in person. Appreciate you reviewing and approving despite this.

ryanofsky · 2026-08-26T14:03:06Z

+if ! cmake "$src_dir" "${cmake_args[@]}"; then
+  # If cmake failed, try it again with debug options.
+  # Could add --trace / --trace-expand here too but they are very verbose.
+  cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG)


re: #352 (comment)

Since this project supports cmake 3.12+ it would be good to guard these flags against a minimum version otherwise the command will just fail for these lower versions.

Thanks! Added this change in latest rebase

ryanofsky · 2026-08-26T14:03:27Z

+  # Could add --trace / --trace-expand here too but they are very verbose.
+  cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG)
+  cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?"
+  cat CMakeFiles/CMakeConfigureLog.yaml || true


re: #352 (comment)

The CMakeFiles/CMakeConfigureLog.yaml is available only in CMake >=3.26.

Thanks! Added guard in latest rebase

ryanofsky · 2026-08-26T14:10:27Z

+  # If cmake failed, try it again with debug options.
+  # Could add --trace / --trace-expand here too but they are very verbose.
+  cmake_args+=(--debug-find --debug-output --debug-trycompile --log-level=DEBUG)
+  cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?"


re: #352 (comment)

The second cmake invocation is reusing CMakeCache.txt, skipping the checks whose results are cached.

Decided not to implement a change here because erasing CMakeCache.txt could make failures harder to debug locally and would not be respecting the CI_CLEAN option. I think it could be reasonable to erase the cache (or entire build directory) after a failure when CI_CLEAN is true and to set CI_CLEAN in CI jobs, but erasing things by default when these scripts are run locally seems unsafe and inconvenient, and value of extra debug output that would be provided seems low.

hebasto

ACK fbd50d2.

hebasto · 2026-08-27T14:02:02Z

+  if ver_ge "$cmake_ver" "3.16"; then cmake_args+=(--log-level=DEBUG); fi
+  if ver_ge "$cmake_ver" "3.17"; then cmake_args+=(--debug-find); fi
+  cmake "$src_dir" "${cmake_args[@]}" || : "cmake exited with $?"
+  if ver_ge "$cmake_ver" "3.26"; then cat CMakeFiles/CMakeConfigureLog.yaml || true; fi


Suggested change

if ver_ge "$cmake_ver" "3.26"; then cat CMakeFiles/CMakeConfigureLog.yaml || true; fi

if ver_ge "$cmake_ver" "3.26"; then

cat CMakeFiles/CMakeConfigureLog.yaml || true

else

cat CMakeFiles/CMakeError.log CMakeFiles/CMakeOutput.log || true

fi

maflcko reviewed Aug 21, 2026

View reviewed changes

ViniciusCestarii reviewed Aug 21, 2026

View reviewed changes

This was referenced Aug 21, 2026

ci: add cmake --parallel build option #353

Closed

ci: add newdeps job testing newer versions of cmake and capnproto #212

Merged

cmake: Increase cmake policy version #209

Open

Set cmake_minimum_required(VERSION 3.22) #175

Open

hebasto reviewed Aug 23, 2026

View reviewed changes

ci: add cmake debug output

fbd50d2

Co-Authored-By: ViniciusCestarii <124843824+ViniciusCestarii@users.noreply.github.com> Co-Authored-By: hebasto <32963518+hebasto@users.noreply.github.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

DrahtBot added the Needs rebase label Aug 26, 2026

ryanofsky mentioned this pull request Aug 26, 2026

cmake: make Threads package optional #354

Closed

ryanofsky force-pushed the pr/cmake-debug branch from 6d719cf to fbd50d2 Compare August 26, 2026 14:02

DrahtBot requested a review from ViniciusCestarii August 26, 2026 14:10

ryanofsky commented Aug 26, 2026

View reviewed changes

DrahtBot removed the Needs rebase label Aug 26, 2026

hebasto approved these changes Aug 27, 2026

View reviewed changes

	cmake "$src_dir" "${cmake_args[@]}" \|\| : "cmake exited with $?"
	cmake "$src_dir" "${cmake_args[@]}" \|\| echo "cmake exited with $?"

Uh oh!

Conversation

ryanofsky commented Aug 20, 2026

Uh oh!

DrahtBot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViniciusCestarii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Aug 26, 2026

Uh oh!

maflcko commented Aug 26, 2026

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Aug 20, 2026 •

edited

Loading