Add CI: per-package checks and repo structural gates - #20
Conversation
AUTHORING.md §6 says "CI will run flutter pub get + flutter analyze on the new package." There is no CI. No pull request in this repository has ever reported a check, so every invariant the porting checklist lists has been enforced by whoever happened to look. Two jobs. `package` fans out over packages/, discovered from the tree rather than a hardcoded list so a new package is covered the moment it lands. Per package: resolve, `dart analyze` on lib/ and test/, run the Dart tests, then resolve and analyse the example. Analysis is scoped to those directories on purpose — analysing the package root descends into example/ and into nested packages that were never resolved (audioplayers_tvos ships an example/server with its own pubspec) and reports their unresolved imports as this package's errors. `dart analyze` is fatal on errors and warnings and reports infos without failing, which is the gate we want: the examples are ported upstream code and carry lint infos that are not ours to churn. `repo` checks five structural invariants with no Flutter and no network: R1 every package has a row in the root README's Ports table R2 pubspec version == podspec s.version == top CHANGELOG heading R3 the files a published package cannot ship without R4 a committed pubspec_overrides.yaml is excluded via .pubignore R5 flutter.plugin.platforms.tvos.pluginClass is declared Each has been broken or nearly broken. R1 is missed by the open firebase_performance_tvos port. R2 was already violated: path_provider_tvos declared 0.0.3 in pubspec and 0.0.2 in its podspec, sqflite_tvos 0.0.2 and 0.0.1 — the podspec version is inert while pods resolve by :path, so nothing surfaced it. R5 fails at runtime on a device, as MissingPluginException at the first call, with nothing pointing at the cause. Both fixed here so the gate is green from the first run, along with a genuine analyzer error in device_info_plus_tvos's example: runZonedGuarded's handler was typed (dynamic, dynamic), and dynamic is not assignable to StackTrace?. Verified by running exactly what the workflow runs, locally, across all sixteen packages: resolve, analyse and test all pass, and the example of every package resolves and analyses clean. Deliberately out of scope: nothing here compiles tvOS sources, resolves a podspec or touches a simulator. Stock Flutter is enough for resolution, analysis and the Dart tests, which is why this runs on ubuntu rather than a macOS runner. The native half stays a reviewer's job.
A review of the first revision measured it under `trace --count --missing`: on a green run, 67 of 105 executable lines ran. Every fail() site was cold, including the append inside fail() itself. The failure half of the gate had never executed anywhere, and four of the five rules were accepting trees they were written to reject. R5 was the worst, because it is the one CI cannot catch any other way — stock flutter_tools does not validate the `tvos:` key at all. Its scan broke only on a column-0 line, so it walked out of the `tvos:` block into whichever platform block followed and returned *that* platform's pluginClass. A package declaring only `ios: pluginClass:` passed the rule whose whole purpose is to prevent a MissingPluginException on a device. The rest of that class: R1 matched a bare substring anywhere in the README, so a prose link satisfied "has a row in the table"; R4 matched a substring in .pubignore, so `# pubspec_overrides.yaml` and even `!pubspec_overrides.yaml` counted as exclusion; the CHANGELOG check matched the first *numeric* heading, so `## Unreleased` on top was skipped and the released entry below validated instead. All four share one root cause — a YAML parser written in regex — so the parsing is now PyYAML and the string-matching rules are line-scoped. Two structural fixes: - Packages are enumerated as directories, not as directories-containing-a- pubspec. The old form made a package with a misnamed pubspec vanish from the run rather than fail it, and it made the R3 "pubspec.yaml is missing" branch unreachable by construction. - The workflow no longer derives its own package list. `--list` is the single definition, and the job fails if the discovered count and the directory count disagree, rather than quietly building fewer packages than exist. `--selftest` runs in CI before the real check: 13 cases, each asserting a rule both fires on a bad tree and stays quiet on a good one. Asserting only the first direction would have caught none of the four defects above — every one was a silent pass on a tree that superficially looked compliant. It moves 78 previously-unexecuted lines into every run. R4's rationale was also wrong. `dart pub publish` already drops a package's own root pubspec_overrides.yaml — verified with `--dry-run` on a copy with .pubignore removed. What ships is the *nested* example/ override, and even then a consumer resolving a hosted dependency ignores that dependency's overrides, so "makes the package uninstallable" overstated it. The rule now checks the file that actually ships and says what it actually costs. Workflow fixes: - `flutter test` was gated on `ls test/*.dart`, which is not recursive while `flutter test` is. Moving tests into test/unit/ would have stopped every package running them, green, while the log said there was no test directory. - Added a terminal `ci` job. Matrix legs are named after their packages, so their check names come and go with the tree and branch protection cannot require them; a new package's failure would not have blocked its own merge. - `cancel-in-progress` is now limited to pull requests, so merges to main stop cancelling each other's runs. - Skips announce themselves with `::warning::` instead of an echo, and an example with no lib/ is an error rather than a silent pass. Comment corrections: the word-splitting rationale claimed a hazard that cannot occur under Actions (bash does split; the breakage is on a zsh prompt); the example step said "errors fail, infos do not" while warnings are fatal too; the matrix-name filter was described as injection defence, but `working-directory` is not a shell sink and a fork PR can edit this file anyway — what contains it is the read-only token. Also recorded that eight examples inherit the plugin's strict analyzer settings and are therefore checked more strictly than the eight that carry their own options file.
The self-test added last round certifies every rule, so the first thing worth asking is whether it can certify nothing. It could: emptying CASES printed "Self-test passed: every rule fires on a bad tree and stays quiet on a good one" and exited 0, with zero assertions — the same anti-pattern it was written to catch, one level up. It now asserts that the union of rules provoked across all cases covers R0-R5, so a rule with no case fails the run. That assertion immediately found R0 untested — the one rule whose whole job is to stop a vacuous pass. Two cases added for it. The oracle was also too weak. `expect in rules` accepted a case where the expected rule fired from a *different* branch than the case was written for: delete R2's "heading is not a version" arm and control falls through to the version-mismatch arm, which still emits R2, so the case stayed green over dead code. It now compares the exact rule set, which is what the R4 loop already did. Measured by mutation rather than by reading. Three regressions that survived the previous self-test are now killed: `REQUIRED_FILES = []` (R3's headline check had no coverage at all), `CASES = []`, and reverting `discover()` to "directories containing a pubspec" — the last being a regression this file's own docstring records having already made once. 28 cases now, from 13. R4 was still accepting trees where the override genuinely ships. `.pubignore` is gitignore syntax, and the implementation got three things wrong: it skipped `!` lines outright, so a negation could never take effect; it compared with `endswith` on the whole line, so `my_pubspec_overrides.yaml` and `other_dir/pubspec_overrides.yaml` both counted; and a leading `/` was ignored, so a root-anchored entry appeared to cover `example/`. Now fnmatch against the real relative path with last-match-wins, and the rule walks for a nested override anywhere rather than stat-ing `example/` alone. Five near-miss cases added — the shapes the old form accepted. Other findings: - An unparseable pubspec `continue`d past four rules that do not depend on it. A package broken five ways reported one problem and said nothing about the four checks that declined to run — the failure mode this file's own docstring calls out. Only the version comparison and R5 are skipped now, and the skip is stated in the message. - `--selftest` accepted and discarded a root argument, so `check_repo.py . --selftest` would have run the self-test, skipped the real check, and exited 0. Collapsing the two CI steps into one command was a one-edit landmine. argparse now rejects it, along with typo'd flags that previously read as "no flags at all". - PyYAML's `safe_load` types `version: 1.10` as the float 1.1, so a tree whose three files literally agree would have been reported as drifted. BaseLoader keeps scalars as text, which is what a checker comparing versions wants. - A `## 0.0.1` inside a fenced code block was read as the newest changelog heading — the original defect through a different door. Fences are stripped first. - An unreadable `.pubignore` was reported as "the override is not excluded", sending the reader to the wrong file. - The README row regex rejected `./packages/x`, `packages/x#readme` and titled links — all legitimate rows, all false failures waiting for a cosmetic edit. Workflow: - A failing `jq` inside a command substitution used as an `echo` argument is invisible to both `set -e` and pipefail, because `echo` succeeds. The step went green having written an empty package list, and the run then died on "Unexpected end of JSON input" pointing at the wrong job. Assigned and validated on its own line. - `dirs=$(ls -d packages/*/ ...)` ran before the empty-list guard, so an empty packages/ killed the step on `ls` before the guard could explain itself. `find` instead. - The one remaining silent skip (`dart analyze test`) now warns like the others.
A comment audit checked every factual claim in these two files against the toolchain and the repo. Twelve held. The three that did not were all ones I had argued from first principles instead of running: - **Concurrency.** The comment said limiting `cancel-in-progress` to pull requests keeps main's build record intact. It does not. GitHub keeps at most one *pending* run per concurrency group and cancels the previous pending one regardless of this setting, so back-to-back merges to main can still skip a build. The setting only guarantees that an already-running build finishes. Rewritten to claim that and no more. - **Analyzer severity.** The comment said the inherited strict-casts / strict-inference / strict-raw-types "produce errors". Measured: strict-casts produces errors, the other two produce warnings. Both still gate, because `dart analyze` is fatal on warnings — but a maintainer grepping for "error" would have been misled. - **Which examples are stricter.** The comment said the examples carrying their own analysis_options.yaml are "checked more loosely", and warned against unifying by lowering the bar. Three of them are not looser at all: path_provider's repeats the plugin's strict block verbatim, sqflite's repeats two thirds of it, and flutter_secure_storage's uses very_good_analysis, which is stricter than anything else in the repo. Getting the direction backwards on the strictest example was the wrong error for a comment whose whole job is to stop someone lowering the bar. Also dropped the hardcoded "eight" counts, which would have gone stale the moment a seventeenth package landed, and tightened `tvos_plugin_class`'s account of its own bug: a package with no `tvos:` block at all was always caught, and the shape that slipped through was a pluginClass-less `tvos:` block followed by a platform that had one. The audit verified the rest empirically, including the two claims most worth having checked: `flutter pub get` really does accept a pluginClass-less `tvos:` block in silence (exit 0, where the same block under `ios:` exits 1), which is why R5 has to exist at all; and `dart pub publish` really does drop the root pubspec_overrides.yaml while shipping the nested one, which is what R4 now targets.
MAUstaoglu
left a comment
MAUstaoglu
left a comment
There was a problem hiding this comment.
Review
The gate is green and correct on the tree as it stands, and I merged it locally against all six pending Firebase ports to confirm it does the job it was written for — it catches every one of them on R1, which is exactly right. Five findings below, none of them blocking: two are latent (no package in the tree triggers them today), one is a coverage gap rather than a regression, two are operational.
The three package edits are correct. Both podspec bumps align podspec ↔ pubspec ↔ CHANGELOG (path_provider_tvos 0.0.3, sqflite_tvos 0.0.2), which is what R2 demands of them, and the device_info_plus_tvos closure signature is the strict-casts fix the workflow comment describes.
1. pubignore_excludes misses directory-prefix patterns — false R4 on a correct package
.github/scripts/check_repo.py:149-154
Both slash-bearing branches treat the pattern as a whole-path glob, so a pattern that excludes a directory never excludes the files beneath it. In gitignore semantics excluding a directory excludes its whole subtree, and pub applies the same semantics to .pubignore.
$ git check-ignore -q example/pubspec_overrides.yaml # with each pattern as the sole .gitignore line
/example/ git: IGNORED
/example git: IGNORED
example/ git: IGNORED
pubspec_overrides.yaml git: IGNOREDThe checker disagrees on the first two:
pattern git says checker
/example/ True False <-- MISMATCH
/example True False <-- MISMATCH
example/ True True okSame defect in the elif "/" in pattern branch: example/nested against example/nested/pubspec_overrides.yaml returns False.
So a package that writes /example/ in its .pubignore — a perfectly ordinary entry — gets R4:
example/pubspec_overrides.yamlis committed but not excluded by .pubignore
→ addpubspec_overrides.yamlto .pubignore
…which it has effectively already done. Nothing in the tree uses the anchored form today, so this is latent, but it's the one rule whose failure message would send the author looking for a problem that isn't there.
Both branches need to treat a match on any leading path component as a hit, not just a match on the full relative path.
2. The overrides mask hosted constraints from CI entirely
.github/workflows/ci.yml:130
flutter pub get honours a committed root pubspec_overrides.yaml, and four packages on main ship one (cloud_firestore_tvos, firebase_auth_tvos, firebase_messaging_tvos, firebase_storage_tvos). For those, the hosted constraint in pubspec.yaml — the one consumers actually resolve against — is never exercised.
I made the constraint unsatisfiable by anything that could ever exist on pub.dev and ran the workflow's own steps:
# packages/firebase_auth_tvos/pubspec.yaml
- firebase_core_tvos: ^0.0.1
+ firebase_core_tvos: ^9.9.9$ flutter pub get
Got dependencies!
$ dart analyze lib
No issues found!Green. The package would be unresolvable for every consumer the moment it is published.
The header comment is careful about what the workflow doesn't cover — podspecs never resolved, tvOS sources never compiled, nothing on a simulator — and this belongs on that list, or better, gets closed: one resolve with overrides disabled would catch it. Worth weighing against the fact that these four overrides are obsolete anyway now that firebase_core_tvos 0.0.1 is published (which is the cleanup already raised on #9).
3. R2 forces a podspec version CocoaPods rejects, for any +build version
.github/scripts/check_repo.py:279
R2 compares versions as exact strings, and the self-test pins that deliberately:
("R2 version with build metadata is compared verbatim",
{"pubspec.yaml": ... .replace("version: 0.0.1", "version: 0.0.1+1")}, "R2"),So version: 0.0.1+1 — the ordinary Flutter convention — requires s.version = '0.0.1+1'. CocoaPods' Pod::Version derives from Gem::Version, which does not accept +:
$ ruby -e 'require "rubygems"; %w[0.0.1 0.0.1+1 1.2.3+4].each { |v| puts "#{v} -> #{Gem::Version.correct?(v)}" }'
0.0.1 -> true
0.0.1+1 -> false
1.2.3+4 -> falseThat leaves an author with no satisfying move: a red R2, or a podspec that raises. Stripping Dart build metadata (everything from +) before the comparison resolves it, and the self-test case should invert to match. Latent — every package is on a plain 0.0.x today.
4. if: always() reports cancelled runs as failed
.github/workflows/ci.yml:209
always() is also true when the run is cancelled, so the gate executes and asserts [ 'cancelled' = 'success' ] → the required check goes red rather than neutral. With cancel-in-progress: true on pull requests that happens on every superseded run: push twice in quick succession and the first run posts a failed ci. Manual cancellations need an explicit re-run to clear.
if: ${{ !cancelled() }} keeps the property the comment is actually after — a skipped job must not count as success — without misreporting cancellation.
5. find and discover() disagree on what a directory is
.github/workflows/ci.yml:85
The guard's premise is one shared definition of "a package", but the two sides differ: discover() skips names starting with . (check_repo.py:197) and follows symlinks via os.path.isdir, while find -type d does neither. Put any dotted directory directly under packages/ — a .template/ scaffold, a tool's cache — and the step hard-fails with
Discovered 16 package(s) but packages/ holds 17 directories.
Something under packages/ is not being checked.
…blaming a silent drop that isn't happening. Mirroring the filter (-not -name '.*') or taking the count from the checker alone keeps the two definitions genuinely single.
What I verified
Merged this branch with each of the six open Firebase PRs in a scratch clone and ran the real gates.
The self-test and the structural run are sound on the current tree:
$ python3 .github/scripts/check_repo.py --selftest
Self-test passed: 28 cases, every rule in R0, R1, R2, R3, R4, R5 both fires and stays quiet.
$ python3 .github/scripts/check_repo.py .
Checked 16 package(s) under packages/
OK — README rows, versions, required files, overrides and tvOS plugin classes all consistent.The Flutter half passes for all six pending ports (pub get, dart analyze lib, dart analyze test, flutter test, example pub get + dart analyze — 3.47.0 locally, against the pinned 3.44.0 here). The only structural failure across all six is R1, on every one of them — so this gate lands and immediately does what it was written to do.
Two ordering hazards it surfaces that were otherwise invisible. #8 and #19 each commit two GeneratedPluginRegistrant.{h,m} files into a different PR's package directory (#8 → firebase_analytics_tvos, #19 → firebase_app_check_tvos). Merging #8 without #7 first gives:
R3 firebase_analytics_tvos pubspec.yaml is missing
R3 firebase_analytics_tvos tvos/ ships no .podspec
— a phantom package with nothing in it but build output. R3 catching that is a good advertisement for the discover() docstring's reasoning about enumerating directories rather than pubspecs.
One note on sequencing rather than on the code. R1 only checks package → row, never row → package, so a README row for a not-yet-merged package is legal. That means the six rows can be pre-seeded in a single commit after this lands, instead of each PR appending at the same anchor and conflicting with the previous five. Verified end-to-end: seed the rows, then all six merge clean with no rebases, gate green throughout, final tree 22 packages.
Nice piece of work — the self-test earning its place is the part I'd have expected to be missing, and the failure-path coverage assertion at the end of it is the right instinct.
…ing a bug
All five findings reproduced before acting on them.
**`.pubignore` directory patterns.** Excluding a directory excludes its subtree,
and the checker did not: `/example/` and `/example` are IGNORED by
`git check-ignore` while `pubignore_excludes` returned False, so a package with
an ordinary anchored entry got
R4 example/pubspec_overrides.yaml is committed but not excluded by .pubignore
→ add `pubspec_overrides.yaml` to .pubignore
telling the author to add an entry they had effectively already written. An
anchored pattern is now matched against every leading prefix of the path. All
seven shapes I tested — anchored, trailing-slash, bare name, scoped-to-another-
directory, near-miss filename — now agree with `git check-ignore`.
**R2 demanded a podspec version CocoaPods rejects.** Dart allows `0.0.1+1`;
`Pod::Version` derives from `Gem::Version`, which returns false for `correct?`
on anything containing `+`. So for any package using the ordinary Flutter
convention, R2 offered a red gate or a podspec that raises. The release part is
now compared and the build metadata dropped.
Worse than the rule was the test: the self-test *asserted* the broken
behaviour — "R2 version with build metadata is compared verbatim" required the
podspec to repeat `0.0.1+1`. A case that pins a bug is the one kind that makes
the bug permanent. Inverted: `0.0.1+1` against a `0.0.1` podspec is now
asserted to stay quiet, and a genuinely wrong podspec still fires.
**`find` and `discover()` disagreed on what a directory is.** `discover()` skips
dotted names, `find -type d` did not, so a `.template/` under packages/ made the
counts differ and hard-failed the step blaming a silent drop that was not
happening. The filter is mirrored, and the two definitions are now genuinely
one.
**`if: always()` reported cancelled runs as failed.** It also runs on
cancellation, where the job asserts `'cancelled' = 'success'` and posts a red
required check — on every superseded PR run, given `cancel-in-progress`.
`!cancelled()` keeps the property that matters (a skipped job must not count as
success) without the misreport. Two reviewers flagged this independently and I
left it the first time.
**One blind spot documented rather than closed.** `flutter pub get` honours a
committed root `pubspec_overrides.yaml`, and four packages ship one, so for
those the hosted constraint in pubspec.yaml is never exercised — an
unsatisfiable `^9.9.9` resolves green, verified. Closing it needs a second
resolve with the overrides moved aside; the cheaper fix is deleting the four
overrides, which are obsolete now that firebase_core_tvos is published. Named
in the header's "does not cover" list until then.
2 participants
AUTHORING.md§6 says "CI will runflutter pub get+flutter analyzeon the new package." There is no CI — no.github/at all, and no pull request in this repository has ever reported a check. Every invariant the porting checklist lists has been enforced by whoever happened to look.package— one job per packageThe matrix is derived from the tree, not hardcoded, so a new package is covered the moment it lands. That is the case a hardcoded list always misses.
Per package: resolve →
dart analyzeonlib/andtest/→ run the Dart tests → resolve and analyse the example.Analysis is scoped to those directories deliberately. Analysing the package root descends into
example/and into nested packages that were never resolved —audioplayers_tvosships anexample/serverwith its own pubspec — and reports their unresolved imports as this package's errors: 19 of them, none real.dart analyzeis fatal on errors and warnings and reports infos without failing, which is the gate we want. Six examples carry lint infos inherited from upstream (up to 26 infirebase_messaging_tvos); churning ported example code to satisfy our lints is not worth it, andlib/is clean everywhere.repo— structural invariants, no Flutter, no networkAUTHORING.mdstep 4. Missed by the openfirebase_performance_tvosport (#9) — the package is otherwise undiscoverable from the repo indexversion== podspecs.version== top CHANGELOG headingpubspec_overrides.yamlis excluded via.pubignoreflutter.plugin.platforms.tvos.pluginClassis declaredMissingPluginExceptionat the first call, at runtime, on a device, with nothing pointing at the causeThree pre-existing problems fixed, so the gate is green from run one
path_provider_tvosdeclared0.0.3inpubspec.yamland0.0.2in its podspec.sqflite_tvosdeclared0.0.2and0.0.1.The podspec version is inert while pods resolve by
:path, so nothing ever surfaced the drift. No package version changes and nothing needs republishing — this is metadata catching up to what is already released.device_info_plus_tvos/example/lib/main.darthad a real analyzer error:runZonedGuarded's handler was typed(dynamic error, dynamic stack), anddynamicis not assignable toStackTrace?. Now(Object error, StackTrace stack).Verification
Ran exactly what the workflow runs, locally, across all sixteen packages — resolve, analyse, test, then the example resolve and analyse. All sixteen pass on every step.
check_repo.pyreports clean after the two version fixes, and I confirmed R1 fires on #9's tree.One thing that did not survive the dry run: the analyze step was originally
dart analyze $targetswithtargets="lib test". That word-splits in bash and does not in zsh, so on a developer's machine it silently analyses a directory namedlib test, which does not exist, anddart analyzereports it as a usage error that reads nothing like the real problem. Split into one invocation per directory.Deliberately out of scope
Nothing here compiles tvOS sources, resolves a podspec, or touches a simulator. Stock Flutter is enough for resolution, analysis and the Dart tests, which is why this runs on
ubuntu-latestrather than a macOS runner — 16 jobs of a few minutes instead of a paid macOS fleet. The native half stays a reviewer's job, and the porting reports are where that evidence belongs.Package names are filtered to
[A-Za-z0-9_]+before entering the matrix: a directory name is attacker-controlled on a fork PR and these values reachworking-directory:.