ci: pin formatter toolchain and centralize the file list - #12
Merged
Conversation
Both format jobs failed on PR #11 without any source change causing it. The runner's clang-format tracks the Ubuntu image (18) while a developer machine may have 22, and setup-dart's `stable` had rolled to 3.13.0 while local was 3.12.2. The two clang-format versions format `struct stat sb {}` differently, and 22 flags its own -i output under --dry-run --Werror; Dart 3.13 collapses some call arguments differently than 3.12. Formatting is only a useful gate if it is reproducible, so pin it. - Pin the format job to DART_SDK_VERSION and the Flutter example job to FLUTTER_VERSION, both declared once at workflow level. Jobs that only build, analyze, or test still float against stable, so real breakage against new SDKs still surfaces here rather than in a user's project. - Stop apt-installing clang-format. scripts/format.sh provisions the pinned version via pip, and downloads a matching Dart SDK into .cache/ when the local one differs, so the script reaches CI's verdict on any machine instead of merely warning that the versions diverged. - Move the list of files to format into scripts/format.sh and have both CI jobs call it. The list was previously inlined in the workflow, which is how it came to reference `tests/` — a directory renamed in 0.2.2 — with nobody noticing. Also fix the README build snippet, which omitted -DAPPSTREAM_BUILD_TESTS=ON and so documented a `ctest` invocation that finds no tests, the same defect fixed in scripts/test.sh in 0.4.0, and document the formatting workflow. Verified: scripts/format.sh --check passes in both modes on a machine with clang-format 22 and Dart 3.12.2 installed, provisioning 18.1.8 and 3.13.0.
Findings from a proper reliability and security pass over the new script,
which the previous commit did not get.
- Fail loudly when a configured directory is missing. find reports the
error, carries on with the directories that do exist, and has its
non-zero status swallowed by the pipeline, so a renamed directory would
silently drop out of coverage while the run reported success. That is
exactly how native_tests/ went unformatted for three releases after
tests/ was renamed — the script written to prevent that bug reproduced
it. A non-empty file list is not evidence that everything was seen.
DART_PATHS gets the same guard for symmetry.
- Verify the Dart SDK archive against its published sha256 before
extracting it. The script downloads an archive and then executes
binaries from it; TLS attests to the transport, not to the bytes on the
bucket. A mismatch deletes the archive and aborts before anything is
extracted.
- Detect the host architecture instead of hardcoding linux-x64, which
would hand an arm64 developer an SDK that fails with a confusing exec
format error. Unknown architectures now fail with an actionable message.
The cache directory is keyed by architecture.
- Print only the first version match; distro strings repeat the number
("clang-format version 18.1.8 (Fedora 18.1.8-4.fc44)"), so the header
read "18.1.8 18.1.8".
Verified: shellcheck clean; the missing-directory guard fires; the
checksum path was exercised end to end, including a tamper test that
corrupts the archive after download and confirms it is rejected, deleted,
and never extracted. clang-tidy reports no warnings in project sources
(the one hit is the vendored dart_api_dl.cpp including a .c file by
design). clang-format ran last. 149/149 C++ and 45/45 Dart tests pass,
dart analyze --fatal-infos clean.
jwinarske
added a commit
that referenced
this pull request
Aug 14, 2026
The CI clang-format job globbed `find src include tests`, but tests/ was renamed native_tests/ in 0.2.2. find printed an error and moved on, and because the pipeline's exit status comes from clang-format rather than find, the job stayed green while silently checking nothing in the native test suite. It had therefore never been formatted, and carried 1233 violations under both clang-format 18 and 22. Add native_tests to CXX_DIRS in scripts/format.sh — which #12 made the single source of truth for both CI jobs — and apply clang-format 18.1.8, the pinned version. This is a mechanical reformat with no behavior change. Verified per file: every body's token stream is byte-identical with whitespace removed, and every #include set is unchanged as a multiset, so the only differences are line wrapping and include ordering (clang-format's SortIncludes). The C++ suite still builds and passes 149/149, the same count as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both format jobs failed on #11 without any source change causing it. This makes the formatting gate reproducible.
Why
Formatter output is version-sensitive, and neither formatter was pinned:
struct stat sb {}differently, and 22 additionally flags its own-ioutput under--dry-run --Werror— it cannot satisfy itself on that construct.setup-dartresolvesstable, which had rolled to 3.13.0 while the local SDK was 3.12.2. 3.13 collapses some call arguments differently.Either one turns CI red on a commit that did not touch the affected file. A format gate is only useful if it is reproducible.
Changes
Pin the version-sensitive jobs.
DART_SDK_VERSIONandFLUTTER_VERSIONare declared once at workflow level and consumed by theformatandflutter-flathub-catalogjobs. Jobs that only build, analyze, or test still float againststable, so genuine breakage against a new SDK still surfaces in CI rather than in a user's project.Make local runs match CI.
scripts/format.shprovisions the pinned clang-format via pip, and downloads a matching Dart SDK into.cache/, when the installed versions differ. A warning would have told you the versions diverged but still left you unable to produce the formatting CI wants — this actually produces it. CI runs the same script, and hits neither download path because its toolchain already matches.Centralize the file list. It previously lived inline in the workflow, which is how it came to glob
tests/— a directory renamed tonative_tests/in 0.2.2 — with nobody noticing, silently excluding the entire native test suite from formatting. It now lives in the script that both CI jobs call. Restoring that coverage is a follow-up PR, sincenative_tests/carries ~1233 violations.README fixes. The build snippet omitted
-DAPPSTREAM_BUILD_TESTS=ON, documenting actestinvocation that finds no tests — the same defect fixed inscripts/test.shin 0.4.0. Also documents the formatting workflow and the clang-tidy-before-clang-format ordering.Verification
scripts/format.sh --checkpasses in both--cxxand--dartmodes on a machine with clang-format 22 and Dart 3.12.2 installed, correctly provisioning 18.1.8 and 3.13.0 rather than using either. Apply mode is a no-op on a clean tree. Workflow YAML parses and both pins resolve.