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

[ryanofsky]

Add CI job testing latest released version of cmake, and latest unreleased version of cap'n proto to uncover any problems there may be with new releases. Use git version of cap'n proto because we've submitted patches to that project to fix issues in the past, so it would be good to know about any new issues as soon as possible.

[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	maflcko, hebasto

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #353 (ci: add cmake --parallel build option by ryanofsky)
• #352 (ci: add cmake debug output by ryanofsky)
• #333 (nix: Make clang tooling and IWYU find the right standard library headers by ryanofsky)
• #231 (Add windows support by ryanofsky)
• #209 (cmake: Increase cmake policy version by ryanofsky)
• #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.

[ryanofsky]

These changes were originally part of #209 but moved them to a separate PR since #209 might be blocked or require more time to discuss

[hebasto]

While this project maintains support for CMake versions as old as 3.12, I believe this support should also extend consistently to the CI infrastructure, particularly given the "olddeps" job, which runs CMake 3.12.4. Therefore, features incompatible with CMake 3.12, such as --debug-find and --log-level, should be avoided. For example, the "olddeps" CI job may result in:

+ cmake /home/runner/work/libmultiprocess/libmultiprocess --debug-find --debug-output --debug-trycompile --log-level=DEBUG
CMake Error: The source directory "/home/runner/work/libmultiprocess/libmultiprocess/build-olddeps/--log-level=DEBUG" does not exist.

Alternatively, the minimum supported CMake version could be bumped.

[ryanofsky]

re: #212 (comment)

For example, the "olddeps" CI job may result in

If the problem you would like me to solve is that extra output potentially appearing in the olddeps job, it is trivial to skip the debug step there with:

-if ! cmake "$src_dir" "${cmake_args[@]}"; then
+if ! cmake "$src_dir" "${cmake_args[@]}" && ver_ge "$cmake_ver" "3.17"; then

But I don't see a problem with this extra output and I don't know if this the issue you actually care about. If you are arguing that systems using cmake 3.16 should not be supported because it does not support the --debug-find option, than that sounds like something to mention in #175 more than here.

[hebasto]

Moving discussion from #209 (comment).

re: #209 (comment)

6ba1050
This log file name has been used only since CMake 3.26.

Yes that's part of the reason for adding || true here. I think this code could potentially print log files used by older versions of cmake too, and originally this change tried that, but since all CI jobs except one are using new enough versions of cmake it didn't seem worth complexity.

One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?

Would happily accept a PR to patch to improve this though.

This would make sense if it were the only compatibility issue in this PR.

[ryanofsky]

re: #212 (comment)

One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?

It would be helpful to know what complexity you are referring to specifically here. The find_packge(Threads REQUIRED) issue would have been impossible to debug without the line:

cat CMakeFiles/CMakeConfigureLog.yaml || true

and I do not feel like it adds much complexity. This CI script in general seems short and simple to me. I don't know what problem you have with the other debug prints either. In general, I feel like it would be a lot easier to respond to your comments if they could make it clear:

1. What observable problem you think this change causes, or could potentially cause.
2. What change you would suggest to fix the problem.

[ryanofsky]

Appreciate your review, even though I am having trouble understanding what your practical concerns are and what specific changes I could make that would address them.

Hopefully we both accept the premise that it is good to have a new CI job testing newer versions of cmake and capnproto, and it is good to show more debug output when cmake configuration fails.

[ryanofsky]

re: #212 (comment)

One may argue: why introduce complexity for a feature that doesn’t work with the minimum supported CMake version in the first place?

It would be helpful to know what complexity you are referring to specifically here. The find_packge(Threads REQUIRED) issue would have been impossible to debug without the line:

cat CMakeFiles/CMakeConfigureLog.yaml || true

and I do not feel like it adds much complexity. This CI script in general seems short and simple to me. I don't know what problem you have with the other debug prints either. In general, I feel like it would be a lot easier to respond to your comments if they could make it clear:

1. What observable problem you think this change causes, or could potentially cause.
2. What change you would suggest to fix the problem.

[ryanofsky]

re: #212 (comment)

For example, the "olddeps" CI job may result in

If the problem you would like me to solve is that extra output potentially appearing in the olddeps job, it is trivial to skip the debug step there with:

-if ! cmake "$src_dir" "${cmake_args[@]}"; then
+if ! cmake "$src_dir" "${cmake_args[@]}" && ver_ge "$cmake_ver" "3.17"; then

But I don't see a problem with this extra output and I don't know if this the issue you actually care about. If you are arguing that systems using cmake 3.16 should not be supported because it does not support the --debug-find option, than that sounds like something to mention in #175 more than here.

[ryanofsky]

Rebased 6ba1050 -> 03b147e (pr/newdeps.1 -> pr/newdeps.2, compare) due to conflict with #334

Updated 03b147e -> b3fc922 (pr/newdeps.2 -> pr/newdeps.3, compare) dropping debug commit 03b147e

[maflcko]

lgtm ACK b3fc922

[maflcko]

is there a ci config that does not have git? I can't see one and it seems easy enough to just require it, so that logs always have the commit embedded?

[ryanofsky]

re: #212 (comment)

is there a ci config that does not have git? I can't see one and it seems easy enough to just require it, so that logs always have the commit embedded?

Sounds like the request here is to remove || true?

I'd slightly prefer to keep it just because these CI scripts are useful for testing locally even outside context of a real CI system, and there are reasons it can be useful to copy files around and not have valid git checkouts during testing. But it is true that a drawback of || true is that it could allow someone to add a CI job that does not have print the commit hash here. So I'd be ok with a change dropping || true in my planned followup, just let me know.

[maflcko]

Suggested change

	CAPNP_CHECKOUT=master
	CAPNP_CHECKOUT=master # This is the v1.x release branch

The default branch is v2, maybe clarify what master means?

[ryanofsky]

re: #212 (comment)

The default branch is v2, maybe clarify what master means?

Good suggestion, have added in followup

[hebasto]

ACK b3fc922.

Add CI job testing latest released version of cmake...

Could be updated as follows:

--- a/ci/configs/newdeps.bash
+++ b/ci/configs/newdeps.bash
@@ -2,5 +2,5 @@ CI_DESC="CI job using newest Cap'n Proto and cmake versions"
 CI_DIR=build-newdeps
 export CXXFLAGS="-Werror -Wall -Wextra -Wpedantic -Wno-unused-parameter -Wno-error=array-bounds"
 CAPNP_CHECKOUT=master
-NIX_ARGS=(--argstr capnprotoVersion "none" --argstr cmakeVersion "4.1.1")
+NIX_ARGS=(--argstr capnprotoVersion "none" --argstr cmakeVersion "4.3.4")
 BUILD_ARGS=(-k)
--- a/shell.nix
+++ b/shell.nix
@@ -55,7 +55,7 @@ let
   clang-tools = llvm.clang-tools.override { inherit enableLibcxx; };
   cmakeHashes = {
     "3.12.4" = "sha256-UlVYS/0EPrcXViz/iULUcvHA5GecSUHYS6raqbKOMZQ=";
-    "4.1.1" = "sha256-sp9vGXM6oiS3djUHoQikJ+1Ixojh+vIrKcROHDBUkoI=";
+    "4.3.4" = "sha256-/e/4l7nrSddkU58rHtxut+FEDfMlZ4qXwZeEmekxrdo=";
   };
   gcc = if gccVersion == null then null else builtins.getAttr ("gcc" + gccVersion) pkgs;
   cmakeBuild = if cmakeVersion == null then pkgs.cmake else (pkgs.cmake.overrideAttrs (old: {

[ryanofsky]

Thanks for the reviews! Am planning to merge as-is since this is reviewed and passes ci, but will make a followup bumping the cmake version and implementing other suggestions

[ryanofsky]

re: #212 (comment)

is there a ci config that does not have git? I can't see one and it seems easy enough to just require it, so that logs always have the commit embedded?

Sounds like the request here is to remove || true?

I'd slightly prefer to keep it just because these CI scripts are useful for testing locally even outside context of a real CI system, and there are reasons it can be useful to copy files around and not have valid git checkouts during testing. But it is true that a drawback of || true is that it could allow someone to add a CI job that does not have print the commit hash here. So I'd be ok with a change dropping || true in my planned followup, just let me know.

[ryanofsky]

re: #212 (comment)

The default branch is v2, maybe clarify what master means?

Good suggestion, have added in followup