Release 1.8.0 — run the Dart build hooks for tvOS, as tvOS - #72
Conversation
The last manual step in the chain was pushing a vX.Y.Z tag after main synced. Forgetting it is silent: main carries a bumped version, no release exists, and nothing reports anything. `Tag release` runs after `Sync main`, reads the version from main's pubspec, and does nothing at all unless there is no matching tag — which is the case for most merges, so the quiet path is the common one. The version is the trigger because bumping it is already a considered decision made in a reviewed pull request; this carries that decision out rather than making a new one. The part worth knowing: GitHub suppresses workflow triggers for pushes made with GITHUB_TOKEN, so the pushed tag cannot start `Release` the way a hand-pushed one does. Left implicit, the tag would land with no release built and no failure reported — the exact silent gap this is meant to close. `Release` therefore gained a `workflow_call` entry point and is invoked explicitly, and the suppression becomes useful rather than harmful: the two entry points cannot race to create the same release. A hand-pushed tag still works unchanged. `Release` also takes the tag as an input now, so under `workflow_call` it checks out the tag rather than the caller's ref, which would otherwise be whatever main happened to be by then. No new secrets: a PAT would have worked too, at the cost of an expiry that fails this the same silent way.
Tags here are v<flutter>-tvos.<cli> — v3.47.1-tvos.1.7.0 — and every release since 3.41.4 follows it. Using the CLI version alone drops the one thing the scheme exists to record: which Flutter release these artifacts and this CLI go together with. The two move independently, so the pairing is not derivable after the fact. Only the commit is pinned in the repository, so the Flutter version is resolved back from it against flutter/flutter's tags rather than duplicated into a second file that could drift from the pin. A pin that no release tag points at is a hard failure: it means the pair cannot be named, and guessing would produce a tag that misdescribes what shipped.
A build hook can produce data assets: files generated on the host by ordinary Dart and chosen for the platform being built, which is how a 3D or shader package compiles its GPU bundles. tvOS skipped that pass, because the same pass also carries Dart code assets, which a tvOS app never uses — its plugins are native and resolved by the package manager. Skipping it was not neutral. Such a package shipped whatever its generated directory happened to hold: output left behind by a macOS or simulator build of the same tree, or nothing at all. Neither failed the build, so the only symptom was an app that rendered wrong. The pass is driven here rather than through flutter_tools' translation layer. That layer maps flutter_tools' own closed set of target platforms onto the hooks protocol, and tvOS is not in it, so anything routed through it is announced as iOS. `TvosCodeAssetExtension` instead lets `CodeAssetExtension` build the config and then writes `target_os` onto it, which the protocol treats as a plain string on both sides — so tvOS can name itself even though the `code_assets` the tool pins has no `OS` for it. The resident runner does the same, so a hot reload and a build no longer name different targets. Code assets are requested and dropped on the floor. The protocol keeps the target OS inside the code-asset config, so asking is the only way to say `tvos`; nothing a code-asset hook produces is installed into a tvOS app, and the bundle is unchanged. The risk this adds is hooks running where none ran before, so the pass falls back. `package:code_assets` parses `target_os` eagerly and throws on a name it does not know until 2.0.0, which means a hook as ordinary as `objective_c`'s dies on its first line rather than reporting tvOS as unsupported. When any hook refuses the name, the pass is retried under the iOS family, which is a faithful `IOSCodeConfig` and the shape these builds sent before. Verified on an app whose only dependency is path_provider: it fails as `tvos`, retries, and builds. An app with no hook-carrying packages returns in 23 ms and is untouched.
CLI-only. `bin/internal/engine.version` and `bin/internal/flutter.version` are untouched, so this is the same Flutter and engine pair 1.7.0 shipped and `precache` has nothing new to fetch. The tagging scheme names both halves, so this releases as v3.47.1-tvos.1.8.0. Minor rather than patch: the build gained a pass it did not run before. Nothing an existing app relies on changes, but what a hook is handed does. Also clears the two analyzer infos the new test file introduced — an import out of order and a `final env` whose type is not obvious from the initializer. CI analyzes lib/ only, so neither would have failed anything; the rest of the 201 infos in lib/ and test/ predate this branch and are left alone.
`dart analyze` exits 0 on infos, so the analyze job reported green while 201 of them accumulated across lib/ and test/. It also only looked at lib/, so nothing watched the tests at all. It now runs `--fatal-infos` over lib/, test/ and bin/, and analyzes packages/flutter_tvos too — a pull request stops on an analyzer finding the way it stops on a failing test. release.yml gets the same command, so a tag cannot ship what a pull request would have been stopped for. Clearing the backlog: 190 of the 201 are `dart fix --apply`, almost entirely `omit_obvious_local_variable_types` and its inverse. The rest by hand: - Six locals in tvos_plugins.dart typed `Object?` rather than left `dynamic`. All six hold a `json.decode` result or a dynamic map lookup and are narrowed by an `is` check on the next line, so `Object?` is both what the rule wants and the safer annotation — it makes the check mandatory instead of optional. - `[CP] Embed Pods Frameworks` in a doc comment was split across two lines, so the analyzer read `[CP]` as a doc reference rather than part of a code span. Kept on one line. - An adjacent-string pair in compatibility_database.dart broke mid-word at a hyphen; moved the break to the space before it. - `prefer_foreach` on a warning loop, and the two infos this branch's own test file introduced. No reformatting. `dart format` would rewrite 51 files here, which is a separate question from the analyzer and not one this branch answers; nothing in CI checks formatting today. 405 tests still pass.
PR #67's branch, folded in here so the version bump this branch carries and the workflow that acts on it land together rather than in an order that decides whether 1.8.0 releases.
CLAUDE.md is globally gitignored, so it exists in nobody's clone but the author's, and naming internal tooling in a public repository is not what these comments were for. Both already stated the rule they were attributing — that a podspec cannot depend on the Flutter pod, and that a subclass of a `base class` must itself be `base` — so the attributions came out and the rules stayed. The second was misattributed anyway: `base` is Dart 3's requirement, not a project convention.
DenisovAV
left a comment
There was a problem hiding this comment.
Review: build hooks for tvOS
The diagnosis behind this is right, and it is the kind of bug that stays invisible until someone notices their shaders look wrong: hooks were skipped, so data assets were whatever a previous macOS or simulator build left behind, and nothing failed. Driving the pass directly rather than through flutter_tools' translation layer is the correct call — that layer has no tvOS and would announce iOS no matter what. Writing target_os onto the config built by upstream's CodeAssetExtension is a neat way through a protocol whose typed side has no name for us yet.
I verified the load-bearing claims rather than taking them: messages-side, attempt 1 really does put target_os: tvos on the wire with no ios sub-config, the fallback really does carry a faithful IOSCodeConfig, there are exactly two attempts, and code assets never reach the bundle. Those all hold.
Four things below block, and the first is the one that matters most.
🔴 Out of the box this runs every hook and collects nothing
lib/build_targets/tvos_hooks.dart:186
final DataAssetsExtension? dataAssets = featureFlags.isDartDataAssetsEnabled
? DataAssetsExtension()
: null;In the pinned SDK, flutter_tools/lib/src/features.dart:
const nativeAssets = Feature(
master: FeatureChannelSetting(available: true, enabledByDefault: true),
beta: FeatureChannelSetting(available: true, enabledByDefault: true),
stable: FeatureChannelSetting(available: true, enabledByDefault: true),
);
const dartDataAssets = Feature(
master: FeatureChannelSetting(available: true), // no enabledByDefault
);and FeatureChannelSetting({this.available = false, this.enabledByDefault = false}). Nothing in lib/ or bin/ supplies its own FeatureFlags — I grepped.
So unless the user has run flutter config --enable-dart-data-assets, dataAssets is null, no data assets are requested, none are produced, and the build reports success. Every package's hook still runs, code assets are still requested and discarded, and any app that trips the fallback runs them all twice.
The guard meant to catch this cannot fire:
if (!featureFlags.isNativeAssetsEnabled && !featureFlags.isDartDataAssetsEnabled) {
throwToolExit('... Enable data assets using `flutter config --enable-dart-data-assets`.');
}isNativeAssetsEnabled is true by default on every channel, so && short-circuits and the message telling the user exactly what to do is unreachable.
Net: on a default install this release adds the cost and the failure surface of running hooks, and delivers none of the benefit — the stale-asset bug survives untouched. Which raises the question about the Verified table: the 3D app row says "named tvos, no retry, ships Metal bundles". That can only have happened with the flag enabled, and neither the PR body nor the CHANGELOG mentions it.
Splitting the guard so the data-assets branch has its own condition, and warning (naming the packages and the command) when hooks exist but data assets are off, would turn this from a silent no-op into a one-line fix for the user.
🔴 flutter-tvos run fails on exactly the app the Verified table covers
_iosFamilyConfig requires the SDK root:
// tvos_hooks.dart:136
final String? sdkRoot = environment.defines[kSdkRoot];
if (sdkRoot == null) {
throw MissingDefineException(kSdkRoot, 'tvos_build_hooks');
}kSdkRoot is supplied in one place — lib/tvos_builder.dart:82, the one-shot build path. The resident path does not go through it:
executable.dart:208 FlutterHookRunner: () => TvosHookRunner()
run.dart:804 dartBuilder: hookRunner
run_hot.dart:493 dartBuilder?.runHooks(environment: environment)
resident_runner.dart:1035 defines: {kTargetFile, kBuildMode} ← no kSdkRoot
tvos_hooks.dart:394 passes that Environment through unchanged
So: an app with path_provider → objective_c, whose hook refuses tvos. flutter-tvos build falls back and builds — the Verified table's row 2. flutter-tvos run on the same app reaches the same fallback and throws MissingDefineException, which is not a ToolExit and is not caught on the way out. The message is Target tvos_build_hooks required define SdkRoot but it was not provided, which says nothing about a package that could not read an OS name.
It fires on the first DevFS upload, so at session start, not only on reload. A reviewer confirmed it with a probe driving runTvosHooks with those two defines and nothing else.
This is also the one divergence the PR set out to prevent — TvosHookRunner exists so a reload and a build name the same target. They do name the same target; one of them just cannot complete. Deriving the SDK root in _iosFamilyConfig when the define is absent (the call tvos_builder.dart:52 already makes) closes it.
🔴 The retry fires on the whole failure set, and explains itself with a cause it never checked
tvos_hooks.dart:211 — the only condition is if (result == null).
Upstream collapses the reason away: FlutterNativeAssetsBuildRunnerImpl.build (native_assets.dart:612) turns Result<BuildResult, HooksRunnerFailure> into BuildResult?, so all four failure kinds — hookRun, infra, projectConfig, internal — arrive as null. A missing toolchain, a compile error, a fetch that timed out, or a hook that deliberately reports tvOS as unsupported are indistinguishable from one that choked on the name.
All of them then print this:
A package holding this back is one that reads its target OS through
code_assets' typed accessor on acode_assetsolder than 2.0.0, which throws on any OS it does not already know.
Nothing established that. A developer chasing a broken Rust hook is pointed at code_assets versions.
To be fair on two points, because the first draft of this review overstated both. It is not silent: hooks_runner logs each failure at SEVERE with the exit code, a repro command and full stderr, and the bridge at native_assets.dart:563 forwards that to printError — so the real error is on screen, next to a confident misdiagnosis of it. And an exception thrown by the runner propagates rather than retrying, so the retry catches the null-collapsed set, not literally anything.
The consequence when the retry then succeeds is what matters: data assets selected for iOS ship, and nothing records which OS the pass was announced as, so no later build can notice. Gating the retry on the failure actually being the unknown-OS one — or, failing that, saying the first attempt failed for an unknown reason and the retry is a guess — costs nothing and removes the misdirection.
🔴 The tag is public before anything validates it
tag-release.yml:103 pushes the tag in the tag job; :111 runs release as needs: tag. Everything that could fail — the flutter/flutter clone, dart test, flutter test, the new --fatal-infos gate — runs after the tag exists.
If that job fails, the tag is on the repository with no Release behind it, and the documented way out does not lead out:
# Recovery hatch: re-run against whatever main holds now.
workflow_dispatch:A re-run reaches if git rev-parse -q --verify "refs/tags/$TAG", sets should_release=false and exits, skipping the release. It is recoverable — "Re-run failed jobs" splices the successful tag job's outputs into a new attempt, and deleting the tag makes dispatch work — but neither is what the comment describes, and neither is written down anywhere.
Ordering the jobs the other way, or letting the existence check also ask gh release view, would make the hatch match its comment.
Worth fixing before this ships
The tests cannot see the thing that would break. Every test drives a fake returning const FakeFlutterNativeAssetsBuilderResult(), whose encodedAssets and dependencies are both [] — so no test has a single asset in flight, and the only assertion about the written result file is existsSync(). Three mutants applied to tvos_hooks.dart and run:
| mutation | suite |
|---|---|
invert if (asset.isDataAsset) — discard every data asset |
4/4 green |
| delete the retry announcement entirely | 4/4 green |
try { … } on Object { result = null; } around attempt 1 |
4/4 green |
The first is the original bug reintroduced; the third is the retry-catches-everything behaviour above, made worse. TvosHookRunner has no test at all — grep finds it only at its definition and its two registration lines — which is why the crash above reached review.
The suite is not weak everywhere: it genuinely pins the wire-level target_os, the absent ios sub-config, the faithful fallback config and the two-attempt sequence. It is specifically blind to asset flow and to failure paths. A fake that returns one DataAsset, plus an assertion on the result file's contents, closes most of it. TvosHookRunner needs the same injection seam TvosBuildHooks already has.
The release fires on the Flutter pin, not the version it says it fires on. The header states "The version in pubspec.yaml is the trigger, deliberately… Nothing here decides whether to release", but the gate is the composite TAG="v$FLUTTER_VERSION-tvos.$VERSION". Bumping bin/internal/flutter.version alone changes the tag name and publishes a release for a CLI version nobody bumped, with no CHANGELOG entry — while the release notes link to CHANGELOG.md.
This is not hypothetical. Merge #60 landed on dev with the pin at 3.44.9 while pubspec.yaml still read 1.5.0; "Release 1.5.1" came later. With this workflow in place it would have published v3.44.9-tvos.1.5.0 — a tag the real history does not contain.
The CHANGELOG's case list is missing a case. The reassurance is correctly scoped ("a hook that cannot cope with an unfamiliar target OS… so apps that built before still build") and true for that class. But the two sentences read as exhaustive, and a third case regresses: hooks never ran for tvOS before, so an app whose hook fails for a non-OS reason built on 1.7.0 and hard-fails on 1.8.0 via _throwHookFailed. The PR body says this plainly ("the risk added is hooks running where none ran before"); the CHANGELOG should too.
Scope. Four independent changes with four different risk profiles: the feature, the release automation, a ~15-file lint sweep, and the version bump. The lint churn is what makes application.dart hard to read, and the two workflow defects above sit in the middle of a 2,600-line diff where nobody reading for the feature would look. The body concedes the coupling, which is honest, but conceding it does not make it reviewable — approving the analyzer cleanup currently also approves publishing a release.
Smaller
_nameWatchOS(tvos_hooks.dart:120) writes the tvOS name. A watchOS copy-paste; worth a skim for other inherited assumptions.- The same function does
config['extensions']! as Mapthen[_codeAssetType]! as Map. If a future layout keeps the block but movestarget_os, this writes a dead key, raises nothing, triggers no fallback, and every build quietly announces iOS again. The test assertingtarget_os == 'tvos'is the only thing standing between that and a silent regression — worth a comment saying so. environmentTypeFromSdkroot(...)!(:141) returns null for any SDK path whose basename lacksiphone. Safe today because the value comes fromxcode.sdkLocation, but this is the fallback path, and pointingkSdkRootat the tvOS SDK — the natural thing for someone to try — turns it into a null-check crash inside a retry.cCompilerConfigMacOS(throwIfNotFound: false)is unconditional, where upstream ties it to whether the assets matter. A data-asset hook that shells out to the provided compiler getsnull, fails, retries withnull, fails again, and_throwHookFailedblames the dependency rather than naming Xcode.- Discarded code assets are never counted or logged, so "no code assets existed" and "code assets existed and were dropped" are indistinguishable afterwards.
release.yml's new analyze step says it "matches the gate in test.yml", buttest.ymlgained two steps and this gained one — a lint regression insidepackages/flutter_tvosblocks a PR and not a tag.tag-release.yml:67— underset -euo pipefail, thegrepthat finds no matching tag aborts the step before the three-line diagnostic below it can print.
|
One addition, which surfaced after I posted the review. Device and simulator are indistinguishable on the primary tvOS path. The irony is that the fallback does not lose the distinction. The comment above That matters for the motivating example specifically. tvOS device and simulator GPU bundles genuinely differ: metallibs compiled against As far as I can see the protocol has no room for this yet — there is no |
… the retry Four things this got wrong, all found in review. **It collected nothing on a default install.** `dartDataAssets` declares no `enabledByDefault` on any channel; `nativeAssets` declares it on all of them. So the guard reading both with `&&` could never fire, and with data assets off the pass ran every package's hook, requested code assets, discarded them, and reported success having produced nothing — the stale-asset bug it exists to fix surviving untouched, at the cost of running hooks and of the failures they can raise. It now returns before running anything and says what is off, which packages it skipped, and the one command that turns it on. Every end-to-end result in the pull request had been measured on a machine where the flag was already set, which is why this was invisible from here. **`run` crashed the moment the retry fired.** A resident session's Environment carries `kTargetFile` and `kBuildMode` and nothing else, so requiring `kSdkRoot` threw `MissingDefineException` — not a `ToolExit`, uncaught, and worded as a missing define rather than as a package that could not be told an OS name. It fires on the first DevFS upload, so at session start. The fallback now derives the SDK root when the define is absent, from the call the builder already makes, and stops asserting non-null on a path whose basename may not name an iPhone SDK. Verified on an app with `path_provider`: `run` reaches the retry, completes, and the VM Service comes up. **The retry named a cause it never checked.** `build` collapses a missing toolchain, a compile error, a timed-out fetch and a hook that choked on the name into the same null, and the message confidently blamed the last of those, next to the real error on screen. It now says the reason is above, that the retry is a guess, and what the guess would help with. **The tag went public before anything validated it.** The existence check asked about the tag, so the documented `workflow_dispatch` hatch exited as "already released" against a tag with no Release behind it. It now asks whether the *release* exists, and finishes a stranded tag rather than skipping it. It also asks whether this CLI *version* has shipped under any pin, because the tag name carries the Flutter pin too: bumping `bin/internal/flutter.version` alone would otherwise publish a release for a version nobody bumped and no CHANGELOG entry describes. Also from review, smaller: `_nameWatchOS` was a copy-paste from the watchOS port; the casts it makes are load-bearing and now say so; `release.yml` claimed to match test.yml's gate while running one of its two steps; and the `grep` that resolves the Flutter version aborted the step under `set -o pipefail` before its own diagnostic could print. The tests could not see any of it. Every one drove a fake returning no assets, so inverting the `isDataAsset` filter — the original bug, reintroduced — left the suite green, and `TvosHookRunner` had no test at all, which is how the crash above reached review. The fake now carries assets, the result file is asserted on its contents rather than its existence, the runner has the same injection seam the target has, and the disabled-feature and derived-SDK-root paths are covered. 409 tests, up from 405; the inverted-filter mutant now fails.
|
Pushed The no-op was the important one. You are right, and the reason it was invisible to me is that Rather than splitting the guard, the pass now returns before running anything when data assets are off. Its entire product is data assets — the code assets it asks for are the OS carrier and get discarded — so with the feature off there is nothing to collect, and running every hook to collect it is cost that can also fail a build. It names what is off, which packages it skipped, and the command. Measured on the
The retry no longer diagnoses. It says the reason is above, that the retry is a guess, and what the guess would help with. Gating on the actual cause is not reachable — The tag hatch. The existence check now asks Tests. The fake carries assets now, the result file is asserted on contents, Smaller ones all done: On scope, you were right and I under-corrected. I checked the numbers: 39 files, 1107+/219−, so the "2,600-line" figure is about double — but the shape of your point holds and line count was the wrong measure. 31 of the 39 files are the lint sweep, which is exactly what buries two workflow files in the Files-changed tab. I kept it in one PR at the author's call; the release automation and the version bump are genuinely one decision (they were coupled on purpose, so merge order could not decide whether 1.8.0 shipped), but the lint sweep has no such tie and I should have split it. The CHANGELOG now carries the flag requirement and the third case — an app whose hook fails for a non-OS reason built on 1.7.0 and fails on 1.8.0. |
Confirmed rather than reasoned: a device build and a simulator build of the same app land in one `hooks_runner` cache entry, `3bafeb9361` for both, because the inputs are byte-identical. There is nothing on this path to separate them — `target_os` is `tvos` either way, the architecture is arm64 either way, and the typed side of the protocol carries device-versus-simulator in a per-OS sub-config that exists for Android, iOS and macOS and not for tvOS. Naming the simulator as its own OS would invent vocabulary no hook reads, so there is nothing to fix here yet; what there is to do is say so, in the doc comment a reader of this file will reach and in the release notes. Worth noting where it bites: a hook shipping source is unaffected, and the motivating one does — the shader bundles this pass exists for carry Metal source, no `metallib` header, so the engine compiles them on the device they end up on. A hook that precompiled instead would be exposed, because Metal validates a library's platform when a pipeline is created rather than degrading. The iOS-family fallback keeps the distinction, its `targetSdk` coming from the SDK root, which leaves the degraded path better off than the accurate one on this one point.
|
Confirmed, and recorded in I verified the core claim rather than reasoning about it: a device build and a simulator build of the same app land in one And there is nowhere to put it: One thing worth adding on severity, which cuts in the PR's favour and I would rather state than let stand unexamined. Your mechanism is right — differing target triples, and Metal validating a library's platform at pipeline creation rather than degrading. But the motivating example does not hit it today: the shader bundles this pass exists for carry Metal source, not a compiled library. That narrows who is exposed to hooks that precompile per-platform binaries — which is a real class, and nothing stops flutter_scene from becoming one, so the boundary is worth recording either way. I have written it as "a hook that ships source is unaffected; one that precompiles is not" rather than as a live bug in the example. |
The test written to pin device-versus-simulator on the fallback path failed on its first run, which is the answer to whether it was worth writing. `environmentTypeFromSdkroot` does not return null for a path that is not an iPhone SDK — it `assert(false)`s and then returns null, so the `?? physical` on the previous commit was unreachable and an `AppleTVOS.sdk` crashed the tool on an assertion, inside a retry, where it reads as the retry being broken. Asking only whether the basename ends in `simulator` answers the same question for every Apple SDK, this platform's included, so pointing kSdkRoot at the tvOS SDK now works rather than either crashing or silently reporting device. The test pins what the fallback still gets right: an `AppleTVOS.sdk` produces `iphoneos` and an `iPhoneSimulator.sdk` produces `iphonesimulator`. The primary path cannot make that distinction at all, which is recorded a few lines above it.
|
Answering the question your comment raises but does not ask: is anything actually broken on an Apple TV today? Measured, no — and the test written to check it found a different bug, which is the better outcome. Not broken today. A device build and a simulator build of That does not make the boundary less real, just differently scoped, and it is recorded as you asked: a hook shipping source is unaffected, one that precompiles is not. On whether it needs a test. The primary path cannot distinguish device from simulator, and a test asserting that would be a tautology — nothing in the extension takes an environment type, so it could not fail. What is worth pinning is the property you pointed out as the irony: the fallback does keep them apart, through So if (sdkName.contains('iphone')) { ... }
assert(false);
return null;It asserts first. So the Fixed by not calling it: asking whether the basename ends in 410 tests. The same fix and test are on flutterwatch/flutter-watchos#5, which had it identically. |
|
Verified on a physical Apple TV 4K (tvOS 26.6) — until now everything in this PR had been measured on the simulator, which is a weak place to stand given what your last comment is about. It renders: 1920x1080, That is the part that matters for the device/simulator question. The shader bundle running on that Apple TV came out of the shared hook cache entry Also closed a second gap while the hardware was attached: release/AOT, which had never been run either — every result before this was The one thing still open is installing on a physical watch: the build is verified, the install failed on |
DenisovAV
left a comment
There was a problem hiding this comment.
Re-reviewed against d4595bd. All four blockers are closed, and I checked them rather than reading the summary.
The no-op. tvos_hooks.dart:242 now has its own condition instead of the && that could never fire, warns at printWarning, names the packages and the command, and returns before running anything. Returning early rather than splitting the guard is the better call — the pass's whole product is data assets, so with them off there is nothing to collect and running every hook to collect it is cost that can also fail a build.
The run crash. _iosFamilyConfig derives the SDK root from xcode.sdkLocation when the define is absent, and the environment-type read no longer routes through a function that asserts. The doc comment at :189 names the exact case I raised.
The retry. It now says the reason is above, calls itself a guess, and states what the guess would help with. That is the whole fix — the misdirection was the defect, not the retry.
The tag hatch. gh release view instead of git rev-parse, so a stranded tag gets finished; and git tag -l "*-tvos.$VERSION" closes the pin-only over-fire, which is the part I had least expected to be fixed in the same pass.
Tests. I ran my own mutant rather than taking your word: inverting if (asset.isDataAsset) now fails on keeps the data assets and drops the code assets. The suite is 9 in that file, the result file's contents are asserted, and the resident runner drives the same pass covers the class that had none.
Two corrections of mine to record, since both were yours and both were right.
The line count. "2,600-line diff" was wrong — that is the diff file including context. The change is 1107+/219−. The shape of the scope point stands, but I measured the wrong thing to make it, and 31 of 39 files being the lint sweep is the number that actually carries the argument.
The device/simulator severity. You narrowed it against your own interest, which I want to acknowledge: the mechanism is real, but the motivating bundle ships Metal source rather than a compiled library, so it does not bite today. Recording it as "a hook that ships source is unaffected; one that precompiles is not" is more accurate than what I wrote.
And the best outcome here was not any of the fixes: writing the test I asked for surfaced environmentTypeFromSdkroot asserting rather than returning null, which made the ?? EnvironmentType.physical unreachable and turned my own smaller point into a crash inside a retry. That is the test earning its place on its first run.
Verifying on a physical Apple TV 4K, and closing release/AOT while the hardware was attached, answers the thing my last comment was really about. The shared cache entry 3bafeb9361 serving both device and simulator is now a measured fact rather than an argument.
One leftover, cosmetic: the PR body still says an explicit aotPayloadStaged: true was added to tvos_app_bundle_test.dart; the diff removes one. The equal-to-default claim holds either way.
Approving.
|
Thank you — and for re-running the mutant rather than taking the summary. Fixed the leftover: the PR body now says both One thing from your original review I have not closed, and would rather name than let pass quietly now that this is approved: Happy either way: I can push a small fix — the failure message naming a missing host compiler when that is what happened — or leave it as follow-up so the approval stands against what you reviewed. Your call, and no rush from my side. |
tvOS skipped the Dart build-hook pass entirely. That pass carries two different
things: Dart code assets, which a tvOS app genuinely never uses (its plugins
are native and resolved by the package manager), and data assets, which are
files generated on the host by ordinary Dart and chosen for the platform being
built — how a 3D or shader package compiles its GPU bundles.
Skipping both was not neutral. Such a package shipped whatever its generated
directory happened to hold: output left behind by a macOS or simulator build of
the same tree, or nothing at all. Neither failed the build, so the only symptom
was an app that rendered wrong.
What this does
Runs the pass, and runs it as tvOS.
It is driven directly rather than through flutter_tools' translation layer,
which maps flutter_tools' own closed set of target platforms onto the hooks
protocol and has no tvOS in it — anything routed through it is announced as
iOS.
TvosCodeAssetExtensionletsCodeAssetExtensionbuild the config andthen writes
target_osonto it, which the protocol treats as a plain string onboth sides, so tvOS names itself even though the
code_assetsthe tool pins hasno
OSfor it.TvosHookRunnerdoes the same during a resident session, so ahot reload and a build no longer name different targets.
Code assets are requested and dropped on the floor: the protocol keeps the
target OS inside the code-asset config, so asking is the only way to say
tvos. Nothing a code-asset hook produces is installed into a tvOS app, and theapp bundle is unchanged.
The Flutter SDK checkout is not patched — it is gitignored and re-cloned, so
this had to work from the CLI alone.
The fallback
The risk added is hooks running where none ran before.
package:code_assetsparses
target_oseagerly and throws on a name it does not know until 2.0.0,so a hook as ordinary as
objective_c's dies on its first line rather thanreporting tvOS as unsupported:
When any hook refuses the name, the pass retries under the iOS family — a
faithful
IOSCodeConfig, the shape these builds sent before — and says so,because the hooks have just printed a stack trace and an unexplained retry
reads as a broken build. Whether the name lands is a property of the app's
packages and cannot be predicted from the resolved
code_assetsversion, so itis tried rather than guessed. This retires itself as packages move to 2.x.
kSdkRootis now supplied byTvosBuilderbecause that fallback needs it.Verified
path_provider(objective_chook)tvos, retries, buildstvos, no retry, ships Metal bundlesThe 3D app's shipped
manifest.jsoncarries 8metalIosentries and the bundlecontents are Metal shader source; it renders on the Apple TV 4K simulator with
Impeller Metal. The hook runner's own inputs show
ios,tvosas separatecache entries, which is the point — they are separate targets now, not one iOS
target reused.
dart analyzeclean onlibandtest;dart test405 passing. Four newtests in
test/general/tvos_build_hooks_test.dartcover what the hook is told,the fallback (exactly two attempts, second one
iosand carrying the iOSconfig), where the result is written, and the failure message.
Release 1.8.0
This branch carries the version bump, so merging it releases 1.8.0.
CLI-only.
bin/internal/engine.versionandbin/internal/flutter.versionare untouched — the same Flutter and engine pair 1.7.0 shipped — so
precachehas nothing new to fetch. The tag will be
v3.47.1-tvos.1.8.0.Minor rather than patch: the build gained a pass it did not run before. Nothing
an existing app relies on changes, but what a hook is handed does.
Folds in #67 (auto-tag release)
ci/auto-tag-releaseis merged into this branch, so #67's content ships hereand that PR can be closed. It merged clean — both branches touch
.github/workflows/release.ymlin different places, and the merged file keepsworkflow_callfrom #67 alongside the tightened analyze step from this branch.This changes what merging does. With
tag-release.ymlpresent, the chain ismerge → Test → Sync main → main carries
version: 1.8.0→ Tag release sees nomatching tag → pushes
v3.47.1-tvos.1.8.0→ Release builds and publishes.So merging this PR publishes release 1.8.0, rather than leaving a tag to be
pushed by hand. That is what #67 was built to do and the version bump here is
the considered decision it acts on — but it is worth stating plainly, because
the two are only coupled once they are in the same branch.
The engine and Flutter pins are unchanged, so the artifacts that release would
reference already exist.
Analyzer audit and gate
dart analyzeexits 0 on infos, so the analyze job reported green while 201of them accumulated across
lib/andtest/. It also only looked atlib/,so nothing watched the tests at all.
The gate now matches the tests.
test.ymlrunsdart analyze --fatal-infos lib/ test/ bin/plusflutter analyze --fatal-infosinpackages/flutter_tvos, so a pull request stops on ananalyzer finding the way it stops on a failing test.
release.ymlgets the samecommand, so a tag cannot ship what a pull request would have been stopped for.
The backlog is cleared — 0 issues across
lib/,test/,bin/and thepackage. 190 of the 201 were
dart fix --apply, almost entirelyomit_obvious_local_variable_typesand its inverse. The eleven left over, byhand:
tvos_plugins.darttypedObject?rather than leftdynamic.Each holds a
json.decoderesult or a dynamic map lookup and is narrowed byan
ischeck on the next line, soObject?is both what the rule wants andthe safer annotation: it makes that check mandatory instead of optional.
[CP] Embed Pods Frameworksin a doc comment was split across two lines, sothe analyzer read
[CP]as a doc reference rather than part of a code span.compatibility_database.dartbroke mid-word at ahyphen; the break moved to the space before it.
prefer_foreachon a warning loop, and the two this branch's own test fileintroduced.
Two
dart fixresults worth a look on review, both removals intvos_app_bundle_test.dart. It dropped a!fromcontains(m.group(1)!)(
containstakesObject?, and that regex always yields group 1 when itmatches). And it dropped an explicit
aotPayloadStaged: trueargument, whichequalled the parameter's default — the file's other call sites pass only the
non-default
false.No reformatting.
dart formatwould rewrite 51 files in this repo, which isa separate question from the analyzer and not one this branch answers; nothing
in CI checks formatting today. Tests pass throughout; 410 as of the last
commit on this branch.