Ci: Unify the linters and drive every workflow through task scripts - #1685
Open
DawidWesierski4 wants to merge 13 commits into
Open
Ci: Unify the linters and drive every workflow through task scripts#1685DawidWesierski4 wants to merge 13 commits into
DawidWesierski4 wants to merge 13 commits into
Conversation
DawidWesierski4
requested review from
Sakoram,
awilczyns,
moleksy and
soopel
as code owners
August 24, 2026 14:01
Five failures of the same kind: the build asserting something about its
environment instead of asking it.
- The rocky9 image reported "libxdp is absent after install" from a successful
install. xdp-tools and libbpf default LIBDIR to ${PREFIX}/lib64, which Debian's
pkg-config does not search and which does not exist on RHEL, so neither default
is portable and the multiarch directory libbpf was pinned to by hand only moved
the problem. Asking `pkg-config --variable pc_path` does not answer it either:
on Rocky 9 that is a string from a .pc file the distribution ships, and it omits
the compiled-in /usr/local/lib64/pkgconfig. So plant a probe .pc in each
candidate under the prefix and install into one pkg-config actually reads, and
register it with the dynamic linker, whose default path is just as
distribution-specific and which fails at load time rather than at link time.
- With libxdp visible, meson enables the manager's XDP target and clang stops on
gnu/stubs-32.h, which no package in the rocky9 image provides. The Ubuntu images
install gcc-multilib for this; glibc-devel.i686 is the RHEL counterpart.
- -flarge-source-files is gcc's, and clang rejects unknown -f arguments outright,
so every RxTxApp translation unit failed once the fuzz leg built with clang. It
is now offered to the compiler and used only if taken, like -msse4.2 above it.
- The fuzz targets link against GPU direct, which their sources call: the wrappers
#include the library sources they exercise, so those callees have to be on the
fuzz link line and not only on libmtl's. tests/unit carries the same conditional.
The assertion that started this was worth keeping: without it the image built
green with pkg-config reporting libxdp absent, which does not fail a build -- it
configures MTL without AF_XDP.
gcc builds these; clang's -Werror does not, so the fuzz leg -- which has to use clang for libFuzzer -- stopped in the test build: - st40p_handler.cpp carried a constant nothing reads; - st40i_tests.cpp captured a compile-time constant into a lambda that does not need it captured; - St30pRedundantLatency stored a latency and a starting time it never read. The constructor body looked like it used one of them, but it assigned to its own parameter, not to the inherited startingTime -- and initializeTiming(), which every caller runs straight after construction, sets that member anyway. So nothing observable changes.
Every access to these counters already goes through C11 atomics -- the tasklet adds to them, st*p_*_get_session_stats() reads them from another thread -- but the fields themselves were plain uint64_t. gcc's atomic builtins accept that; clang does not, so the library did not compile with clang at all, which is how it stayed unnoticed. Same codegen under gcc, because gcc was already emitting atomic operations. The USDT probes are the other half: their argument macros do arithmetic on what they are handed, and clang refuses that on an _Atomic operand, so the three sites that passed framebuff->stat now read it out first.
Auto pacing selection picks rl on any driver whose rl_type is TM, and then treats the two ways that choice can fail differently: a queue whose rate limit will not set falls back to tsc with a warning, while a tm hierarchy that will not build fails mtl_init outright. Both mean the same thing -- the driver in front of us has no rate limiter to offer -- and in auto mode neither is the user's choice to defend, so both should degrade the same way. Found while looking at an E830 whose PF grants its VF no QoS capability. That host crashes inside the iavf PMD before returning, so this does not rescue it; what it fixes is the case where a driver reports the missing capability properly, which today is a refusal to start rather than a session paced by tsc.
The workflows implemented themselves. Host setup, cache decisions, driver builds, test invocation and report collection were multi-line `run:` programs inside YAML, which meant no operation could be run, read or tested anywhere except on a runner, by pushing. Debugging a bare-metal job took a push and a forty-minute wait for a forty-megabyte log. Every operation is now a focused script under `.github/scripts/ci/`, verb-style and runnable by hand, with `Taskfile.yml` as the single entry point both the workflows and a developer call. The YAML orchestrates; it does not compute. `check-yaml-policy.sh` keeps it that way, and also rejects a third-party action that is not pinned to an immutable SHA. Three contracts the scripts encode, each one a failure this fleet actually had: **Jobs verify host state; they never install it.** apt packages, kernel modules, DMA bindings, the media share, the analyser -- a job that repairs what it finds hides drift in the host image and races every other job on the machine. So each check fails in seconds with the one command that fixes it, on the host that needs it. `configure-host.sh`, `media-assets.sh`, `ebu-list.sh` and `ice-required.sh` are that contract; a missing analyser is degraded mode rather than failure, because absence is not misconfiguration, and `MTL_CI_REQUIRE_COMPLIANCE=1` makes it fatal again on a host that has one. Lab facts come from `/etc/mtl-ci/runner.env` on the host that owns the hardware, not from GitHub secrets, which are a second copy of lab configuration kept in sync by hand. **A cache hit has to be usable, not merely present.** `actions/cache` saves in a post step that runs whether the job passed or not, so a run that died half-way through installing MTL stores the half-written tree under an unchanged key, and every later run restores it, skips the build and fails in the first consumer that resolves `mtl.pc`. The keys are content-addressed with an explicit schema (`cache-keys.sh`, `cache-schema.env`) so a fixed layout bug can be rotated past, entries are immutable, and `validate-cache.sh`, `validate-ice.sh`, `validate-jpegxs.sh` and `validate-dependencies.sh` reject a hit that cannot be used -- including an ICE module whose vermagic, kernel ABI fingerprint, compiler identity or Kahawai QoS capability does not match the host it is about to load on. **The acceptance virtualenv is a cache, not host state.** It is built from `requirements.txt` in the checkout, lives in the runner user's cache outside anything `git clean` touches, and is the same for every job on the host. So it is created once, rebuilt when the requirements change or when the host's python moves under it, built with `python3 -m venv`, `virtualenv` or `uv` -- whichever the host has, since none of them installs anything -- and names `python3-venv` when the host has none. `watch-run.sh` is the developer's side of the same layer: it resolves a commit from `--run/--pr/--sha/--branch`, defaulting to the pushed tip so it cannot report on a commit that only exists locally, distinguishes "queued, no runner yet" from a failure, and ends with the job, the failed step and the error lines. Twenty lines instead of the log.
The YAML now only orchestrates: each step names a `task ci:*` entry point, so the same command runs in a job and on a developer's machine. What is left in the workflows is the part that is genuinely about GitHub -- what triggers, what runs where, what may run at once, and what a queue is allowed to cost. - **A shared fleet is the scheduling problem.** Every NIC label is served by one host, so a second job at a label is a queue and not throughput. Superseded Build, smoke, base and docker runs are cancelled before they queue, and `pr-gate.yml` states its wait as a queue budget rather than a flat twenty minutes -- a build that has not started yet is not a build that is failing. When the budget does expire, the gate says it gave up because no runner came, which is a different thing from a failed build and used to look identical. - **A bare-metal job has to be bounded at every level.** Job timeout, run timeout and per-suite caps, so a hung test releases the host instead of holding a card for hours; `gtest-bare-metal.yml` carries the bounds the local harness asserts. - **The smoke matrix says what each leg is for.** The `i225` leg runs the low-bandwidth subset with no capture device and its own timeouts, because that card has no SR-IOV and two PFs rather than VFs; it is `optional` while the label is new. A leg with `no_capture` skips the analyser check it cannot use. - **`provision-runner.yml` is dispatch-only**, the single deliberate exception to "jobs install nothing": a human asks for a host to be prepared, by name. - Privileged bare-metal steps no longer open with a trace-fd error, the acceptance report the non-smoke suites write in place is left alone, and a host running an ICE driver that is not the one the suite needs is told so before it tests.
A change to a bare-metal job could only be tested by pushing it and waiting for a runner that might be busy for hours. `run-job.sh` runs the same job here -- `--runner docker` for anything that does not need a card, `--runner host` on a machine that has one -- through the same Taskfile entry points the workflow calls, with a local cache store that reproduces `actions/cache` semantics including its immutability. That is what proved the `i225` leg before CI ever dispatched it: two runs on a host with an I225-LM, `11 passed, 3 skipped` in about twenty-two minutes, while the leg itself was still queued behind a label nothing advertised. `tests/` holds the assertions about all of this that do not need hardware: cache schema rotation and poisoning, the gtest bounds, the wait-for-workflow script, the YAML migration, the virtualenv builders, and that a missing compliance analyser is not a gate. `task ci:test-dependencies` runs them, and so does the build workflow.
The loop an agent runs on CI is: push, find the run, find the failing job, find the failing step, find the error in a forty-megabyte log. The first four steps are mechanical and the last is a needle in ANSI-coded output, so all five became tools: `ci_pr_checks` and `ci_pr_failures` for what a pull request's checks say, `ci_last_log` for the interesting lines of one, and `ci_watch_run(pr=…)`, which blocks until the runs of a commit finish and then names the job, the step and the error line. The other half drives the local harness rather than reading GitHub -- `ci_list_jobs`, `ci_run_job`, `ci_test_pr`, `ci_list_tasks`, `ci_run_task`, `ci_cache_status`, `ci_check_ebpf`, `ci_diagnostics` -- so the same agent can reproduce a leg here instead of queueing behind the fleet. Failure counts and captured stderr are bounded, so a broken job cannot flood a context window; the repository argument is validated before anything is spawned; and the watcher's subprocess is given two minutes beyond its own poll deadline, so it reports the timeout itself instead of being killed while writing it.
The four agents each own a phase and nothing owned the sequence: which of them takes the next task, and which tasks may run at the same time. That decision was made ad hoc, once per session, and it is where this repository's own facts matter more than general judgement. So the orchestrator is the only agent permitted to invoke the others, and its parallel-safety test is five mutexes that are real here: a file, the build tree and acceptance virtualenv, a physical host with its hugepages and VF layout, a NIC label -- one host per label, so a second job at one is a queue and not throughput -- and the gate chain of a single change. It dispatches, records evidence and never implements: its edit tool is for the task board and its shell is for observation. Evidence is named per dispatch, including a non-empty `runner_name` for a CI job, which is the distinction the i225 analysis turned on. The `mtl-cicd` skill is the CI half of the routing matrix, which had no agent: the design contract that workflows orchestrate and scripts implement, and the inventory to read before editing either.
Resolve the datapath from the ports under test instead of hardcoding it, register the JPEG XS plugin tree, capture ST 2110 pcaps under sudo so compliance checks get complete captures, make room before recording raw video and delete the RX recording once it has been checked. Generated configs and the low-bandwidth cases are adjusted for the i225 smoke leg.
… leg Three things a person needs and could previously only get by reading the workflows: what the pipeline is (`cicd_setup_proposition.md`, with the prebuilt dependency problem it solves in `.github/github_actions_issue.md`), what a runner has to have before it can serve a label (`ci_runner_setup.md` -- the packages the jobs check but cannot fix, the media share, the EBU LIST analyser and when its absence is degraded mode rather than failure, and which host carries which card), and why the `i225` leg was red for two days without ever running (`i225_leg_analysis.md`). That last one is here because the evidence is not visible in the Actions UI. A job queued because the fleet is busy and a job queued because no host advertises its label are the same grey dot; they differ only in `runner_name`, and every i225 job record had an empty one until mtl-runner-12 came online. The one real failure after that took twelve seconds and named its own fix.
…ners The local harness, its cache store and the reports the jobs collect all land in the checkout, and `git status` has to stay readable for the suites that assert on it. CODEOWNERS gains the CI directories, which had no owner.
Three lint systems disagreed. format-coding.sh invoked clang-format, shfmt
and the Python tools directly and required each on PATH, so the version a
developer happened to have installed decided the result; linter.yml ran
super-linter against a second set of rule files; neither pinned the same
versions as the other.
.pre-commit-config.yaml is now the only place a tool, a version, an
argument or a file filter is declared. checkpatch.sh chooses which files
to feed it and how to report, format-coding.sh is its write-mode wrapper,
and the git hooks and linter.yml run that same list on Linux, macOS and
Windows. super-linter keeps only what the list cannot reproduce, including
GITLEAKS: its hook scans the staged diff and so cannot scan a whole tree
or a pull request.
.clang-format becomes a real file at the repository root because
clang-format searches upward from each source file, and a Windows checkout
without symlink support materializes a symlink as a text file, at which
point it silently falls back to LLVM style.
Every pin is at its latest release, and each was measured on its own with
the rest of the config held fixed:
clang-format 14.0.6 -> 22.1.8 isort 5.13.2 -> 8.0.1
black 24.4.0 -> 26.5.1 flake8 7.0.0 -> 7.3.0
ruff 0.4.1 -> 0.16.3 shfmt 3.7.0 -> 3.13.1
shellcheck 0.10.0 -> 0.11.0 markdownlint 0.43.0 -> 0.49.1
yamllint 1.35.1 -> 1.38.0 actionlint 1.7.7 -> 1.7.12
gitleaks 8.16.3 -> 8.30.0 textlint 14.0.4 -> 15.8.0
htmlhint 1.1.4 -> 1.9.2 pre-commit-hooks 6.0.0 (new)
Nine of those are byte-identical over the whole tree. Four are not, and
the interesting content of this commit is what was done about them.
**A version bump may not smuggle in a rule change.** Three tried:
ruff 0.16 reported 592 findings on a tree ruff 0.4 passed -- blind
excepts, datetime timezones, pyupgrade rewrites. None of it was new code.
ruff's *implicit* default rule set grew from about 40 rules to 413, and
.ruff.toml named no rules, so it pinned the version rather than the check.
It now selects E, W and F explicitly.
markdownlint 0.49 ships MD059 and MD060, which did not exist when this
config was vendored; MD060 alone reports 309 findings and rewrites 20
files. Both are off, with the reason recorded at the key.
textlint-rule-terminology 5.x rewrote prose in 15 files, including 18
lines of published CHANGELOG.md history, and replaced "blank line" with
"empty line" -- in a document describing git commit format, where "blank
line" is git's own wording. The *engine* is bumped to textlint 15.8.0; the
*word list* stays at terminology 4.0.1, because a word list is rule
content and rule content is versioned separately for exactly this reason.
clang-format 22 changed two things. It no longer reads `(type)-1` as a
cast, so `((mtl_iova_t)-1)` becomes `((mtl_iova_t) - 1)`; that is
whitespace, the tokens are identical, and for `((align)-1)` -- a macro
parameter, not a type -- the new spacing is simply correct. Accepted. But
from 18 on it also breaks a braced initializer whose elements carry
trailing comments to one element per line, which added 480 lines to
st_avx512_vbmi.c and destroyed the layout of six permute tables that are
written one pixel group per row so the pattern can be read against the
422le10 packing they implement. Those six now sit in a
`/* clang-format off */` region: a pin on the layout, not an exemption
from review.
flake8 is kept rather than folded into ruff, for one measured reason:
ruff 0.16 does not implement F824 ("dead `global` declaration") at all --
`ruff rule F824` answers "unknown rule". flake8 7.3 found two, both
genuinely dead (the names are only mutated, never rebound), both removed
here. .ruff.toml mirrors flake8's rule set rather than extending it so
that retiring flake8 later is a delete and not a re-measurement.
black's `--line-length 88` is black's own default, written down because
the documented Python line length was 120 while the formatter had been
wrapping at 88 the whole time. Not a conflict -- black wraps at 88, ruff
only rejects past 120 -- but only one of the two was stated, and a default
is not a pin.
Five pre-commit-hooks guards are added. None is a style check; each
mechanically enforces a claim this repository already makes and nothing
checked: destroyed-symlinks (the .clang-format-as-text failure above),
check-illegal-windows-names and mixed-line-ending (the platform support
claim), check-merge-conflict, and detect-private-key -- the only secret
scan that runs in a bare whole-tree checkpatch.sh, since gitleaks sees
only the staged diff.
Four more from that repo were probed and left off, two because they fail
on pre-existing defects that are not this commit's to fix, both now
recorded in doc/coding_standard.md §3.1:
check-json 20+ tests/tools/RxTxApp/script/**/*.json use
trailing commas; json-c accepts them, strict JSON
does not.
check-case-conflict tests/acceptance/mtl_engine/RxTxApp.py and
rxtxapp.py are both tracked, so this tree cannot be
checked out on a case-insensitive filesystem --
which contradicts the macOS and Windows support
claimed above. Renaming a module mtl_engine imports
is not a lint change.
The remaining source churn is clang-format 22 and black 26 improving what
they touch: `struct st40_meta m {}` becomes `m{}`, single-expression
lambdas collapse, and black hugs a sole `textwrap.dedent` argument.
check-illegal-windows-names earned its place immediately. patches/ was
excluded globally -- a vendored patch series must not be reformatted -- and
a global exclude turned out to be wrong in both directions. It protected
nothing, because every formatter here is selected by language type and a
*.patch file is none of those types; deleting it changed no hook's result
over the whole tree. And a hook-level exclude cannot un-exclude a global
one, so it silently disabled the one hook that reads paths instead of
content. Blinded, that hook reported "no files to check" while
patches/dpdk/26.03/0012-net-ice-e830:-use-direct-MMIO-for-PHC-update.patch
sat in the tree. A colon is not a legal filename character on Windows, so
`git checkout` there refuses the whole clone with "error: invalid path"
and exit 128 -- meaning the Windows support claimed in §6 had been broken
for as long as that file existed, and the new Windows CI job could never
have gone green no matter what the linters said. The file is renamed
(nothing references it by name; script/build_dpdk.sh globs *.patch and the
0012- ordering prefix is preserved), the exclusion now sits only on
mixed-line-ending, the one hook that does read every file regardless of
type, and the guard is what keeps the name legal from here on.
Renaming the CI job broke build.yml's linter gate, so that is fixed here too.
wait-for-linter polled for super-linter's check run, "Lint Code Base",
which stopped existing the moment checkpatch replaced it. A missing check
is not a failure in that action, it is a wait, so every pull request spent
ten minutes timing out and reported infrastructure flake rather than a
configuration error. The gate now names all four check runs that linter.yml
actually produces -- the three checkpatch OSes and the residual job -- and
build still declares needs: [wait-for-linter, checksums], so a lint failure
skips the DPDK build instead of paying for it. wait-for-workflow takes a
newline-separated list and requires every entry to reach success; a single
name is unchanged, which is what the other two callers pass.
The coupling is by check-run name and nothing validates the two lists
against each other, so all three files now say so at the point where the
mistake would be made: both linter.yml job names, the gate itself, and
doc/coding_standard.md §4.1. The action also reads its inputs from env
instead of interpolating them into the script body, which a multi-line
value would have broken outright.
18 hooks, ./checkpatch.sh clean and idempotent, ColumnLimit stays 90.
Signed-off-by: Wesierski, Dawid <dawid.wesierski@intel.com>
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.
Rebased onto
mainated7dfaa6. 13 commits.What this is
The CI/CD rework: workflow operations moved into task scripts, a local harness
that runs a CI job off the fleet, a CI diagnostics MCP server, the orchestrator
agent and
mtl-cicdskill, the acceptance-suite hardening for the validationfleet, the architecture/host-contract/i225 docs, and the linter unification.
Linter unification
.pre-commit-config.yamlbecomes the single source of truth for which tool,which version, which arguments and which files.
checkpatch.shchooses the fileset and reports;
format-coding.shis its write-mode wrapper;linter.ymlorchestrates only. super-linter keeps just what the hook list cannot reproduce --
notably
VALIDATE_GITLEAKS, whose hook scans the staged diff and so cannot scana whole tree or a pull request. That residual job is now an allow-list, which is
also what keeps ansible-lint off: super-linter refuses to start when true and
false values are mixed, so the
VALIDATE_ANSIBLE: falsethe deny-list neededwould now break the job.
./checkpatch.shis clean and idempotent over the whole tree: 17 hooks, exit 0.Notes for review
.clang-formatbecomes a real file at the repository root. clang-formatsearches upward from each source file, and a Windows checkout without symlink
support materializes a symlink as text, at which point it silently falls back
to LLVM style.
destroyed-symlinksis what keeps that honest.patches/dpdk/26.03/0012-net-ice-e830:-...patchis renamed. A colon is not alegal Windows filename character, so
git checkoutthere refused the wholeclone with exit 128 -- the Windows support this repository claims had been
broken for as long as that file existed.
check-illegal-windows-nameskeepsthe name legal.
wait-for-lintergate now names all four check runslinter.ymlproduces. It previously polled for super-linter's "Lint Code Base", which
stopped existing; a missing check is a wait rather than a failure in that
action, so every pull request spent ten minutes timing out and reported
infrastructure flake instead of a configuration error.
Still open
.github/claude/has nomtl-orchestrator.md, noskills/mtl-cicdsymlink andno
mtl-ci-localentry inmcp.json.ed7dfaa6deliberately left those unwiredbecause the targets did not exist on
mainyet; after this branch they do, sothat wiring is a sensible follow-up. The Copilot side under
.github/agents/and
.github/skills/has all three.