Set matching CC alongside CXX in Linux CI matrix - #4863
Conversation
|
The I don't have permission to re-run just the failed jobs (not a repo admin) — happy to push an empty commit to retrigger if useful, or this should resolve on its own next time CI runs against this branch. |
|
I triggered a re-run. |
|
Thanks for the re-run -- that surfaced something worth flagging. Good news: the compiler downloads succeeded this time (no more But Importantly, this isn't something this PR's diff introduces (it only touches the workflow file), and it isn't new in the code -- it's a preexisting incompatibility between the C API and real Clang 3.6 that CI never actually caught before, for exactly the reason this PR exists:
Since fixing |
If there is an easy fix, then I suggest doing it as part of the current PR. Otherwise, we could disable the C test in the problematic config for now. Thanks for looking into it! |
|
Dug into this -- root cause is confirmed and it's not a bug in fmt's macro. FMT_MAKE_ARG's _Generic dispatch in fmt-c.h relies on the controlling expression decaying char[N] string literals to char*/const char* before type matching. Whether _Generic's controlling expression decays arrays was genuinely ambiguous in the C11 wording (WG14 N1930) -- GCC always applied the decay, Clang didn't, until the fix for LLVM PR16340 landed in Nov 2015 (clang 3.8). Clang 3.6.2 (Feb 2015) predates that fix by a year, so "foo" never matches char*/const char* and falls through to the 0-arg default, which is then called with an argument -- exactly the reported error. I looked hard for a macro-level fix and don't think there's a safe one for this PR. The obvious workaround -- forcing decay by wrapping the argument as (x)+0 or 1 ? (x) : (x) -- does force the array decay, but it also triggers ordinary C integer promotion on every small-integer case in the table (char, bool, unsigned char, short, ...), on every compiler, not just old Clang. I verified this concretely: it reroutes a plain char argument to the int case instead of char, which would silently change how the C API formats char/bool/short arguments everywhere, not just paper over Clang 3.6. There's no way to scope the workaround to "just the array case" without already knowing the argument's type at macro-expansion time, which is the very thing _Generic exists to determine, so this really is a compiler bug rather than something fixable in fmt-c.h without a bigger redesign. Given that, I went with your second option: skipped just the c-test target (via One thing worth flagging on my own work: my first pass at this had a real bug -- the templated Pushed. |
vitaut
left a comment
There was a problem hiding this comment.
The lint warning has been fixed in main (you need to rebase) but it looks that the test-only opt out was insufficient.
The Configure step only set CXX, so CMake's C compiler detection fell back to whatever the default happened to be on the runner, independent of which C++ compiler the matrix entry was actually testing (e.g. CXX=clang++-3.6 but CC left to detect GCC 11). Derive CC from the same matrix.cxx value the job already installs a matching compiler for.
Setting CC from matrix.cxx means the clang++-3.6 job now configures with real clang 3.6 instead of falling back to the runner's default GCC, which exposes a compile failure in test/c-test.c: clang predates the LLVM PR16340 fix (landed in 3.8) that applies array-to-pointer decay to the controlling expression of _Generic, so fmt-c.h's FMT_MAKE_ARG dispatch never matches a string literal (char[4]) against its char*/const char* associations. It falls through to the zero-argument default association, which is then called with one argument. The guard is at configure time rather than in the CI Test step. c-test is part of the default target, so `cmake --build` compiles it before ctest ever runs -- excluding it with `ctest -E` cannot help, because the job has already failed in the Build step. Guarding the add_executable also means anyone building fmt's tests with an old clang benefits, not just this one CI job. enable_language(C) stays first and unconditional, since CMAKE_C_COMPILER_VERSION is not set before it. fmt-c itself (src/fmt-c.cc, C++) still builds on that job; only the C-consumer smoke test is skipped.
841a3c3 to
5ae4721
Compare
|
Both addressed — rebased onto current main (picked up c851fbe, so cmake-format is green again), and the opt-out moved to where it can actually take effect. Why the previous one couldn't work. So the guard is now at configure time, in enable_language(C)
# Clang < 3.8 doesn't apply array-to-pointer decay to the controlling
# expression of _Generic (https://llvm.org/PR16340), so fmt-c.h's argument
# dispatch doesn't match string literals there.
if (CMAKE_C_COMPILER_ID STREQUAL "Clang" AND CMAKE_C_COMPILER_VERSION
VERSION_LESS 3.8)
message(STATUS "Skipping c-test: _Generic is broken in Clang < 3.8")
else ()
...
endif ()
The Verified locally (cmake 4.4.0, AppleClang 21):
One correction to something I said earlier. I claimed there was no way to scope a macro-level workaround to just the array case. That was too strong. One thing to watch on this run. With If you'd rather feature-test than version-gate, a |
|
The one red check that just appeared ( Step 3 Same commit, That leaves 45/46 green, with |
|
g++-4.9 is now green too after a re-run. |
|
Merged, thank you! |
Fixes #4858
Problem
The Linux CI matrix deliberately varies
matrix.cxxacross old and new compilers (g++-4.9,clang++-3.6,clang++-20, etc.) to test compatibility, but theConfigurestep only setsCXX. With noCC, CMake's C compiler detection falls back to whatever default happens to be on the runner (as reported:clang++-3.6for C++, butGNU 11.4.0detected for C) — completely unrelated to the compiler version the job is actually meant to be testing.Fix
Add a small step before
Configurethat derivesCCfrom the samematrix.cxxvalue each job already installs a matching compiler for (clang++-N→clang-N,g++-N→gcc-N):Testing
This is a CI-config-only change, so the real test is the workflow run itself — I checked every distinct
matrix.cxxvalue used in this file (g++-4.9,g++-11,g++-13,g++-14,clang++-3.6,clang++-11,clang++-14,clang++-20) against the correspondinginstall/package steps in the same file to confirm a matchinggcc-N/clang-Nbinary is actually installed for each before relying on it (e.g. the "Install GCC 4.9" step explicitlydpkg -is agcc-4.9package, "Install Clang 3.6" explicitly installsclang-3.6, etc.) — the two entries without an explicit install step (g++-13/g++-14onubuntu-24.04, oneclang++-14variant) rely on the runner image's default toolchain, where the C and C++ drivers for the same version are always installed together. I also verified the string-substitution logic locally in bash against all 8 values (clang++-3.6correctly does not get double-substituted by theg++→gccreplacement, since theclang++→clangsubstitution runs first and already consumes the trailingg++substring), and validated the YAML withruby -ryaml. I can't run the actual matrix locally, so I'll be watching this PR's own CI run closely.