ci: add cmake debug output

[ryanofsky]

Show detailed debug information if cmake fails in CI

[DrahtBot]

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	hebasto
Stale ACK	ViniciusCestarii, maflcko

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

[maflcko]

lgtm ACK 6d719cf

[maflcko]

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

[ryanofsky]

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]

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

[ryanofsky]

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.

[ryanofsky]

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]

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

[ryanofsky]

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

[ryanofsky]

re: #352 (comment)

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

Thanks! Added guard in latest rebase

[hebasto]

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

[ryanofsky]

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.

[ryanofsky]

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.

[ryanofsky]

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

[maflcko]

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]

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]

re: #352 (comment)

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

Thanks! Added guard in latest rebase

[ryanofsky]

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.

[ryanofsky]

Updated fbd50d2 -> a7ff9d5 (pr/cmake-debug.2 -> pr/cmake-debug.3, compare) applying CMakeError.log / CMakeOutput.log suggestion

[hebasto]

re-ACK a7ff9d5.