Repository navigation
Skip platform tooling regeneration for non-Flutter pub workspace packages - #189705
auto-submit[bot] merged 4 commits into
Conversation
…ages `flutter pub get` regenerates platform-specific tooling for every workspace root package listed in package_graph.json. A pub workspace root that is a plain Dart package was processed too, so a stray ios/ or android/ directory at the workspace root was populated with Flutter project files (GeneratedPluginRegistrant, Generated.xcconfig, etc.). Skip packages that do not depend on flutter, directly or transitively, according to the resolved package graph. Workspace members that are Flutter apps are still processed, including apps that only depend on Flutter transitively (e.g. through a plugin). Fixes flutter#189550
github-actions
Bot
added
the
tool
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents regenerating platform-specific tooling for workspace packages that do not depend on Flutter, addressing a regression where plain Dart workspace roots were populated with Flutter project files. It introduces a helper method to check for transitive dependencies on Flutter and adds a corresponding test. The review feedback suggests a minor refactoring in the dependency traversal logic to avoid using a null-coalescing operator with an empty list fallback, improving efficiency and readability.
| if (current == 'flutter') { | ||
| return true; | ||
| } | ||
| toVisit.addAll(graph.dependencies[current] ?? const <String>[]); |
There was a problem hiding this comment.
Instead of using the null-coalescing operator with a constant empty list fallback (?? const <String>[]), we can retrieve the dependencies list and only call addAll if it is non-null. This avoids referencing the fallback list and calling addAll on an empty list, making the code slightly cleaner and more efficient.
final List<String>? dependencies = graph.dependencies[current];
if (dependencies != null) {
toVisit.addAll(dependencies);
}
|
Thanks for the PR @UditDewan! This looks good to me overall, just a couple of minor comments before I can approve. |
| /// transitively, according to the resolved package [graph]. | ||
| static bool _dependsOnFlutter(PackageGraph graph, String packageName) { | ||
| final visited = <String>{}; | ||
| final toVisit = <String>[packageName]; |
There was a problem hiding this comment.
Nit: we should use a Queue here instead of a List.
| if (current == 'flutter') { | ||
| return true; | ||
| } | ||
| toVisit.addAll(graph.dependencies[current] ?? const <String>[]); |
Switch the pending-package worklist in _dependsOnFlutter from a List to a Queue, and read the dependency list into a local instead of falling back to a constant empty list.
chingjun
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/189705, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
bkonyi
added
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/189705, because - The status or check suite Tree_analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
bkonyi
added
CICD
|
autosubmit label was removed for flutter/flutter/189705, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
Bot
removed
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
bkonyi
added
the
cp: stable
3 participants
flutter pub getpost-processing iterates every workspace root package listed in.dart_tool/package_graph.jsonand unconditionally regenerates platform-specific tooling for each of them. SinceregeneratePlatformSpecificToolinggates only on whether platform directories exist on disk, a pub workspace root that is a plain Dart package (no dependency on Flutter) with a strayios/orandroid/directory got that directory populated with Flutter project files (GeneratedPluginRegistrant.*,Generated.xcconfig,flutter_export_environment.sh, ...).This PR skips platform tooling regeneration for workspace packages that do not depend on the
flutterpackage, directly or transitively, according to the resolved package graph. Checking the transitive closure (rather than just the direct dependencies of the pubspec) keeps workspace members that only depend on Flutter through another package (e.g. an app that only declares a dependency on a plugin) working exactly as before, which is covered by the existing testget creates plugin registrants for each app in workspace.Adds a regression test that sets up a workspace whose root is a plain Dart package with empty
ios/andandroid/directories, and verifies thatflutter pub getleaves them empty while still processing the Flutter app member of the workspace. The test fails before this change (ios/gainsRunner/andFlutter/directories) and passes after it.Fixes #189550
Pre-launch Checklist
///).