Mostly to get more recent iwyu coverage (ref #294 (comment))
ci: Bump channel to nixos-26.05 #296
pull maflcko wants to merge 2 commits into bitcoin-core:master from maflcko:2606-ci-bump changing 2 files +4 −1-
maflcko commented at 5:31 AM on June 11, 2026: contributor
-
ci: Bump channel to nixos-26.05 fa2c56ec27
-
DrahtBot commented at 5:31 AM on June 11, 2026: none
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
-
maflcko commented at 5:41 AM on June 11, 2026: contributor
Some bumps:
- gcc 14 -> 15
- llvm 20 -> 22 (albeit shell.nix pins it to 21)
- cmake 3 -> 4
- capn 1.1 -> 1.4
I wonder if it makes sense to have an llvm-olddeps check here as well (mostly for capn 1.1), to check the full range, but this can be done in a follow-up. Looks loke olddeps fails anyway ...
-
maflcko commented at 6:19 AM on June 11, 2026: contributor
I am at a loss on reproducing and fixing the cmake 3.12 compile failure. Also, I don't see the point on wasting more time with ancient cmake versions that are only used by a single person, when #175 can be done, so I've included that here, to get the CI green.
-
maflcko commented at 7:23 AM on June 11, 2026: contributor
Ok, the failure seems to be:
4206-/nix/var/nix/builds/nix-37473-276334059/cmake-3.12.4/Utilities/cmcurl/lib/hostip.c: In function 'Curl_resolv_timeout': 4207:/nix/var/nix/builds/nix-37473-276334059/cmake-3.12.4/Utilities/cmcurl/lib/hostip.c:721:23: error: assignment to '__sighandler_t' {aka 'void (*)(int)'} from incompatible pointer type 'int (*) (int)' [-Wincompatible-pointer-types] 4208- 721 | sigact.sa_handler = alarmfunc; 4209- | ^ 4210-/nix/var/nix/builds/nix-37473-276334059/cmake-3.12.4/Utilities/cmcurl/lib/hostip.c:623:12: note: 'alarmfunc' declared here 4211- 623 | RETSIGTYPE alarmfunc(int sig) 4212- | ^~~~~~~~~ 4213-In file included from /nix/var/nix/builds/nix-37473-276334059/cmake-3.12.4/Utilities/cmcurl/lib/hostip.c:46: 4214-/nix/store/15h9askp4k1lx44d9871wid23j2a8ijp-glibc-2.42-61-dev/include/signal.h:72:16: note: '__sighandler_t' declared here 4215- 72 | typedef void (*__sighandler_t) (int); 4216- | ^~~~~~~~~~~~~~I used
NIX_PATH='nixpkgs=https://github.com/NixOS/nixpkgs/archive/bd0ff2d3eac24699c3664d5966b9ef36f388e2ca.tar.gz' nix --extra-experimental-features 'nix-command flakes' build --impure --expr 'let pkgs = import <nixpkgs> {}; in (pkgs.cmake.overrideAttrs (old: { version = "3.12.4"; src = pkgs.fetchurl { url = "https://cmake.org/files/v3.12/cmake-3.12.4.tar.gz"; hash = "sha256-UlVYS/0EPrcXViz/iULUcvHA5GecSUHYS6raqbKOMZQ="; }; patches = []; })).override { isMinimalBuild = true; }' -LSo I guess the alternative would be to pin olddeps to an older snapshot/channel, but this just seems tedious for little benefit.
-
ryanofsky commented at 11:20 AM on June 11, 2026: collaborator
Thanks for the PR. The main change fa2c56ec27f21eac9a102afadd49a073f74504f2 looks good but as discussed previously I don't think 34f48d1e8f85dcd471c16a14b0bae01c1f2d561d is a good change because it is changing the cmake policy version and minimum version at the same time. These are two different concepts that have different effects and should not be coupled together, especially without even mentioning the policy changes in the commit message or PR description.
If you believe it's important to trigger a fatal error that says cmake 3.22 is required (even though it is not required), I think you should do that without changing the policy version. Or you might consider just showing a warning that versions <= 3.22 are not supported. Or you might consider dropping the cmake change from this PR and leaving it for a dedicated PR.
I also don't think the
ci.shchanges in 34f48d1e8f85dcd471c16a14b0bae01c1f2d561d are good and would prefer to expand CI coverage (#212) rather than reduce it. But if there isn't a simpler / better way to fix the nixpkg error #296 (comment) maybe those changes would be worth it. Would want to investigate a little more. -
hebasto commented at 11:58 AM on June 11, 2026: member
... as discussed previously I don't think 34f48d1 is a good change because it is changing the cmake policy version and minimum version at the same time.
I think the discussion about CMake policies would benefit from starting with a few basic questions:
- Which policies affect the project?
- Which minimum Policy Version is required by the project?
All previous changes in this repository, including 06e1045baba09fc1fb876b6cb48d93d4b41b101b, 6902bfd40e33a0007f75bce8af0fc0b10a147e86, and 729ff16d559c7727e883850c6ed245ec48d11954, do not provide details in that regard.
-
maflcko commented at 12:49 PM on June 11, 2026: contributor
I don't think there is a policy in 3.12->3.22 that affects libmul.
However, if you want me to push something like this to #175, then I am happy to do that.
cmake_minimum_required(VERSION 3.22) cmake_policy(VERSION 3.12) # Set older policy than minimum, after minimum was bumped. This line has no effect on this project, but was done to minimize and de-tangle changesThen, I can open a follow-up to remove the line again.
- maflcko closed this on Jul 8, 2026
- maflcko reopened this on Jul 8, 2026
- DrahtBot added the label Needs rebase on Jul 17, 2026
- maflcko force-pushed on Jul 17, 2026
- DrahtBot removed the label Needs rebase on Jul 17, 2026
-
maflcko commented at 10:30 AM on August 2, 2026: contributor
Not sure how to proceed here. I am happy to work on alternatives, but there is #175 with two acks, but it is not getting merged.
I can also try an alternative here, as suggested two months ago:
diff --git a/ci/configs/olddeps.bash b/ci/configs/olddeps.bash index 1a363b1..9cb9b4a 100644 --- a/ci/configs/olddeps.bash +++ b/ci/configs/olddeps.bash @@ -2,4 +2,7 @@ CI_DESC="CI job using old Cap'n Proto and cmake versions" CI_DIR=build-olddeps +# Keep olddeps on the previous Nixpkgs channel while the other CI jobs use the +# default channel, since compiling the old CMake requires an older GCC. +NIXPKGS_CHANNEL=nixos-25.05but there wasn't any feedback since.
-
ci: Pin oldeps config to older nixpkgs channel to compile older cmake with older gcc 7402affd0c
- maflcko force-pushed on Aug 2, 2026
- hebasto approved
-
hebasto commented at 6:56 PM on August 2, 2026: member
ACK 7402affd0ce7370fb6a82efb8268f3f168408519.
However, it might be better to pin
include-what-you-useto the pinned LLVM version: 53dd2f20ade74fa76d671e24c5075b4d9704e112. -
maflcko commented at 8:05 AM on August 3, 2026: contributor
However, it might be better to pin
include-what-you-useto the pinned LLVM version: 53dd2f2.Pretty sure your patch will fail with llvm 22 on the olddeps ci task, as that has not llvm 22. An alternative could be to select the llvm version like this:
diff --git a/shell.nix b/shell.nix index 1a4614e..0db3b7a 100644 --- a/shell.nix +++ b/shell.nix @@ -10,7 +10,9 @@ let lib = pkgs.lib; - llvmBase = crossPkgs.llvmPackages_21; + llvmBase = if lib.versionAtLeast (lib.versions.majorMinor crossPkgs.lib.version) "26.05" + then crossPkgs.llvmPackages_22 + else crossPkgs.llvmPackages_21; llvm = llvmBase // lib.optionalAttrs (libcxxSanitizers != null) { libcxx = llvmBase.libcxx.override { devExtraCmakeFlags = [ "-DLLVM_USE_SANITIZER=${libcxxSanitizers}" ];However, I don't think it matters? There is an iwyu task with GCC, which works fine, and the libcxx headers from clang 21 work fine with iwyu 24.0 and iwyu 26.0, so I don't think it matters much for this project.
I think I'll leave this as-is for now, but I am happy to push my diff, if reviewers insist.
- ryanofsky approved
-
ryanofsky commented at 1:06 PM on August 3, 2026: collaborator
Code review ACK 7402affd0ce7370fb6a82efb8268f3f168408519. Thanks for the update! It is good to decouple this from #175.
I also agree it would not be good to include 53dd2f20ade74fa76d671e24c5075b4d9704e112 here in it's current form, because it seems to be hardcoding a version number without any code comment explaining how the version number is chosen or why it is needed or what it needs to match. LLM agrees with hebasto there is a bug here and suggests the following change instead: 155794356555c2e2a3cda86a5ebb55b624ae14fb, but I haven't looked closely at it and need to understand the problem it fixes.
re: #296 (comment)
Not sure how to proceed here. I am happy to work on alternatives, but there is #175 with two acks, but it is not getting merged.
My bad! I've been focusing on new feature/bugfix/test coverage PRs, not the old build/ci ones and haven't revisited that in a while. I think the new PRs are more important and there's less disagreement about them so there's more progress that can be made with them in the short term.
For #175, I'm opposed to that change because:
- I don't know what problem it solves
- I am interested in maintaining compatibility with older versions not dropping it
- The
cmake_minimum_requiredis misleading about compatibility requirements - It bundles changes together that should be unrelated and orthogonal: changing which cmake policies are enabled & changing the minimum version of cmake required to avoid a fatal error about incompatibility.
I don't think I'm saying anything new here. My suggestion for making progress on #175 if it is important, is to split it up into more focused prs with individual goals, and to avoid misleading error messages about incompatibility in favor of more accurate messages and comments. I think it can also be kept open as it might begin to make more sense later if compatibility requirements change.
-
maflcko commented at 1:23 PM on August 3, 2026: contributor
LLM agrees with hebasto there is a bug here and suggests the following change instead: 1557943, but I haven't looked closely at it and need to understand the problem it fixes.
Ok, that is a bit involved, but using the llvm from the upstream iwyu package in the nixpkgs config appears fine as well.
Though, my preference would be to leave this as-is for now, given that any iwyu issues are unrelated and don't matter too much (recall that the iwyu ci config passes on all three proposed alternatives, and current master).
Maybe a separate pull can deal with iwyu?
-
ryanofsky commented at 1:32 PM on August 3, 2026: collaborator
Yes definitely, I dont think either of hebsto or LLM commits belong in this PR. Am planning to merge this PR and other pending ones in a batch soon.
-
ryanofsky commented at 3:20 PM on August 3, 2026: collaborator
Followup on IWYU:
Following up on the IWYU discussion above: after tracing real CI logs I believe there is no bug here and no pin is needed, so leaving this PR as-is seems right. My earlier suggested commit (1557943) was based on a wrong analysis and should be disregarded — it would actually introduce a small mapping-file mismatch rather than fix one.
What makes the current setup work:
CMakeLists.txtlines 107-114 (the nix workaround originally added for clang-tidy) copies the compiler's implicit include directories onto every compile command as explicit-isystemflags. Since IWYU is invoked with the compile command's flags, it parses the exact standard library the build uses: the pinned libc++ in the llvm job, the build gcc's libstdc++ in the default job. Thelibcxx.imphanded viaIWYU_MAPPING_FILEcomes from the samellvm.libcxxas those-isystemflags, so the mapping file and the parsed headers cannot disagree, regardless of which LLVM major the nixpkgsinclude-what-you-usepackage tracks. This is visible in CI job logs, where the--mapping_file=argument and the-isystem .../include/c++/v1argument reference the same libcxx store path.Without that context IWYU's behavior here is genuinely confusing to reason about, because the nixpkgs IWYU wrapper bakes in the include paths of its own toolchain's libstdc++, so bare
include-what-you-useinvocations outside the build silently analyze libstdc++ even in anenableLibcxxshell. That is what misled my earlier analysis — it only affects manual runs, not CMake-driven builds, where command-line-isystemtakes precedence over the wrapper's environment variables.Two data points from testing: 53dd2f2 fails to build on the nixos-26.05 channel (IWYU 0.26 requires clang 22 headers:
fatal error: clang/AST/NestedNameSpecifierBase.h: No such file), confirming maflcko's concern about hardcoding LLVM versions for IWYU. And recent CI runs where one IWYU job failed while the other passed — for example run 29304123578, where the llvm job required<type_traits>for the libc++-internal__libcpp_remove_reference_twhile the default job was satisfied — are the two jobs correctly enforcing different stdlib include requirements, working as cbb1e43 intended. The only real constraint in the current setup is that IWYU's embedded clang frontend must remain able to parse the pinned libc++ headers (clang supports its own and the previous libc++ release), and a violation would fail loudly with parse errors rather than silently misreporting.That said, the
CMAKE_CXX_STANDARD_INCLUDE_DIRECTORIESsetting is only justified as a temporary workaround — ideally shell.nix, not the build system, should be responsible for giving IWYU a correct environment. A follow-up commit works towards that: https://github.com/bitcoin-core/libmultiprocess/commit/f025f671196000cea658a8b6541e6a12ccfce169 documents the mechanism inCMakeLists.txtandshell.nix, and rebinds the compiler recorded in the IWYU wrapper to the shell's toolchain, so bare IWYU runs resolve the same standard library the build uses. - ryanofsky merged this on Aug 3, 2026
- ryanofsky closed this on Aug 3, 2026
- maflcko deleted the branch on Aug 4, 2026