From a935edcbe263cf36955a548ef72367059287b377 Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Fri, 14 Aug 2026 12:01:06 +0900 Subject: [PATCH 1/5] Add CI: per-package checks and repo structural gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/scripts/check_repo.py | 169 ++++++++++++++++++ .github/workflows/ci.yml | 128 +++++++++++++ .../example/lib/main.dart | 2 +- .../tvos/path_provider_tvos.podspec | 2 +- .../sqflite_tvos/tvos/sqflite_tvos.podspec | 2 +- 5 files changed, 300 insertions(+), 3 deletions(-) create mode 100755 .github/scripts/check_repo.py create mode 100644 .github/workflows/ci.yml diff --git a/.github/scripts/check_repo.py b/.github/scripts/check_repo.py new file mode 100755 index 0000000..b0d93f6 --- /dev/null +++ b/.github/scripts/check_repo.py @@ -0,0 +1,169 @@ +#!/usr/bin/env python3 +"""Structural checks over packages/ that need neither Flutter nor a network. + +These are the invariants nobody re-reads a checklist for. Each one has been +broken at least once, or was caught only because a human happened to look: + + R1 every package has a row in the root README's Ports table + Missed on the firebase_performance_tvos port (#9). AUTHORING.md makes it + step 4, and the package is otherwise undiscoverable from the repo index. + + R2 pubspec version == podspec s.version == the top CHANGELOG heading + path_provider_tvos and sqflite_tvos had already drifted. The podspec + version is inert while pods resolve by :path, so nothing surfaces it. + + R3 the files a published package cannot ship without + + R4 a committed pubspec_overrides.yaml is excluded from the published archive + A local path override that reaches pub.dev makes the package uninstallable. + + R5 the tvOS plugin class is declared + Without flutter.plugin.platforms.tvos.pluginClass the CLI never registers + the plugin and the app gets MissingPluginException at the first call — + at runtime, on a device, with nothing pointing at the cause. + +Usage: check_repo.py [repo-root] +Exits non-zero if any check fails. +""" + +import os +import re +import sys + +REQUIRED_FILES = ["pubspec.yaml", "README.md", "CHANGELOG.md", "LICENSE"] + + +def scalar(text, key): + """Top-level `key: value` from a pubspec, without a YAML dependency.""" + m = re.search(rf"^{re.escape(key)}:\s*(.+?)\s*$", text, re.M) + return m.group(1).strip().strip("\"'") if m else None + + +def tvos_plugin_class(text): + """flutter.plugin.platforms.tvos.pluginClass, by indentation.""" + m = re.search(r"^\s*tvos:\s*$", text, re.M) + if not m: + return None + rest = text[m.end():] + for line in rest.split("\n"): + if line.strip() and not line.startswith((" ", "\t")): + break # dedented out of the tvos: block + m2 = re.match(r"\s*pluginClass:\s*(\S+)", line) + if m2: + return m2.group(1) + return None + + +def main(): + root = os.path.abspath(sys.argv[1] if len(sys.argv) > 1 else ".") + pkg_root = os.path.join(root, "packages") + if not os.path.isdir(pkg_root): + print(f"ERROR: no packages/ under {root}") + return 1 + + try: + readme = open(os.path.join(root, "README.md"), encoding="utf-8").read() + except OSError as exc: + print(f"ERROR: cannot read the root README ({exc})") + return 1 + + packages = sorted( + d for d in os.listdir(pkg_root) + if os.path.isfile(os.path.join(pkg_root, d, "pubspec.yaml")) + ) + if not packages: + print("ERROR: packages/ holds no package with a pubspec.yaml") + return 1 + + failures = [] + + def fail(pkg, rule, msg, fix=None): + failures.append((pkg, rule, msg, fix)) + + for pkg in packages: + d = os.path.join(pkg_root, pkg) + + # R3 — required files + for name in REQUIRED_FILES: + if not os.path.isfile(os.path.join(d, name)): + fail(pkg, "R3", f"{name} is missing") + + pubspec_path = os.path.join(d, "pubspec.yaml") + pubspec = open(pubspec_path, encoding="utf-8").read() + + # R1 — README Ports row + if f"packages/{pkg})" not in readme: + fail(pkg, "R1", "no row in the root README's Ports table", + f"add a row linking packages/{pkg} — AUTHORING.md step 4") + + # R2 — three versions agree + version = scalar(pubspec, "version") + if not version: + fail(pkg, "R2", "pubspec.yaml declares no version") + else: + podspecs = [] + tvos_dir = os.path.join(d, "tvos") + if os.path.isdir(tvos_dir): + podspecs = [f for f in os.listdir(tvos_dir) if f.endswith(".podspec")] + if not podspecs: + fail(pkg, "R3", "tvos/ ships no .podspec") + for spec in podspecs: + text = open(os.path.join(tvos_dir, spec), encoding="utf-8").read() + m = re.search(r"s\.version\s*=\s*['\"]([^'\"]+)['\"]", text) + if not m: + fail(pkg, "R2", f"{spec} declares no s.version") + elif m.group(1) != version: + fail(pkg, "R2", + f"{spec} says {m.group(1)}, pubspec.yaml says {version}", + f"set s.version = '{version}'") + + changelog_path = os.path.join(d, "CHANGELOG.md") + if os.path.isfile(changelog_path): + cl = open(changelog_path, encoding="utf-8").read() + m = re.search(r"^##\s+\[?([0-9][^\]\s]*)\]?", cl, re.M) + if not m: + fail(pkg, "R2", "CHANGELOG.md has no `## ` heading") + elif m.group(1) != version: + fail(pkg, "R2", + f"CHANGELOG.md starts at {m.group(1)}, " + f"pubspec.yaml says {version}", + f"add a `## {version}` entry at the top") + + # R4 — a committed override must not reach the published archive + if os.path.isfile(os.path.join(d, "pubspec_overrides.yaml")): + pubignore = os.path.join(d, ".pubignore") + listed = ( + os.path.isfile(pubignore) + and "pubspec_overrides.yaml" in open(pubignore, encoding="utf-8").read() + ) + if not listed: + fail(pkg, "R4", + "pubspec_overrides.yaml is committed but not in .pubignore", + "add `pubspec_overrides.yaml` to .pubignore, or delete the " + "override if the dependency it pins is now published") + + # R5 — the tvOS plugin class + if not tvos_plugin_class(pubspec): + fail(pkg, "R5", + "pubspec.yaml declares no flutter.plugin.platforms.tvos.pluginClass", + "without it the CLI never registers the plugin — " + "MissingPluginException at the first call") + + print(f"Checked {len(packages)} package(s) under packages/\n") + if not failures: + print(" OK — README rows, versions, required files, " + "overrides and tvOS plugin classes all consistent.") + return 0 + + width = max(len(p) for p, _, _, _ in failures) + for pkg, rule, msg, fix in failures: + print(f" {rule} {pkg.ljust(width)} {msg}") + if fix: + print(f" → {fix}") + print(f"\n{len(failures)} problem(s). See the docstring in " + f".github/scripts/check_repo.py for why each rule exists.") + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..67b8d7d --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,128 @@ +name: CI + +# What this does and does not cover. +# +# Stock Flutter is enough for everything here — resolution, analysis and the +# Dart tests all work without the tvOS embedder, which is why this can run on +# ubuntu instead of a macOS runner. What it therefore does NOT cover is the half +# that actually needs Apple hardware: the podspecs are never resolved, the tvOS +# sources are never compiled, and nothing is run on a simulator. Those stay a +# reviewer's job, and the porting reports are where that evidence belongs. +# +# The point is the other half — the things a human is asked to remember and +# eventually won't. + +on: + pull_request: + push: + branches: [main] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +env: + # Pinned rather than "stable" so a Flutter release cannot turn a green PR red + # without a commit here saying so. Bump deliberately. + FLUTTER_VERSION: "3.44.0" + +jobs: + repo: + name: repo structure + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - run: python3 .github/scripts/check_repo.py . + + discover: + name: discover packages + runs-on: ubuntu-latest + outputs: + packages: ${{ steps.list.outputs.packages }} + steps: + - uses: actions/checkout@v4 + # Derived from the tree, not hardcoded: a new package is covered the + # moment it lands, which is the case a hardcoded list always misses. + # + # Names are filtered to [A-Za-z0-9_]+ before they become matrix values. + # A directory name is attacker-controlled on a fork PR, and these values + # reach `working-directory:` — the filter keeps anything shell-shaped from + # ever entering the matrix, rather than relying on where it lands. + - id: list + run: | + packages=$(ls -d packages/*/ \ + | sed 's|packages/||; s|/$||' \ + | grep -E '^[A-Za-z0-9_]+$' \ + | jq -R -s -c 'split("\n") | map(select(length > 0))') + if [ "$packages" = "[]" ] || [ -z "$packages" ]; then + echo "ERROR: no packages matched — refusing to report a vacuous pass." >&2 + exit 1 + fi + echo "packages=$packages" >> "$GITHUB_OUTPUT" + echo "Found: $packages" + + package: + name: ${{ matrix.package }} + needs: discover + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + package: ${{ fromJSON(needs.discover.outputs.packages) }} + defaults: + run: + working-directory: packages/${{ matrix.package }} + steps: + - uses: actions/checkout@v4 + + - uses: subosito/flutter-action@v2 + with: + flutter-version: ${{ env.FLUTTER_VERSION }} + channel: stable + cache: true + + - name: flutter pub get + run: flutter pub get + + # Scoped to the package's own Dart. Analysing the package root would also + # descend into example/ and into nested packages that were never resolved + # (audioplayers_tvos ships an example/server with its own pubspec), and + # report 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. + # One invocation per directory rather than `dart analyze $targets`: an + # unquoted variable holding two paths word-splits in bash and does not in + # zsh, so the collapsed form silently analyses a directory named "lib test" + # — which does not exist, and which `dart analyze` reports as a usage + # error that reads nothing like the real problem. + - name: dart analyze (lib + test) + run: | + dart analyze lib + if [ -d test ]; then dart analyze test; fi + + - name: flutter test + run: | + if [ -d test ] && ls test/*.dart >/dev/null 2>&1; then + flutter test + else + echo "no test/ — skipping" + fi + + # The example is what a user copies, so it has to at least resolve and + # analyse. Errors fail; infos do not. + - name: example — resolve and analyse + run: | + if [ ! -f example/pubspec.yaml ]; then + echo "no example/ — skipping" + exit 0 + fi + cd example + flutter pub get + if [ -d lib ]; then + dart analyze lib + fi diff --git a/packages/device_info_plus_tvos/example/lib/main.dart b/packages/device_info_plus_tvos/example/lib/main.dart index 1c23698..893d164 100644 --- a/packages/device_info_plus_tvos/example/lib/main.dart +++ b/packages/device_info_plus_tvos/example/lib/main.dart @@ -17,7 +17,7 @@ void main() { () { runApp(const MyApp()); }, - (dynamic error, dynamic stack) { + (Object error, StackTrace stack) { developer.log("Something went wrong!", error: error, stackTrace: stack); }, ); diff --git a/packages/path_provider_tvos/tvos/path_provider_tvos.podspec b/packages/path_provider_tvos/tvos/path_provider_tvos.podspec index 28c91ce..7665a58 100644 --- a/packages/path_provider_tvos/tvos/path_provider_tvos.podspec +++ b/packages/path_provider_tvos/tvos/path_provider_tvos.podspec @@ -6,7 +6,7 @@ # Pod::Spec.new do |s| s.name = 'path_provider_tvos' - s.version = '0.0.2' + s.version = '0.0.3' s.summary = 'tvOS implementation of path_provider.' s.description = <<-DESC tvOS implementation of path_provider, the federated platform diff --git a/packages/sqflite_tvos/tvos/sqflite_tvos.podspec b/packages/sqflite_tvos/tvos/sqflite_tvos.podspec index 251cbf1..1e83989 100644 --- a/packages/sqflite_tvos/tvos/sqflite_tvos.podspec +++ b/packages/sqflite_tvos/tvos/sqflite_tvos.podspec @@ -6,7 +6,7 @@ # Pod::Spec.new do |s| s.name = 'sqflite_tvos' - s.version = '0.0.1' + s.version = '0.0.2' s.summary = 'tvOS implementation of sqflite.' s.description = <<-DESC tvOS implementation of sqflite, the federated platform From 306e780d753aa213b85c4d5bd46c48136ca443c0 Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Fri, 14 Aug 2026 18:37:57 +0900 Subject: [PATCH 2/5] Fix the gate: four rules were silently passing trees they reject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/scripts/check_repo.py | 479 ++++++++++++++++++++++++++-------- .github/workflows/ci.yml | 158 +++++++---- 2 files changed, 483 insertions(+), 154 deletions(-) diff --git a/.github/scripts/check_repo.py b/.github/scripts/check_repo.py index b0d93f6..8b0bbd0 100755 --- a/.github/scripts/check_repo.py +++ b/.github/scripts/check_repo.py @@ -4,166 +4,435 @@ These are the invariants nobody re-reads a checklist for. Each one has been broken at least once, or was caught only because a human happened to look: - R1 every package has a row in the root README's Ports table - Missed on the firebase_performance_tvos port (#9). AUTHORING.md makes it - step 4, and the package is otherwise undiscoverable from the repo index. + R1 every package has a row in the root README's plugin table + Missed on the firebase_performance_tvos port. AUTHORING.md makes it a + step of its own, and without the row the package is undiscoverable from + the repo index. - R2 pubspec version == podspec s.version == the top CHANGELOG heading + R2 pubspec version == podspec s.version == the newest CHANGELOG heading path_provider_tvos and sqflite_tvos had already drifted. The podspec version is inert while pods resolve by :path, so nothing surfaces it. - R3 the files a published package cannot ship without + R3 the files a package cannot ship without - R4 a committed pubspec_overrides.yaml is excluded from the published archive - A local path override that reaches pub.dev makes the package uninstallable. + R4 a committed pubspec_overrides.yaml does not reach the published archive + `dart pub publish` drops the package's own root override on its own, but + NOT a nested one: example/pubspec_overrides.yaml ships unless .pubignore + excludes it. That is cosmetic rather than fatal — a consumer resolving a + hosted dependency ignores that dependency's overrides — but the archive + should describe what consumers actually get. - R5 the tvOS plugin class is declared - Without flutter.plugin.platforms.tvos.pluginClass the CLI never registers - the plugin and the app gets MissingPluginException at the first call — - at runtime, on a device, with nothing pointing at the cause. + R5 the tvOS platform block declares a pluginClass + AUTHORING.md: the CLI discovers the plugin through + flutter.plugin.platforms.tvos. Miss it and the plugin silently does not + register, giving MissingPluginException at the first call, at runtime, on + a device, with nothing pointing at the cause. Every package here is + native-backed, so pluginClass is the field that has to be present. + +Every rule reports the package it failed on and how to fix it, and a run +reports what it *skipped* as loudly as what it rejected: a check that quietly +declines to run is the failure mode this file exists to prevent. + +Usage: + check_repo.py [repo-root] run the checks + check_repo.py --list [root] print package names, one per line + check_repo.py --selftest verify every rule fires, and does not -Usage: check_repo.py [repo-root] Exits non-zero if any check fails. """ import os import re +import shutil import sys +import tempfile + +import yaml REQUIRED_FILES = ["pubspec.yaml", "README.md", "CHANGELOG.md", "LICENSE"] -def scalar(text, key): - """Top-level `key: value` from a pubspec, without a YAML dependency.""" - m = re.search(rf"^{re.escape(key)}:\s*(.+?)\s*$", text, re.M) - return m.group(1).strip().strip("\"'") if m else None +def load_yaml(path): + """Parsed YAML, or a (None, reason) pair. Never raises.""" + try: + with open(path, encoding="utf-8") as handle: + return yaml.safe_load(handle), None + except (OSError, UnicodeDecodeError, yaml.YAMLError) as exc: + return None, str(exc).split("\n")[0] + + +def read_text(path): + """File contents, or a (None, reason) pair. Never raises.""" + try: + with open(path, encoding="utf-8") as handle: + return handle.read(), None + except (OSError, UnicodeDecodeError) as exc: + return None, str(exc).split("\n")[0] + +def tvos_plugin_class(pubspec): + """flutter.plugin.platforms.tvos.pluginClass, or None. -def tvos_plugin_class(text): - """flutter.plugin.platforms.tvos.pluginClass, by indentation.""" - m = re.search(r"^\s*tvos:\s*$", text, re.M) - if not m: + Parsed rather than scanned. The hand-rolled version walked out of the + `tvos:` block into whichever platform followed it and returned *that* + platform's class, so a package declaring only `ios: pluginClass:` passed + the one rule whose failure is invisible until an app calls the plugin. + """ + if not isinstance(pubspec, dict): return None - rest = text[m.end():] - for line in rest.split("\n"): - if line.strip() and not line.startswith((" ", "\t")): - break # dedented out of the tvos: block - m2 = re.match(r"\s*pluginClass:\s*(\S+)", line) - if m2: - return m2.group(1) - return None + node = pubspec.get("flutter") + for key in ("plugin", "platforms", "tvos"): + if not isinstance(node, dict): + return None + node = node.get(key) + if not isinstance(node, dict): + return None + value = node.get("pluginClass") + return value if isinstance(value, str) and value.strip() else None -def main(): - root = os.path.abspath(sys.argv[1] if len(sys.argv) > 1 else ".") - pkg_root = os.path.join(root, "packages") - if not os.path.isdir(pkg_root): - print(f"ERROR: no packages/ under {root}") - return 1 +def readme_table_rows(readme): + """The set of `packages/` links inside the README's plugin table. - try: - readme = open(os.path.join(root, "README.md"), encoding="utf-8").read() - except OSError as exc: - print(f"ERROR: cannot read the root README ({exc})") - return 1 + Scoped to table rows so a passing mention in prose cannot satisfy R1 — the + rule is about the index a reader browses, not about the string appearing + somewhere in the file. + """ + rows = set() + for line in readme.split("\n"): + if not line.lstrip().startswith("|"): + continue + rows.update(re.findall(r"\(packages/([A-Za-z0-9_]+)/?\)", line)) + return rows - packages = sorted( + +def pubignore_excludes(text, name): + """True if `name` is excluded by this .pubignore. + + Line-wise, ignoring comments and rejecting negations: a substring test + passes on `# pubspec_overrides.yaml` and, worse, on `!pubspec_overrides`, + which forces the file *in*. + """ + for raw in text.split("\n"): + line = raw.strip() + if not line or line.startswith("#"): + continue + if line.startswith("!"): + continue + if line.rstrip("/").endswith(name): + return True + return False + + +def newest_changelog_version(text): + """The first `## ` heading's version, or (None, heading). + + The first heading of any kind, not the first numeric one: matching on a + leading digit skipped `## Unreleased` and validated the released entry + underneath it, so a changelog whose top section had no version passed. + """ + match = re.search(r"^##\s+(.+?)\s*$", text, re.M) + if not match: + return None, None + heading = match.group(1).strip() + version = re.match(r"\[?([0-9][^\]\s]*)\]?", heading) + return (version.group(1) if version else None), heading + + +def podspec_version(text): + """`s.version` from a podspec, ignoring commented-out lines.""" + match = re.search(r"^\s*s\.version\s*=\s*['\"]([^'\"]+)['\"]", text, re.M) + return match.group(1) if match else None + + +def discover(pkg_root): + """Every directory under packages/. Not "every directory with a pubspec" — + that made a package with a misnamed pubspec vanish from the run instead of + failing it, and silently disagreed with the workflow's own matrix.""" + if not os.path.isdir(pkg_root): + return [] + return sorted( d for d in os.listdir(pkg_root) - if os.path.isfile(os.path.join(pkg_root, d, "pubspec.yaml")) + if os.path.isdir(os.path.join(pkg_root, d)) and not d.startswith(".") ) - if not packages: - print("ERROR: packages/ holds no package with a pubspec.yaml") - return 1 + +def check(root): + """Returns (failures, packages). Each failure is (rule, package, message, fix).""" + pkg_root = os.path.join(root, "packages") + packages = discover(pkg_root) failures = [] - def fail(pkg, rule, msg, fix=None): - failures.append((pkg, rule, msg, fix)) + def fail(rule, pkg, msg, fix=None): + failures.append((rule, pkg, msg, fix)) + + if not packages: + fail("R0", "-", f"no package directories under {pkg_root}", + "an empty run would report success having checked nothing") + return failures, packages + + readme, err = read_text(os.path.join(root, "README.md")) + table = readme_table_rows(readme) if readme is not None else None + if table is None: + fail("R0", "-", f"cannot read the root README.md ({err})", + "R1 cannot run without it, so this is fatal rather than skipped") for pkg in packages: d = os.path.join(pkg_root, pkg) - # R3 — required files + # R3 — required files. for name in REQUIRED_FILES: if not os.path.isfile(os.path.join(d, name)): - fail(pkg, "R3", f"{name} is missing") + fail("R3", pkg, f"{name} is missing") - pubspec_path = os.path.join(d, "pubspec.yaml") - pubspec = open(pubspec_path, encoding="utf-8").read() + # R1 — a row in the README's plugin table. + if table is not None and pkg not in table: + fail("R1", pkg, "no row in the root README's plugin table", + f"add a row linking packages/{pkg} under '## List of plugins'") - # R1 — README Ports row - if f"packages/{pkg})" not in readme: - fail(pkg, "R1", "no row in the root README's Ports table", - f"add a row linking packages/{pkg} — AUTHORING.md step 4") + pubspec_path = os.path.join(d, "pubspec.yaml") + pubspec, err = load_yaml(pubspec_path) + if pubspec is None: + fail("R3", pkg, f"pubspec.yaml is unreadable or invalid ({err})") + continue # nothing below can be judged without it + if not isinstance(pubspec, dict): + fail("R3", pkg, "pubspec.yaml is not a YAML mapping") + continue - # R2 — three versions agree - version = scalar(pubspec, "version") + version = pubspec.get("version") + version = str(version).strip() if version is not None else None if not version: - fail(pkg, "R2", "pubspec.yaml declares no version") + fail("R2", pkg, "pubspec.yaml declares no version") + + # R2 — podspec. Presence is checked whether or not the version parsed; + # nesting it under the version check hid a missing podspec behind an + # unrelated failure and cost a round-trip through CI to discover. + tvos_dir = os.path.join(d, "tvos") + podspecs = ( + sorted(f for f in os.listdir(tvos_dir) if f.endswith(".podspec")) + if os.path.isdir(tvos_dir) else [] + ) + if not podspecs: + fail("R3", pkg, "tvos/ ships no .podspec") + for spec in podspecs: + text, err = read_text(os.path.join(tvos_dir, spec)) + if text is None: + fail("R3", pkg, f"{spec} is unreadable ({err})") + continue + declared = podspec_version(text) + if declared is None: + fail("R2", pkg, f"{spec} declares no s.version") + elif version and declared != version: + fail("R2", pkg, f"{spec} says {declared}, pubspec.yaml says {version}", + f"set s.version = '{version}'") + + # R2 — changelog. + changelog, err = read_text(os.path.join(d, "CHANGELOG.md")) + if changelog is None: + if os.path.exists(os.path.join(d, "CHANGELOG.md")): + fail("R3", pkg, f"CHANGELOG.md is unreadable ({err})") else: - podspecs = [] - tvos_dir = os.path.join(d, "tvos") - if os.path.isdir(tvos_dir): - podspecs = [f for f in os.listdir(tvos_dir) if f.endswith(".podspec")] - if not podspecs: - fail(pkg, "R3", "tvos/ ships no .podspec") - for spec in podspecs: - text = open(os.path.join(tvos_dir, spec), encoding="utf-8").read() - m = re.search(r"s\.version\s*=\s*['\"]([^'\"]+)['\"]", text) - if not m: - fail(pkg, "R2", f"{spec} declares no s.version") - elif m.group(1) != version: - fail(pkg, "R2", - f"{spec} says {m.group(1)}, pubspec.yaml says {version}", - f"set s.version = '{version}'") - - changelog_path = os.path.join(d, "CHANGELOG.md") - if os.path.isfile(changelog_path): - cl = open(changelog_path, encoding="utf-8").read() - m = re.search(r"^##\s+\[?([0-9][^\]\s]*)\]?", cl, re.M) - if not m: - fail(pkg, "R2", "CHANGELOG.md has no `## ` heading") - elif m.group(1) != version: - fail(pkg, "R2", - f"CHANGELOG.md starts at {m.group(1)}, " - f"pubspec.yaml says {version}", - f"add a `## {version}` entry at the top") - - # R4 — a committed override must not reach the published archive - if os.path.isfile(os.path.join(d, "pubspec_overrides.yaml")): - pubignore = os.path.join(d, ".pubignore") - listed = ( - os.path.isfile(pubignore) - and "pubspec_overrides.yaml" in open(pubignore, encoding="utf-8").read() - ) - if not listed: - fail(pkg, "R4", - "pubspec_overrides.yaml is committed but not in .pubignore", + newest, heading = newest_changelog_version(changelog) + if heading is None: + fail("R2", pkg, "CHANGELOG.md has no `## ` heading") + elif newest is None: + fail("R2", pkg, f"CHANGELOG.md's newest heading is '{heading}', not a version", + f"the top section should be `## {version or ''}`") + elif version and newest != version: + fail("R2", pkg, f"CHANGELOG.md starts at {newest}, pubspec.yaml says {version}", + f"add a `## {version}` entry at the top") + + # R4 — a nested override must not reach the archive. + nested = os.path.join(d, "example", "pubspec_overrides.yaml") + if os.path.isfile(nested): + pubignore, _ = read_text(os.path.join(d, ".pubignore")) + if pubignore is None or not pubignore_excludes(pubignore, "pubspec_overrides.yaml"): + fail("R4", pkg, + "example/pubspec_overrides.yaml is committed but not excluded by .pubignore", "add `pubspec_overrides.yaml` to .pubignore, or delete the " "override if the dependency it pins is now published") - # R5 — the tvOS plugin class + # R5 — the tvOS plugin class. if not tvos_plugin_class(pubspec): - fail(pkg, "R5", - "pubspec.yaml declares no flutter.plugin.platforms.tvos.pluginClass", + fail("R5", pkg, + "declares no flutter.plugin.platforms.tvos.pluginClass", "without it the CLI never registers the plugin — " "MissingPluginException at the first call") + return failures, packages + + +def report(root): + failures, packages = check(root) print(f"Checked {len(packages)} package(s) under packages/\n") if not failures: - print(" OK — README rows, versions, required files, " - "overrides and tvOS plugin classes all consistent.") + print(" OK — README rows, versions, required files, overrides and " + "tvOS plugin classes all consistent.") return 0 - - width = max(len(p) for p, _, _, _ in failures) - for pkg, rule, msg, fix in failures: + width = max(len(p) for _, p, _, _ in failures) + for rule, pkg, msg, fix in failures: print(f" {rule} {pkg.ljust(width)} {msg}") if fix: print(f" → {fix}") - print(f"\n{len(failures)} problem(s). See the docstring in " - f".github/scripts/check_repo.py for why each rule exists.") + print(f"\n{len(failures)} problem(s). Each rule's rationale is in the " + f"docstring at the top of .github/scripts/check_repo.py.") return 1 +# --- Self-test ------------------------------------------------------------- +# +# The rules are only half the file; the other half is the failure paths, and on +# a green tree none of them execute. Measured with `trace --count --missing`, +# an earlier revision ran 67 of 105 lines on a passing run — every fail() site +# was cold, including the one inside fail() itself. Four of five rules were +# silently passing trees they were written to reject, and nothing could have +# told us. +# +# So each case asserts in BOTH directions. Asserting only "the rule fires on a +# bad tree" would have caught none of those four: every one was a silent pass +# on a tree that superficially looked compliant. + +GOOD_PUBSPEC = """\ +name: {name} +version: 0.0.1 +flutter: + plugin: + platforms: + tvos: + pluginClass: GoodPlugin +""" + + +def _fixture(root, name="widget_tvos", **overrides): + """A minimal package that passes every rule, then selectively broken.""" + d = os.path.join(root, "packages", name) + os.makedirs(os.path.join(d, "tvos")) + files = { + "pubspec.yaml": GOOD_PUBSPEC.format(name=name), + "README.md": "# widget_tvos\n", + "CHANGELOG.md": "## 0.0.1\n\n* Initial.\n", + "LICENSE": "BSD-3-Clause\n", + f"tvos/{name}.podspec": "Pod::Spec.new do |s|\n s.version = '0.0.1'\nend\n", + } + # `//README` addresses the *root* README, not a file inside the package — + # keep it out of the per-package writes below. + readme_override = overrides.pop("//README", None) + files.update(overrides) + for rel, content in files.items(): + if content is None: + continue + path = os.path.join(d, rel) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w", encoding="utf-8") as handle: + handle.write(content) + readme = readme_override or ( + f"# Plugins\n\n## List of plugins\n\n| Plugin | Upstream |\n" + f"|---|---|\n| [`{name}`](packages/{name}) | upstream |\n" + ) + with open(os.path.join(root, "README.md"), "w", encoding="utf-8") as handle: + handle.write(readme) + return d + + +CASES = [ + ("baseline passes", {}, None), + ("R1 prose mention is not a table row", + {"//README": "# Plugins\n\nSee [the notes](packages/widget_tvos) if curious.\n"}, "R1"), + ("R2 podspec drift", + {"tvos/widget_tvos.podspec": "Pod::Spec.new do |s|\n s.version = '0.0.2'\nend\n"}, "R2"), + ("R2 commented-out s.version does not count", + {"tvos/widget_tvos.podspec": + "Pod::Spec.new do |s|\n # s.version = '0.0.1'\n s.version = '9.9.9'\nend\n"}, "R2"), + ("R2 non-version changelog heading on top", + {"CHANGELOG.md": "## Unreleased\n\n* wip\n\n## 0.0.1\n\n* Initial.\n"}, "R2"), + ("R2 version with build metadata is compared verbatim", + {"pubspec.yaml": GOOD_PUBSPEC.format(name="widget_tvos").replace( + "version: 0.0.1", "version: 0.0.1+1")}, "R2"), + ("R3 missing podspec is reported even without a version", + {"pubspec.yaml": "name: widget_tvos\nflutter:\n plugin:\n platforms:\n" + " tvos:\n pluginClass: GoodPlugin\n", + "tvos/widget_tvos.podspec": None}, "R3"), + ("R3 unparseable pubspec fails rather than vanishing", + {"pubspec.yaml": "name: [unclosed\n"}, "R3"), + ("R5 sibling platform's pluginClass does not satisfy tvos", + {"pubspec.yaml": "name: widget_tvos\nversion: 0.0.1\nflutter:\n plugin:\n" + " platforms:\n tvos:\n sharedDarwinSource: true\n" + " ios:\n pluginClass: RealIosPlugin\n"}, "R5"), +] + + +def selftest(): + failures = 0 + for label, overrides, expect in CASES: + root = tempfile.mkdtemp(prefix="check_repo_selftest.") + try: + podspec_removed = overrides.get("tvos/widget_tvos.podspec", "keep") is None + _fixture(root, **{k: v for k, v in overrides.items() if v is not None}) + if podspec_removed: + os.remove(os.path.join(root, "packages", "widget_tvos", "tvos", + "widget_tvos.podspec")) + found, _ = check(root) + rules = {rule for rule, _, _, _ in found} + if expect is None: + ok = not found + detail = "expected a clean run, got: " + "; ".join( + f"{r} {m}" for r, _, m, _ in found) + else: + ok = expect in rules + detail = f"expected {expect}, got {sorted(rules) or 'nothing'}" + print(f" {'ok ' if ok else 'FAIL'} {label}") + if not ok: + print(f" {detail}") + failures += 1 + finally: + shutil.rmtree(root, ignore_errors=True) + + # Extra R4 cases: the shapes a substring test accepts while the override + # still ships. Built directly, since they need an example/ subtree. + for label, pubignore, expect_fail in [ + ("R4 real entry passes", "pubspec_overrides.yaml\n", False), + ("R4 comment does not count", "# pubspec_overrides.yaml\n", True), + ("R4 negation does not count", "!pubspec_overrides.yaml\n", True), + ("R4 missing .pubignore", None, True), + ]: + root = tempfile.mkdtemp(prefix="check_repo_selftest.") + try: + d = _fixture(root) + os.makedirs(os.path.join(d, "example")) + open(os.path.join(d, "example", "pubspec_overrides.yaml"), "w").close() + if pubignore is not None: + with open(os.path.join(d, ".pubignore"), "w", encoding="utf-8") as handle: + handle.write(pubignore) + found, _ = check(root) + fired = "R4" in {rule for rule, _, _, _ in found} + ok = fired == expect_fail + print(f" {'ok ' if ok else 'FAIL'} {label}") + if not ok: + print(f" expected R4 to {'fire' if expect_fail else 'stay quiet'}") + failures += 1 + finally: + shutil.rmtree(root, ignore_errors=True) + + print() + if failures: + print(f"{failures} self-test case(s) failed — the gate itself is broken.") + return 1 + print("Self-test passed: every rule fires on a bad tree and stays quiet on a good one.") + return 0 + + +def main(): + args = sys.argv[1:] + if "--selftest" in args: + return selftest() + positional = [a for a in args if not a.startswith("--")] + root = os.path.abspath(positional[0] if positional else ".") + if "--list" in args: + for pkg in discover(os.path.join(root, "packages")): + print(pkg) + return 0 + return report(root) + + if __name__ == "__main__": sys.exit(main()) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 67b8d7d..6fb85f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,14 +3,18 @@ name: CI # What this does and does not cover. # # Stock Flutter is enough for everything here — resolution, analysis and the -# Dart tests all work without the tvOS embedder, which is why this can run on +# Dart tests all work without the tvOS embedder, which is why this runs on # ubuntu instead of a macOS runner. What it therefore does NOT cover is the half -# that actually needs Apple hardware: the podspecs are never resolved, the tvOS -# sources are never compiled, and nothing is run on a simulator. Those stay a -# reviewer's job, and the porting reports are where that evidence belongs. +# that needs Apple hardware: podspecs are never resolved, tvOS sources are never +# compiled, nothing runs on a simulator. Those stay a reviewer's job, and the +# porting reports are where that evidence belongs. # -# The point is the other half — the things a human is asked to remember and -# eventually won't. +# Worth knowing why the `repo` job exists at all: stock flutter_tools does not +# validate the `tvos:` platform key. `_validateMultiPlatformYaml` knows only +# android/ios/linux/macos/windows and ignores anything else, so `flutter pub +# get` will happily accept a tvOS block with no pluginClass. Nothing in the +# Flutter half of this workflow can catch that; check_repo.py's R5 is the only +# thing standing between it and a MissingPluginException on a device. on: pull_request: @@ -21,8 +25,12 @@ permissions: contents: read concurrency: + # Per-ref, and only cancellable on pull requests: cancelling a superseded PR + # run is what you want, but cancelling main's runs when two merges land close + # together leaves main's history dotted with cancelled builds and no record of + # whether the tree was ever green. group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + cancel-in-progress: ${{ github.event_name == 'pull_request' }} env: # Pinned rather than "stable" so a Flutter release cannot turn a green PR red @@ -33,45 +41,60 @@ jobs: repo: name: repo structure runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - run: python3 .github/scripts/check_repo.py . - - discover: - name: discover packages - runs-on: ubuntu-latest outputs: packages: ${{ steps.list.outputs.packages }} steps: - uses: actions/checkout@v4 - # Derived from the tree, not hardcoded: a new package is covered the - # moment it lands, which is the case a hardcoded list always misses. - # - # Names are filtered to [A-Za-z0-9_]+ before they become matrix values. - # A directory name is attacker-controlled on a fork PR, and these values - # reach `working-directory:` — the filter keeps anything shell-shaped from - # ever entering the matrix, rather than relying on where it lands. + + # Preinstalled on ubuntu-latest, but asserted rather than assumed: an + # ImportError here would fail the job with a traceback instead of a + # sentence, and this job is the only gate on several invariants. + - name: Ensure PyYAML + run: python3 -c "import yaml" 2>/dev/null || pip install --quiet pyyaml + + # Before the real run: the rules are only half the file, and on a passing + # tree none of the failure paths execute. An earlier revision had four of + # five rules silently accepting trees they were written to reject, and + # nothing could have told us. Each case asserts in both directions. + - name: Self-test the checker + run: python3 .github/scripts/check_repo.py --selftest + + - name: Structural invariants + run: python3 .github/scripts/check_repo.py . + + # One definition of "a package", shared with the matrix below. When the + # workflow derived its own list, the two could disagree silently — a + # directory the checker skipped still got a build job, and vice versa. - id: list run: | - packages=$(ls -d packages/*/ \ - | sed 's|packages/||; s|/$||' \ - | grep -E '^[A-Za-z0-9_]+$' \ - | jq -R -s -c 'split("\n") | map(select(length > 0))') - if [ "$packages" = "[]" ] || [ -z "$packages" ]; then - echo "ERROR: no packages matched — refusing to report a vacuous pass." >&2 + set -euo pipefail + names=$(python3 .github/scripts/check_repo.py --list .) + count=$(printf '%s\n' "$names" | grep -c . || true) + dirs=$(ls -d packages/*/ | wc -l | tr -d ' ') + if [ "$count" -eq 0 ]; then + echo "::error::No packages discovered — refusing to report a vacuous pass." + exit 1 + fi + # A silent drop is the failure this whole job exists to prevent, so + # compare against the raw directory count rather than trusting the + # filter to have kept everything. + if [ "$count" -ne "$dirs" ]; then + echo "::error::Discovered $count package(s) but packages/ holds $dirs directories." + echo "Something under packages/ is not being checked. Names seen:" + printf '%s\n' "$names" exit 1 fi - echo "packages=$packages" >> "$GITHUB_OUTPUT" - echo "Found: $packages" + echo "packages=$(printf '%s\n' "$names" | jq -R -s -c 'split("\n") | map(select(length > 0))')" >> "$GITHUB_OUTPUT" + echo "Discovered $count package(s)." package: name: ${{ matrix.package }} - needs: discover + needs: repo runs-on: ubuntu-latest strategy: fail-fast: false matrix: - package: ${{ fromJSON(needs.discover.outputs.packages) }} + package: ${{ fromJSON(needs.repo.outputs.packages) }} defaults: run: working-directory: packages/${{ matrix.package }} @@ -89,40 +112,77 @@ jobs: # Scoped to the package's own Dart. Analysing the package root would also # descend into example/ and into nested packages that were never resolved - # (audioplayers_tvos ships an example/server with its own pubspec), and + # (audioplayers_tvos ships an example/server with its own pubspec) and # report 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. - # One invocation per directory rather than `dart analyze $targets`: an - # unquoted variable holding two paths word-splits in bash and does not in - # zsh, so the collapsed form silently analyses a directory named "lib test" - # — which does not exist, and which `dart analyze` reports as a usage - # error that reads nothing like the real problem. + # One invocation per directory rather than `dart analyze $targets`. Under + # Actions the shell is bash, where the unquoted expansion does split, so + # the collapsed form works here — but it silently breaks when pasted into + # a macOS zsh prompt, which does not word-split: `dart analyze` then gets + # the single argument "lib test" and exits 64. Two invocations behave the + # same in both shells. + # + # `dart analyze` is fatal on errors AND warnings, and reports infos + # without failing. - name: dart analyze (lib + test) run: | dart analyze lib if [ -d test ]; then dart analyze test; fi + # `flutter test` discovers test/**/*_test.dart recursively. An earlier + # version gated this on `ls test/*.dart`, which only sees the top level: + # move the tests into test/unit/ — an ordinary tidy-up — and every package + # silently stopped running them while the log claimed there was no test + # directory at all. - name: flutter test run: | - if [ -d test ] && ls test/*.dart >/dev/null 2>&1; then - flutter test - else - echo "no test/ — skipping" + if [ ! -d test ]; then + echo "::warning::no test/ directory — nothing ran" + exit 0 + fi + if ! find test -name '*_test.dart' -print -quit | grep -q .; then + echo "::warning::test/ exists but holds no *_test.dart — nothing ran" + exit 0 fi + flutter test - # The example is what a user copies, so it has to at least resolve and - # analyse. Errors fail; infos do not. + # The example is what a user copies, so it has to resolve and analyse. + # + # Note these do not all get the same treatment: eight examples ship no + # analysis_options.yaml of their own and therefore inherit the plugin's + # strict-casts / strict-inference / strict-raw-types, which produce + # errors. That is not hypothetical — the commit adding this workflow had + # to fix one such error in device_info_plus_tvos's example. The eight that + # do carry their own options file are checked more loosely. Worth + # unifying, but not by silently lowering the bar here. - name: example — resolve and analyse run: | if [ ! -f example/pubspec.yaml ]; then - echo "no example/ — skipping" + echo "::warning::no example/ — nothing resolved or analysed" exit 0 fi cd example flutter pub get - if [ -d lib ]; then - dart analyze lib + if [ ! -d lib ]; then + echo "::error::example/ has no lib/ — a Flutter example without one is broken" + exit 1 fi + dart analyze lib + + # A single stable name for branch protection. The matrix legs are named after + # the packages they build, so their check names come and go with the tree: + # adding a package creates a check that is not in the protection list, and its + # failure would not block the merge — exactly the case a derived matrix exists + # to cover. `if: always()` plus explicit result assertions is required, since + # a skipped job otherwise counts as success. + ci: + name: ci + if: always() + needs: [repo, package] + runs-on: ubuntu-latest + steps: + - name: Assert every job succeeded + run: | + echo 'repo=${{ needs.repo.result }} package=${{ needs.package.result }}' + [ '${{ needs.repo.result }}' = 'success' ] || exit 1 + [ '${{ needs.package.result }}' = 'success' ] || exit 1 From 2fe9e15a2a4a4b3b2c37df91dc3b4dfb9fbe2c62 Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Fri, 14 Aug 2026 19:04:05 +0900 Subject: [PATCH 3/5] Close the second-round findings, including a vacuously-passing self-test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/scripts/check_repo.py | 255 +++++++++++++++++++++++++++------- .github/workflows/ci.yml | 24 +++- 2 files changed, 228 insertions(+), 51 deletions(-) diff --git a/.github/scripts/check_repo.py b/.github/scripts/check_repo.py index 8b0bbd0..29350af 100755 --- a/.github/scripts/check_repo.py +++ b/.github/scripts/check_repo.py @@ -41,6 +41,8 @@ Exits non-zero if any check fails. """ +import argparse +import fnmatch import os import re import shutil @@ -53,10 +55,16 @@ def load_yaml(path): - """Parsed YAML, or a (None, reason) pair. Never raises.""" + """(parsed, error). `error` is None when the file parsed, even to nothing. + + BaseLoader rather than safe_load, so every scalar stays a string. safe_load + types `version: 1.10` as the float 1.1, and this checker compares versions + as text — it would have reported "podspec says 1.10, pubspec.yaml says 1.1" + about a tree where all three files literally agree. + """ try: with open(path, encoding="utf-8") as handle: - return yaml.safe_load(handle), None + return yaml.load(handle, Loader=yaml.BaseLoader), None except (OSError, UnicodeDecodeError, yaml.YAMLError) as exc: return None, str(exc).split("\n")[0] @@ -102,26 +110,53 @@ def readme_table_rows(readme): for line in readme.split("\n"): if not line.lstrip().startswith("|"): continue - rows.update(re.findall(r"\(packages/([A-Za-z0-9_]+)/?\)", line)) + # Tolerate `./packages/x`, a trailing slash, an `#anchor` and a link + # title — all legitimate rows that the stricter form rejected. + rows.update(re.findall(r"\(\.?/?packages/([A-Za-z0-9_]+)[/#)\s]", line)) return rows -def pubignore_excludes(text, name): - """True if `name` is excluded by this .pubignore. +def pubignore_excludes(text, relpath): + """True if `relpath` (relative to the package root) is excluded. + + Gitignore semantics, and the parts that matter here are the ones that are + easy to get wrong: - Line-wise, ignoring comments and rejecting negations: a substring test - passes on `# pubspec_overrides.yaml` and, worse, on `!pubspec_overrides`, - which forces the file *in*. + * **Last match wins.** A `!pattern` after a matching pattern re-includes the + file. Skipping `!` lines instead — which an earlier version did — means a + negation can never take effect, and `foo` followed by `!foo` reads as + excluded when the file actually ships. + * **Match the path, not the name.** `other_dir/pubspec_overrides.yaml` + excludes a file in `other_dir/`, and says nothing about the one in + `example/`. Comparing basenames accepted it anyway — the same shape of + sloppiness as the substring test this rule started with. + * A pattern with no slash matches at any depth; one with a slash is + anchored to the package root. """ + excluded = False for raw in text.split("\n"): line = raw.strip() if not line or line.startswith("#"): continue - if line.startswith("!"): + negated = line.startswith("!") + if negated: + line = line[1:].strip() + pattern = line.rstrip("/") + if not pattern: continue - if line.rstrip("/").endswith(name): - return True - return False + if pattern.startswith("/"): + # Anchored to the package root: `/pubspec_overrides.yaml` excludes + # the root file and says nothing about example/'s. + hit = fnmatch.fnmatch(relpath, pattern.lstrip("/")) + elif "/" in pattern: + hit = fnmatch.fnmatch(relpath, pattern) + else: + # Unanchored: matches the basename, or any directory component. + parts = relpath.split("/") + hit = any(fnmatch.fnmatch(part, pattern) for part in parts) + if hit: + excluded = not negated + return excluded def newest_changelog_version(text): @@ -131,7 +166,11 @@ def newest_changelog_version(text): leading digit skipped `## Unreleased` and validated the released entry underneath it, so a changelog whose top section had no version passed. """ - match = re.search(r"^##\s+(.+?)\s*$", text, re.M) + # Strip fenced blocks first: a ``` example containing `## 0.0.1` above the + # real `## Unreleased` heading would otherwise be read as the newest entry, + # which is the original defect through a different door. + stripped = re.sub(r"^```.*?^```", "", text, flags=re.M | re.S) + match = re.search(r"^##\s+(.+?)\s*$", stripped, re.M) if not match: return None, None heading = match.group(1).strip() @@ -190,19 +229,32 @@ def fail(rule, pkg, msg, fix=None): fail("R1", pkg, "no row in the root README's plugin table", f"add a row linking packages/{pkg} under '## List of plugins'") + # Only the version comparisons and R5 need the pubspec. The podspec, + # changelog and override rules do not — and an earlier revision's + # `continue` here cancelled all of them on any parse error, reporting + # one problem out of five and saying nothing about the four it skipped. + # That is the same mistake the podspec check's comment below records + # having already learned once. pubspec_path = os.path.join(d, "pubspec.yaml") pubspec, err = load_yaml(pubspec_path) - if pubspec is None: - fail("R3", pkg, f"pubspec.yaml is unreadable or invalid ({err})") - continue # nothing below can be judged without it - if not isinstance(pubspec, dict): - fail("R3", pkg, "pubspec.yaml is not a YAML mapping") - continue - - version = pubspec.get("version") - version = str(version).strip() if version is not None else None - if not version: - fail("R2", pkg, "pubspec.yaml declares no version") + if err is not None: + fail("R3", pkg, f"pubspec.yaml could not be parsed ({err})", + "R2's version comparison and R5 are skipped for this package") + pubspec = None + elif pubspec is None: + fail("R3", pkg, "pubspec.yaml is empty", + "R2's version comparison and R5 are skipped for this package") + elif not isinstance(pubspec, dict): + fail("R3", pkg, "pubspec.yaml is not a YAML mapping", + "R2's version comparison and R5 are skipped for this package") + pubspec = None + + version = None + if isinstance(pubspec, dict): + raw_version = pubspec.get("version") + version = str(raw_version).strip() if raw_version is not None else None + if not version: + fail("R2", pkg, "pubspec.yaml declares no version") # R2 — podspec. Presence is checked whether or not the version parsed; # nesting it under the version check hid a missing podspec behind an @@ -242,18 +294,38 @@ def fail(rule, pkg, msg, fix=None): fail("R2", pkg, f"CHANGELOG.md starts at {newest}, pubspec.yaml says {version}", f"add a `## {version}` entry at the top") - # R4 — a nested override must not reach the archive. - nested = os.path.join(d, "example", "pubspec_overrides.yaml") - if os.path.isfile(nested): - pubignore, _ = read_text(os.path.join(d, ".pubignore")) - if pubignore is None or not pubignore_excludes(pubignore, "pubspec_overrides.yaml"): + # R4 — a nested override must not reach the archive. Walked rather + # than stat-ing example/ alone: the override only has to be below the + # package root to ship, and an example restructured into a subdirectory + # would otherwise become invisible to this rule. + nested = [ + os.path.relpath(os.path.join(where, "pubspec_overrides.yaml"), d) + for where, _, files in os.walk(d) + if "pubspec_overrides.yaml" in files and os.path.abspath(where) != os.path.abspath(d) + ] + if nested: + pubignore_path = os.path.join(d, ".pubignore") + pubignore, pubignore_err = read_text(pubignore_path) + if pubignore is not None: + covered = all(pubignore_excludes(pubignore, rel) for rel in nested) + else: + covered = False + if os.path.exists(pubignore_path): + # Blaming the override would send the reader to the wrong + # file; the override is fine, the .pubignore is unreadable. + fail("R3", pkg, f".pubignore is unreadable ({pubignore_err})") + if not covered: fail("R4", pkg, - "example/pubspec_overrides.yaml is committed but not excluded by .pubignore", + f"{', '.join(sorted(nested))} is committed but not excluded by .pubignore", "add `pubspec_overrides.yaml` to .pubignore, or delete the " "override if the dependency it pins is now published") - # R5 — the tvOS plugin class. - if not tvos_plugin_class(pubspec): + # R5 — the tvOS plugin class. Skipped (loudly, above) when the pubspec + # did not parse; a missing class cannot be distinguished from a missing + # file, and guessing either way would be a verdict we cannot support. + if pubspec is None: + pass + elif not tvos_plugin_class(pubspec): fail("R5", pkg, "declares no flutter.plugin.platforms.tvos.pluginClass", "without it the CLI never registers the plugin — " @@ -348,12 +420,28 @@ def _fixture(root, name="widget_tvos", **overrides): ("R2 version with build metadata is compared verbatim", {"pubspec.yaml": GOOD_PUBSPEC.format(name="widget_tvos").replace( "version: 0.0.1", "version: 0.0.1+1")}, "R2"), + # Expects BOTH rules: the point of the case is that R3 still runs when R2 + # has already failed, so asserting only R3 would let the co-firing it + # demonstrates go unchecked. ("R3 missing podspec is reported even without a version", {"pubspec.yaml": "name: widget_tvos\nflutter:\n plugin:\n platforms:\n" " tvos:\n pluginClass: GoodPlugin\n", - "tvos/widget_tvos.podspec": None}, "R3"), + "tvos/widget_tvos.podspec": None}, ["R2", "R3"]), ("R3 unparseable pubspec fails rather than vanishing", {"pubspec.yaml": "name: [unclosed\n"}, "R3"), + ("R3 each required file is enforced — LICENSE", {"LICENSE": None}, "R3"), + ("R3 each required file is enforced — CHANGELOG", {"CHANGELOG.md": None}, "R3"), + ("R3 each required file is enforced — README", {"README.md": None}, "R3"), + ("R2 podspec without an s.version at all", + {"tvos/widget_tvos.podspec": "Pod::Spec.new do |s|\n s.name = 'x'\nend\n"}, "R2"), + ("R2 changelog with no ## heading", {"CHANGELOG.md": "Nothing yet.\n"}, "R2"), + ("R2 changelog heading inside a fence is not the newest", + {"CHANGELOG.md": "```\n## 0.0.1\n```\n\n## Unreleased\n\n* wip\n"}, "R2"), + ("R5 no flutter block at all", + {"pubspec.yaml": "name: widget_tvos\nversion: 0.0.1\n"}, "R5"), + ("R5 platforms without a tvos key", + {"pubspec.yaml": "name: widget_tvos\nversion: 0.0.1\nflutter:\n plugin:\n" + " platforms:\n ios:\n pluginClass: RealIosPlugin\n"}, "R5"), ("R5 sibling platform's pluginClass does not satisfy tvos", {"pubspec.yaml": "name: widget_tvos\nversion: 0.0.1\nflutter:\n plugin:\n" " platforms:\n tvos:\n sharedDarwinSource: true\n" @@ -363,23 +451,30 @@ def _fixture(root, name="widget_tvos", **overrides): def selftest(): failures = 0 + exercised = set() for label, overrides, expect in CASES: root = tempfile.mkdtemp(prefix="check_repo_selftest.") try: - podspec_removed = overrides.get("tvos/widget_tvos.podspec", "keep") is None - _fixture(root, **{k: v for k, v in overrides.items() if v is not None}) - if podspec_removed: - os.remove(os.path.join(root, "packages", "widget_tvos", "tvos", - "widget_tvos.podspec")) + # `None` means "this file should not exist" — handled inside + # _fixture for every path. Filtering it out here instead left that + # branch dead and the deletion hardcoded to one filename, so a new + # case like {"LICENSE": None} silently built a *good* tree. + _fixture(root, **dict(overrides)) found, _ = check(root) - rules = {rule for rule, _, _, _ in found} + rules = sorted({rule for rule, _, _, _ in found}) + exercised.update(rules) if expect is None: ok = not found detail = "expected a clean run, got: " + "; ".join( f"{r} {m}" for r, _, m, _ in found) else: - ok = expect in rules - detail = f"expected {expect}, got {sorted(rules) or 'nothing'}" + # Exact set, not membership. `expect in rules` passed when the + # expected rule fired from a *different* branch than the case + # was written for — deleting R2's "heading is not a version" + # arm still produced an R2, from the version-mismatch arm, and + # the case stayed green over a dead branch. + ok = rules == sorted(set(expect if isinstance(expect, list) else [expect])) + detail = f"expected exactly {expect}, got {rules or 'nothing'}" print(f" {'ok ' if ok else 'FAIL'} {label}") if not ok: print(f" {detail}") @@ -391,6 +486,14 @@ def selftest(): # still ships. Built directly, since they need an example/ subtree. for label, pubignore, expect_fail in [ ("R4 real entry passes", "pubspec_overrides.yaml\n", False), + ("R4 root-anchored entry does not cover example/", + "/pubspec_overrides.yaml\n", True), + ("R4 a different filename does not count", + "my_pubspec_overrides.yaml\n", True), + ("R4 an entry scoped to another directory does not count", + "other_dir/pubspec_overrides.yaml\n", True), + ("R4 negation after a match re-includes the file", + "pubspec_overrides.yaml\n!example/pubspec_overrides.yaml\n", True), ("R4 comment does not count", "# pubspec_overrides.yaml\n", True), ("R4 negation does not count", "!pubspec_overrides.yaml\n", True), ("R4 missing .pubignore", None, True), @@ -404,6 +507,7 @@ def selftest(): with open(os.path.join(d, ".pubignore"), "w", encoding="utf-8") as handle: handle.write(pubignore) found, _ = check(root) + exercised.update(rule for rule, _, _, _ in found) fired = "R4" in {rule for rule, _, _, _ in found} ok = fired == expect_fail print(f" {'ok ' if ok else 'FAIL'} {label}") @@ -413,21 +517,76 @@ def selftest(): finally: shutil.rmtree(root, ignore_errors=True) + # R0 — the guards against a vacuous run. Built directly: both need a tree + # with no valid package in it, which _fixture exists to prevent. + for label, build, expect_rule in [ + ("R0 empty packages/ is not a silent pass", + lambda root: os.makedirs(os.path.join(root, "packages")), "R0"), + # Pins the regression `discover`'s docstring records: enumerating by + # "directories containing a pubspec" made such a directory VANISH from + # the run instead of failing it, and silently disagreed with the + # workflow's matrix. Nothing caught that until it was found by hand. + ("R3 a package directory with no pubspec is reported, not skipped", + lambda root: (_fixture(root), + os.makedirs(os.path.join(root, "packages", "stray_tvos"))), "R3"), + ("R0 unreadable root README is fatal, not skipped", + lambda root: (_fixture(root), os.remove(os.path.join(root, "README.md"))), "R0"), + ]: + root = tempfile.mkdtemp(prefix="check_repo_selftest.") + try: + build(root) + found, _ = check(root) + rules = {rule for rule, _, _, _ in found} + exercised.update(rules) + ok = expect_rule in rules + print(f" {'ok ' if ok else 'FAIL'} {label}") + if not ok: + print(f" expected {expect_rule}, got {sorted(rules) or 'nothing'}") + failures += 1 + finally: + shutil.rmtree(root, ignore_errors=True) + + # The self-test is what certifies every rule, so it must not be able to + # certify nothing. Emptying CASES made an earlier revision print "passed" + # and exit 0 with zero assertions — the exact anti-pattern this file exists + # to catch, one level up. Assert coverage of the rule set explicitly. + expected_rules = {"R0", "R1", "R2", "R3", "R4", "R5"} + missing = expected_rules - exercised + if missing: + print(f" FAIL no case exercises {', '.join(sorted(missing))}") + print(" A rule with no case is indistinguishable from a broken one.") + failures += 1 + print() if failures: - print(f"{failures} self-test case(s) failed — the gate itself is broken.") + print(f"{failures} self-test problem(s) — the gate itself is not trustworthy.") return 1 - print("Self-test passed: every rule fires on a bad tree and stays quiet on a good one.") + print(f"Self-test passed: {len(CASES) + 11} cases, every rule in " + f"{', '.join(sorted(expected_rules))} both fires and stays quiet.") return 0 def main(): - args = sys.argv[1:] - if "--selftest" in args: + # argparse rather than scanning sys.argv: the hand-rolled version accepted + # `check_repo.py --selftest`, ran the self-test, ignored the root and + # exited 0 — so collapsing the two CI steps into one command would have made + # the structural gate permanently green. It also silently accepted `--slftest` + # as "no flags at all". + parser = argparse.ArgumentParser(description=__doc__.split("\n")[0]) + parser.add_argument("root", nargs="?", default=".", help="repository root") + mode = parser.add_mutually_exclusive_group() + mode.add_argument("--list", action="store_true", + help="print package names, one per line") + mode.add_argument("--selftest", action="store_true", + help="verify every rule fires, and does not") + opts = parser.parse_args() + + if opts.selftest: + if opts.root != ".": + parser.error("--selftest takes no root; it builds its own trees") return selftest() - positional = [a for a in args if not a.startswith("--")] - root = os.path.abspath(positional[0] if positional else ".") - if "--list" in args: + root = os.path.abspath(opts.root) + if opts.list: for pkg in discover(os.path.join(root, "packages")): print(pkg) return 0 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6fb85f9..d4fb13f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,11 +70,14 @@ jobs: set -euo pipefail names=$(python3 .github/scripts/check_repo.py --list .) count=$(printf '%s\n' "$names" | grep -c . || true) - dirs=$(ls -d packages/*/ | wc -l | tr -d ' ') if [ "$count" -eq 0 ]; then echo "::error::No packages discovered — refusing to report a vacuous pass." exit 1 fi + # `find`, not `ls -d packages/*/`: with packages/ empty or absent the + # glob does not expand, ls exits 2, and `set -e` kills the step before + # the guard above can explain what happened. + dirs=$(find packages -mindepth 1 -maxdepth 1 -type d | wc -l | tr -d ' ') # A silent drop is the failure this whole job exists to prevent, so # compare against the raw directory count rather than trusting the # filter to have kept everything. @@ -84,7 +87,18 @@ jobs: printf '%s\n' "$names" exit 1 fi - echo "packages=$(printf '%s\n' "$names" | jq -R -s -c 'split("\n") | map(select(length > 0))')" >> "$GITHUB_OUTPUT" + # Assigned on its own line rather than inline in the echo: a failing + # jq inside a command substitution used as an argument is invisible to + # both `set -e` and pipefail, because `echo` itself succeeds. The step + # would go green having written an empty `packages` output, and the + # run would die later on "Unexpected end of JSON input" pointing at + # the wrong job. + pkgs=$(printf '%s\n' "$names" | jq -R -s -c 'split("\n") | map(select(length > 0))') + case "$pkgs" in + \[*\]) ;; + *) echo "::error::Package list is not a JSON array: '$pkgs'"; exit 1 ;; + esac + echo "packages=$pkgs" >> "$GITHUB_OUTPUT" echo "Discovered $count package(s)." package: @@ -127,7 +141,11 @@ jobs: - name: dart analyze (lib + test) run: | dart analyze lib - if [ -d test ]; then dart analyze test; fi + if [ -d test ]; then + dart analyze test + else + echo "::warning::no test/ directory to analyse" + fi # `flutter test` discovers test/**/*_test.dart recursively. An earlier # version gated this on `ls test/*.dart`, which only sees the top level: From 71969e62395e24df9e7ebb15cddbd53657922ac4 Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Fri, 14 Aug 2026 19:07:10 +0900 Subject: [PATCH 4/5] Correct three comments that were reasoned rather than measured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/scripts/check_repo.py | 6 ++++-- .github/workflows/ci.yml | 33 ++++++++++++++++++++++----------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/.github/scripts/check_repo.py b/.github/scripts/check_repo.py index 29350af..a0535e2 100755 --- a/.github/scripts/check_repo.py +++ b/.github/scripts/check_repo.py @@ -83,8 +83,10 @@ def tvos_plugin_class(pubspec): Parsed rather than scanned. The hand-rolled version walked out of the `tvos:` block into whichever platform followed it and returned *that* - platform's class, so a package declaring only `ios: pluginClass:` passed - the one rule whose failure is invisible until an app calls the plugin. + platform's class. The shape that slipped through was a `tvos:` block present + but pluginClass-less, followed by a platform that had one — a package with + no `tvos:` block at all was still caught. That is the case the R5 self-test + now pins, and the rule whose failure is invisible until an app calls it. """ if not isinstance(pubspec, dict): return None diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d4fb13f..98faa20 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,10 +25,15 @@ permissions: contents: read concurrency: - # Per-ref, and only cancellable on pull requests: cancelling a superseded PR - # run is what you want, but cancelling main's runs when two merges land close - # together leaves main's history dotted with cancelled builds and no record of - # whether the tree was ever green. + # Per-ref, and only cancellable on pull requests: superseding a PR run is what + # you want, while on main a cancelled run tells you nothing about whether that + # commit was ever green. + # + # This does NOT guarantee one build per merge. 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 can still skip a build. + # Guaranteeing one per merge would need a queue, which cannot be combined with + # a truthy cancel-in-progress. group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} @@ -166,13 +171,19 @@ jobs: # The example is what a user copies, so it has to resolve and analyse. # - # Note these do not all get the same treatment: eight examples ship no - # analysis_options.yaml of their own and therefore inherit the plugin's - # strict-casts / strict-inference / strict-raw-types, which produce - # errors. That is not hypothetical — the commit adding this workflow had - # to fix one such error in device_info_plus_tvos's example. The eight that - # do carry their own options file are checked more loosely. Worth - # unifying, but not by silently lowering the bar here. + # Note these do not all get the same treatment. Examples with no + # analysis_options.yaml of their own inherit the plugin's strict-casts / + # strict-inference / strict-raw-types. Measured on Dart 3.12: strict-casts + # produces errors, the other two produce warnings — which `dart analyze` + # also treats as fatal, so all three gate. Not hypothetical: the commit + # adding this workflow had to fix exactly such an error in + # device_info_plus_tvos's example. + # + # The examples that do carry an options file are not uniformly looser, and + # it would be a mistake to "unify" by assuming they are: path_provider's + # repeats the same strict block, sqflite's repeats two thirds of it, and + # flutter_secure_storage's uses very_good_analysis, which is stricter than + # anything here. Only the plain flutter_lints ones are genuinely looser. - name: example — resolve and analyse run: | if [ ! -f example/pubspec.yaml ]; then From a3e17727330b837f001e9f9c64a82e213ba42cfc Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Sat, 15 Aug 2026 00:34:02 +0900 Subject: [PATCH 5/5] Address the review: four rule/workflow fixes, one of them a test pinning a bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/scripts/check_repo.py | 41 +++++++++++++++++++++++++---------- .github/workflows/ci.yml | 21 ++++++++++++++++-- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/.github/scripts/check_repo.py b/.github/scripts/check_repo.py index a0535e2..e8b76df 100755 --- a/.github/scripts/check_repo.py +++ b/.github/scripts/check_repo.py @@ -146,12 +146,18 @@ def pubignore_excludes(text, relpath): pattern = line.rstrip("/") if not pattern: continue - if pattern.startswith("/"): - # Anchored to the package root: `/pubspec_overrides.yaml` excludes - # the root file and says nothing about example/'s. - hit = fnmatch.fnmatch(relpath, pattern.lstrip("/")) - elif "/" in pattern: - hit = fnmatch.fnmatch(relpath, pattern) + if pattern.startswith("/") or "/" in pattern: + # Anchored to the package root. Excluding a *directory* excludes its + # whole subtree, so match the pattern against every leading prefix + # of the path, not only against the path itself — `/example/` and + # `/example` both cover `example/pubspec_overrides.yaml`, which is + # what `git check-ignore` says and what pub follows. Comparing only + # the full path called that a miss and produced a false R4 telling + # the author to add an entry they had effectively already written. + anchored = pattern.lstrip("/") + parts = relpath.split("/") + prefixes = ["/".join(parts[:i]) for i in range(1, len(parts) + 1)] + hit = any(fnmatch.fnmatch(prefix, anchored) for prefix in prefixes) else: # Unanchored: matches the basename, or any directory component. parts = relpath.split("/") @@ -276,9 +282,17 @@ def fail(rule, pkg, msg, fix=None): declared = podspec_version(text) if declared is None: fail("R2", pkg, f"{spec} declares no s.version") - elif version and declared != version: - fail("R2", pkg, f"{spec} says {declared}, pubspec.yaml says {version}", - f"set s.version = '{version}'") + else: + # Dart allows `0.0.1+1`; CocoaPods does not. Pod::Version derives + # from Gem::Version, which rejects `+` outright, so demanding the + # podspec repeat the build metadata would leave an author with a + # red gate or a podspec that raises. Compare the release part. + want = version.split("+")[0] if version else None + if want and declared != want: + fail("R2", pkg, f"{spec} says {declared}, pubspec.yaml says {version}", + f"set s.version = '{want}'" + + (" (CocoaPods rejects Dart's `+build` suffix)" + if want != version else "")) # R2 — changelog. changelog, err = read_text(os.path.join(d, "CHANGELOG.md")) @@ -419,9 +433,14 @@ def _fixture(root, name="widget_tvos", **overrides): "Pod::Spec.new do |s|\n # s.version = '0.0.1'\n s.version = '9.9.9'\nend\n"}, "R2"), ("R2 non-version changelog heading on top", {"CHANGELOG.md": "## Unreleased\n\n* wip\n\n## 0.0.1\n\n* Initial.\n"}, "R2"), - ("R2 version with build metadata is compared verbatim", + # `0.0.1+1` in the pubspec against `0.0.1` in the podspec is CORRECT, since + # CocoaPods cannot express the build metadata. This case asserts the rule + # stays quiet — an earlier revision asserted the opposite and would have + # forced a podspec version that raises. + ("R2 build metadata is stripped, not demanded of the podspec", {"pubspec.yaml": GOOD_PUBSPEC.format(name="widget_tvos").replace( - "version: 0.0.1", "version: 0.0.1+1")}, "R2"), + "version: 0.0.1", "version: 0.0.1+1"), + "CHANGELOG.md": "## 0.0.1+1\n\n* Initial.\n"}, None), # Expects BOTH rules: the point of the case is that R3 still runs when R2 # has already failed, so asserting only R3 would let the co-firing it # demonstrates go unchecked. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 98faa20..530a200 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,6 +9,14 @@ name: CI # compiled, nothing runs on a simulator. Those stay a reviewer's job, and the # porting reports are where that evidence belongs. # +# One more blind spot worth naming: `flutter pub get` honours a committed root +# pubspec_overrides.yaml, and four packages ship one. For those, the hosted +# constraint in pubspec.yaml — the one consumers actually resolve against — is +# never exercised here. An unsatisfiable constraint resolves green. Closing it +# needs a second resolve with overrides moved aside; the cheaper fix is deleting +# the four overrides, which are obsolete now that firebase_core_tvos is +# published. +# # Worth knowing why the `repo` job exists at all: stock flutter_tools does not # validate the `tvos:` platform key. `_validateMultiPlatformYaml` knows only # android/ios/linux/macos/windows and ignores anything else, so `flutter pub @@ -82,7 +90,11 @@ jobs: # `find`, not `ls -d packages/*/`: with packages/ empty or absent the # glob does not expand, ls exits 2, and `set -e` kills the step before # the guard above can explain what happened. - dirs=$(find packages -mindepth 1 -maxdepth 1 -type d | wc -l | tr -d ' ') + # `-not -name '.*'` mirrors discover()'s filter. Without it a dotted + # directory under packages/ — a .template/ scaffold, a tool cache — + # makes the counts disagree and the step hard-fails blaming a silent + # drop that is not happening. + dirs=$(find packages -mindepth 1 -maxdepth 1 -type d -not -name '.*' | wc -l | tr -d ' ') # A silent drop is the failure this whole job exists to prevent, so # compare against the raw directory count rather than trusting the # filter to have kept everything. @@ -206,7 +218,12 @@ jobs: # a skipped job otherwise counts as success. ci: name: ci - if: always() + # `!cancelled()` rather than `always()`: both keep the job running when an + # upstream one is skipped (which is the property that matters, since a + # skipped job otherwise counts as success), but `always()` also runs on + # cancellation and then asserts 'cancelled' = 'success', posting a red + # required check for every superseded PR run. + if: ${{ !cancelled() }} needs: [repo, package] runs-on: ubuntu-latest steps: