Skip to content

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

Description

@DenisovAV

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.

Full review notes: #32 review · #33 review


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:74Generated.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.)


2. flutter-tvos clean doesn't remove flutter_export_environment.sh

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 absolute FLUTTER_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.

_cleanFile(tvosDir, 'Flutter/flutter_export_environment.sh');

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:79findPackageConfigFileOrDefault(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:

final File packageConfigFile = project.packageConfig;

Two behavioural improvements come free:

  • 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 = packageConfigFile.parent.uri.resolve(rootUri).toFilePath();

(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 discoverytest/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).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions