You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up to #32/#33: COCOAPODS_PARALLEL_CODE_SIGN is a no-op, clean misses flutter_export_environment.sh, and plugin discovery duplicates findPackageConfigFile #36
Follow-up to #32 and #33, which are already merged. Both fixes are correct — I verified them against a real pub workspace and against cargokit's actual scripts — but the review landed after the merge, so these three items are now in main unaddressed. Each is small and independent.
1. COCOAPODS_PARALLEL_CODE_SIGN is in the wrong file and currently has no effect
lib/build_targets/application.dart — #33 writes this only into flutter_export_environment.sh.
It's consumed by CocoaPods' [CP] Embed Pods Frameworks script phase, which reads it as an Xcode build setting — that phase never sources flutter_export_environment.sh. cargokit ignores it too. So as written it does nothing.
Upstream builds a single xcodeBuildSettings list and writes it to both files (xcode_build_settings.dart:74 → Generated.xcconfig, :114 → the .sh), with the setting added at :185 — so upstream gets it into the xcconfig where it actually applies.
The tvOS Podfile uses use_frameworks! (templates/app/swift/tvos.tmpl/Podfile:16), so the embed phase exists here and would genuinely benefit from parallel signing.
// alongside the existing xcconfig.writeln(...) calls
xcconfig.writeln('COCOAPODS_PARALLEL_CODE_SIGN=true');
(Keep it in the .sh as well, for upstream parity.)
lib/commands/clean.dart:28 cleans Flutter/Generated.xcconfig, but #33 added a second generated file beside it that isn't registered. A stale script with an absoluteFLUTTER_ROOT / FLUTTER_APPLICATION_PATH / FLUTTER_BUILD_DIR therefore survives a clean.
Failure mode: user runs flutter-tvos clean, then moves/renames the project (or the flutter-tvos install), then opens Xcode directly rather than going through flutter-tvos build. cargokit's pod script phase sources the stale file and gets paths that no longer exist — failing with an opaque error pointing at a directory they've never heard of.
3. Plugin discovery re-implements findPackageConfigFile, which the repo already uses
lib/tvos_plugins.dart — #32 added a hand-rolled upward walk to locate .dart_tool/package_config.json. That helper already exists in flutter_tools (dart/package_map.dart:25,48) and is exposed as FlutterProject.packageConfig (project.dart:227). Upstream's own findPlugins() — the direct analogue of _walkPluginDependencies — uses it (flutter_plugins.dart:109).
And this repo already calls it: lib/tvos_builder.dart:79 → findPackageConfigFileOrDefault(project.directory).path. So the tvOS kernel path already resolved pub workspaces correctly while plugin discovery didn't — that asymmetry was the original bug.
_walkPluginDependencies already takes a FlutterProject and project.dart is already imported, so the added block collapses to one line with no new imports:
Upstream normalizes/absolutizes first (fileSystem.path.normalize(dir.absolute.path)). The hand-rolled loop walks project.directory as-is; for a relative path path.dirname('.') == '.', so the termination check is true on the first pass and the walk silently doesn't walk. Never bites today (the dir is always absolute), but it's a free guard.
Upstream terminates on fileSystem.path.equals(...) rather than a raw string != (case/separator-insensitive on Windows).
While there: the rootUri.startsWith('./') clause added in #32 is dead code — pub never emits ./ rootUris (I checked three real package_config.json files, 169/273/98 packages: every non-file:// entry starts with ../). One expression handles ../, ./, bare-relative, file://, and percent-decoding uniformly:
(rootUri is a URI, so a path with a space arrives as ../my%20plugin; the current ../ branch does a raw string join and never decodes it.)
4. Test coverage for both fixes
Neither PR shipped a test, and in both cases the codebase already has the pattern:
Plugin discovery — test/general/tvos_plugins_test.dart is ~1100 lines, but every existing test uses an absolute file:// rootUri (:573, :671, :735, :778, :842, :964, :1068). The ../-relative branch that fix: plugin discovery finds package_config.json in pub workspaces #32 rewrote is pinned by nothing; you could break relative resolution outright and the suite stays green. A workspace-member test (model it on the ensureReadyForTvosTooling end-to-end group at :539) would be the regression guard: /ws/.dart_tool/package_config.json with rootUri: '../packages/foo_tvos', plugin pubspec declaring flutter.plugin.platforms.tvos, app at /ws/apps/my_app with no local .dart_tool → assert foo_tvos reaches GeneratedPluginRegistrant.
xcconfig / export script — nothing in test/general/ asserts on generated xcconfig content. NativeTvosBundle already exposes static, MemoryFileSystem-testable builders for exactly this (copyFlutterAssetsTree, buildAppFrameworkInfoPlist, tvosGenSnapshotArgs — see tvos_app_bundle_test.dart, tvos_aot_snapshot_test.dart). Extracting the two string builders as statics would let a test pin: FLUTTER_ROOT present in the xcconfig; the .sh starts with #!/bin/sh and every line is export "NAME=VALUE"; every var in the xcconfig also appears in the .sh (that one catches item 1 above); and a flutterRoot containing a space round-trips intact.
Also worth doing eventually (not from these PRs)
lib/tvos_plugins.dart:171-174 — a plugin that's in the dependency graph but can't be resolved is dropped with a bare continue: no warning, not even a trace. The tool knows it has an inconsistency (the dep graph comes from .flutter-plugins-dependencies, which pub populates correctly regardless of platform keys) and discards the evidence. That silence is why the workspace bug was invisible until it surfaced as MissingPluginException at runtime — and it still covers every other route to an empty/partial package map (malformed package_config.json at :174, unexpected JSON shape at :176, missing pubspec at the resolved path). One warning here would have turned that bug into a 30-second diagnosis:
if (pluginPath ==null) {
globals.logger.printWarning(
'tvOS: plugin "$pluginName" is in the dependency graph but could not be ''resolved in ${packageConfigFile.path}. It will not be registered; calls ''into it will fail at runtime with MissingPluginException.',
);
continue;
}
The same pattern is already used correctly ~400 lines down for .flutter-plugins-dependencies (:583-592).
Follow-up to #32 and #33, which are already merged. Both fixes are correct — I verified them against a real pub workspace and against cargokit's actual scripts — but the review landed after the merge, so these three items are now in
mainunaddressed. Each is small and independent.Full review notes: #32 review · #33 review
1.
COCOAPODS_PARALLEL_CODE_SIGNis in the wrong file and currently has no effectlib/build_targets/application.dart— #33 writes this only intoflutter_export_environment.sh.It's consumed by CocoaPods'
[CP] Embed Pods Frameworksscript phase, which reads it as an Xcode build setting — that phase never sourcesflutter_export_environment.sh. cargokit ignores it too. So as written it does nothing.Upstream builds a single
xcodeBuildSettingslist and writes it to both files (xcode_build_settings.dart:74→Generated.xcconfig,:114→ the.sh), with the setting added at:185— so upstream gets it into the xcconfig where it actually applies.The tvOS Podfile uses
use_frameworks!(templates/app/swift/tvos.tmpl/Podfile:16), so the embed phase exists here and would genuinely benefit from parallel signing.(Keep it in the
.shas well, for upstream parity.)2.
flutter-tvos cleandoesn't removeflutter_export_environment.shlib/commands/clean.dart:28cleansFlutter/Generated.xcconfig, but #33 added a second generated file beside it that isn't registered. A stale script with an absoluteFLUTTER_ROOT/FLUTTER_APPLICATION_PATH/FLUTTER_BUILD_DIRtherefore survives a clean.Failure mode: user runs
flutter-tvos clean, then moves/renames the project (or the flutter-tvos install), then opens Xcode directly rather than going throughflutter-tvos build. cargokit's pod script phase sources the stale file and gets paths that no longer exist — failing with an opaque error pointing at a directory they've never heard of.3. Plugin discovery re-implements
findPackageConfigFile, which the repo already useslib/tvos_plugins.dart— #32 added a hand-rolled upward walk to locate.dart_tool/package_config.json. That helper already exists influtter_tools(dart/package_map.dart:25,48) and is exposed asFlutterProject.packageConfig(project.dart:227). Upstream's ownfindPlugins()— the direct analogue of_walkPluginDependencies— uses it (flutter_plugins.dart:109).And this repo already calls it:
lib/tvos_builder.dart:79→findPackageConfigFileOrDefault(project.directory).path. So the tvOS kernel path already resolved pub workspaces correctly while plugin discovery didn't — that asymmetry was the original bug._walkPluginDependenciesalready takes aFlutterProjectandproject.dartis already imported, so the added block collapses to one line with no new imports:Two behavioural improvements come free:
fileSystem.path.normalize(dir.absolute.path)). The hand-rolled loop walksproject.directoryas-is; for a relative pathpath.dirname('.') == '.', so the termination check is true on the first pass and the walk silently doesn't walk. Never bites today (the dir is always absolute), but it's a free guard.fileSystem.path.equals(...)rather than a raw string!=(case/separator-insensitive on Windows).While there: the
rootUri.startsWith('./')clause added in #32 is dead code — pub never emits./rootUris (I checked three realpackage_config.jsonfiles, 169/273/98 packages: every non-file://entry starts with../). One expression handles../,./, bare-relative,file://, and percent-decoding uniformly:(
rootUriis a URI, so a path with a space arrives as../my%20plugin; the current../branch does a raw string join and never decodes it.)4. Test coverage for both fixes
Neither PR shipped a test, and in both cases the codebase already has the pattern:
Plugin discovery —
test/general/tvos_plugins_test.dartis ~1100 lines, but every existing test uses an absolutefile://rootUri (:573, :671, :735, :778, :842, :964, :1068). The../-relative branch that fix: plugin discovery finds package_config.json in pub workspaces #32 rewrote is pinned by nothing; you could break relative resolution outright and the suite stays green. A workspace-member test (model it on theensureReadyForTvosTooling end-to-endgroup at :539) would be the regression guard:/ws/.dart_tool/package_config.jsonwithrootUri: '../packages/foo_tvos', plugin pubspec declaringflutter.plugin.platforms.tvos, app at/ws/apps/my_appwith no local.dart_tool→ assertfoo_tvosreachesGeneratedPluginRegistrant.xcconfig / export script — nothing in
test/general/asserts on generated xcconfig content.NativeTvosBundlealready exposes static, MemoryFileSystem-testable builders for exactly this (copyFlutterAssetsTree,buildAppFrameworkInfoPlist,tvosGenSnapshotArgs— seetvos_app_bundle_test.dart,tvos_aot_snapshot_test.dart). Extracting the two string builders as statics would let a test pin:FLUTTER_ROOTpresent in the xcconfig; the.shstarts with#!/bin/shand every line isexport "NAME=VALUE"; every var in the xcconfig also appears in the.sh(that one catches item 1 above); and aflutterRootcontaining a space round-trips intact.Also worth doing eventually (not from these PRs)
lib/tvos_plugins.dart:171-174— a plugin that's in the dependency graph but can't be resolved is dropped with a barecontinue: no warning, not even a trace. The tool knows it has an inconsistency (the dep graph comes from.flutter-plugins-dependencies, which pub populates correctly regardless of platform keys) and discards the evidence. That silence is why the workspace bug was invisible until it surfaced asMissingPluginExceptionat runtime — and it still covers every other route to an empty/partial package map (malformedpackage_config.jsonat:174, unexpected JSON shape at:176, missing pubspec at the resolved path). One warning here would have turned that bug into a 30-second diagnosis:The same pattern is already used correctly ~400 lines down for
.flutter-plugins-dependencies(:583-592).