Avoid depending on files from build_system/targets other than from top level entrypoints in flutter_tools. (flutter#142760)

chingjun · web-flow · commit ac7879e2aa6d · 2024-02-02T18:23:08.000Z
Add a new `BuildTargets` class that provides commonly used build targets. And avoid importing files from `build_system/targets` except from the top level entrypoints or from top level commands. Also move `scene_importer.dart` and `shader_compiler.dart` into `build_system/tools` because they are not `Target` classes, but wrapper for certain tools. With this change, we can ignore all files in `build_system/targets` internally and make PR flutter#142709 easier to land internally. See cl/603434066 for the corresponding internal change. Related to: flutter#142709 flutter#142041 Also note that I have opted to add a new variable in `globals.dart` for `BuildTargets` in this PR, but I know that we are trying to get rid of globals. Several alternatives that I was considering: 1. Add a new field in `BuildSystem` that returns a `BuildTargets` instance. Since `BuildSystem` is already in `globals`, we can access build targets using `globals.buildSystem.buildTargets` without adding a new global variable. 2. Properly inject the `BuildTargetsImpl` instance from the top level `executable.dart` and top level commands. Let me know if you want me to do one of the above instead. Thanks!
diff --git a/packages/flutter_tools/lib/executable.dart b/packages/flutter_tools/lib/executable.dart
@@ -11,6 +11,7 @@ import 'src/base/platform.dart';
 import 'src/base/template.dart';
 import 'src/base/terminal.dart';
 import 'src/base/user_messages.dart';
+import 'src/build_system/build_targets.dart';
 import 'src/cache.dart';
 import 'src/commands/analyze.dart';
 import 'src/commands/assemble.dart';
@@ -47,6 +48,7 @@ import 'src/devtools_launcher.dart';
 import 'src/features.dart';
 import 'src/globals.dart' as globals;
 // Files in `isolated` are intentionally excluded from google3 tooling.
+import 'src/isolated/build_targets.dart';
 import 'src/isolated/mustache_template.dart';
 import 'src/isolated/resident_web_runner.dart';
 import 'src/pre_run_validator.dart';
@@ -110,6 +112,7 @@ Future<void> main(List<String> args) async {
         logger: globals.logger,
         botDetector: globals.botDetector,
       ),
+      BuildTargets: () => const BuildTargetsImpl(),
       Logger: () {
         final LoggerFactory loggerFactory = LoggerFactory(
           outputPreferences: globals.outputPreferences,
diff --git a/packages/flutter_tools/lib/src/build_system/build_targets.dart b/packages/flutter_tools/lib/src/build_system/build_targets.dart
@@ -0,0 +1,58 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import '../base/file_system.dart';
+import '../web/compiler_config.dart';
+import './build_system.dart';
+
+/// Commonly used build [Target]s.
+abstract class BuildTargets {
+  const BuildTargets();
+
+  Target get copyFlutterBundle;
+  Target get releaseCopyFlutterBundle;
+  Target get generateLocalizationsTarget;
+  Target get dartPluginRegistrantTarget;
+  Target webServiceWorker(FileSystem fileSystem, List<WebCompilerConfig> compileConfigs);
+}
+
+/// BuildTargets that return NoOpTarget for every action.
+class NoOpBuildTargets extends BuildTargets {
+  const NoOpBuildTargets();
+
+  @override
+  Target get copyFlutterBundle => const _NoOpTarget();
+
+  @override
+  Target get releaseCopyFlutterBundle => const _NoOpTarget();
+
+  @override
+  Target get generateLocalizationsTarget => const _NoOpTarget();
+
+  @override
+  Target get dartPluginRegistrantTarget => const _NoOpTarget();
+
+  @override
+  Target webServiceWorker(FileSystem fileSystem, List<WebCompilerConfig> compileConfigs) => const _NoOpTarget();
+}
+
+/// A [Target] that does nothing.
+class _NoOpTarget extends Target {
+  const _NoOpTarget();
+
+  @override
+  String get name => 'no_op';
+
+  @override
+  List<Source> get inputs => const <Source>[];
+
+  @override
+  List<Source> get outputs => const <Source>[];
+
+  @override
+  List<Target> get dependencies => const <Target>[];
+
+  @override
+  Future<void> build(Environment environment) async {}
+}
diff --git a/packages/flutter_tools/lib/src/build_system/targets/assets.dart b/packages/flutter_tools/lib/src/build_system/targets/assets.dart
@@ -12,10 +12,10 @@ import '../../convert.dart';
 import '../../devfs.dart';
 import '../build_system.dart';
 import '../depfile.dart';
+import '../tools/scene_importer.dart';
+import '../tools/shader_compiler.dart';
 import 'common.dart';
 import 'icon_tree_shaker.dart';
-import 'scene_importer.dart';
-import 'shader_compiler.dart';
 
 /// A helper function to copy an asset bundle into an [environment]'s output
 /// directory.
diff --git a/packages/flutter_tools/lib/src/build_system/targets/common.dart b/packages/flutter_tools/lib/src/build_system/targets/common.dart
@@ -16,12 +16,12 @@ import '../../globals.dart' as globals show xcode;
 import '../build_system.dart';
 import '../depfile.dart';
 import '../exceptions.dart';
+import '../tools/shader_compiler.dart';
 import 'assets.dart';
 import 'dart_plugin_registrant.dart';
 import 'icon_tree_shaker.dart';
 import 'localizations.dart';
 import 'native_assets.dart';
-import 'shader_compiler.dart';
 
 /// Copies the pre-built flutter bundle.
 // This is a one-off rule for implementing build bundle in terms of assemble.
diff --git a/packages/flutter_tools/lib/src/build_system/targets/ios.dart b/packages/flutter_tools/lib/src/build_system/targets/ios.dart
@@ -20,10 +20,10 @@ import '../../reporting/reporting.dart';
 import '../build_system.dart';
 import '../depfile.dart';
 import '../exceptions.dart';
+import '../tools/shader_compiler.dart';
 import 'assets.dart';
 import 'common.dart';
 import 'icon_tree_shaker.dart';
-import 'shader_compiler.dart';
 
 /// Supports compiling a dart kernel file to an assembly file.
 ///
diff --git a/packages/flutter_tools/lib/src/build_system/targets/web.dart b/packages/flutter_tools/lib/src/build_system/targets/web.dart
@@ -29,15 +29,6 @@ import '../exceptions.dart';
 import 'assets.dart';
 import 'localizations.dart';
 
-/// Whether the application has web plugins.
-const String kHasWebPlugins = 'HasWebPlugins';
-
-/// Base href to set in index.html in flutter build command
-const String kBaseHref = 'baseHref';
-
-/// The caching strategy to use for service worker generation.
-const String kServiceWorkerStrategy = 'ServiceWorkerStrategy';
-
 @visibleForTesting
 List<String> updateDartDefines(List<String> dartDefines, WebRendererMode webRenderer) {
   final Set<String> dartDefinesSet = dartDefines.toSet();
diff --git a/packages/flutter_tools/lib/src/build_system/tools/scene_importer.dart b/packages/flutter_tools/lib/src/build_system/tools/scene_importer.dart
@@ -92,7 +92,7 @@ class SceneImporter {
   /// See [Target.inputs].
   static const List<Source> inputs = <Source>[
     Source.pattern(
-        '{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/scene_importer.dart'),
+        '{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/tools/scene_importer.dart'),
     Source.hostArtifact(HostArtifact.scenec),
   ];
 
diff --git a/packages/flutter_tools/lib/src/build_system/tools/shader_compiler.dart b/packages/flutter_tools/lib/src/build_system/tools/shader_compiler.dart
@@ -136,7 +136,7 @@ class ShaderCompiler {
   ///
   /// See [Target.inputs].
   static const List<Source> inputs = <Source>[
-    Source.pattern('{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart'),
+    Source.pattern('{FLUTTER_ROOT}/packages/flutter_tools/lib/src/build_system/tools/shader_compiler.dart'),
     Source.hostArtifact(HostArtifact.impellerc),
   ];
 
diff --git a/packages/flutter_tools/lib/src/bundle_builder.dart b/packages/flutter_tools/lib/src/bundle_builder.dart
@@ -12,9 +12,8 @@ import 'base/logger.dart';
 import 'build_info.dart';
 import 'build_system/build_system.dart';
 import 'build_system/depfile.dart';
-import 'build_system/targets/common.dart';
-import 'build_system/targets/scene_importer.dart';
-import 'build_system/targets/shader_compiler.dart';
+import 'build_system/tools/scene_importer.dart';
+import 'build_system/tools/shader_compiler.dart';
 import 'bundle.dart';
 import 'cache.dart';
 import 'devfs.dart';
@@ -78,8 +77,8 @@ class BundleBuilder {
       generateDartPluginRegistry: true,
     );
     final Target target = buildInfo.mode == BuildMode.debug
-        ? const CopyFlutterBundle()
-        : const ReleaseCopyFlutterBundle();
+        ? globals.buildTargets.copyFlutterBundle
+        : globals.buildTargets.releaseCopyFlutterBundle;
     final BuildResult result = await buildSystem.build(target, environment);
 
     if (!result.success) {
diff --git a/packages/flutter_tools/lib/src/commands/create_base.dart b/packages/flutter_tools/lib/src/commands/create_base.dart
@@ -544,6 +544,7 @@ abstract class CreateBase extends FlutterCommand {
       await generateLocalizationsSyntheticPackage(
         environment: environment,
         buildSystem: globals.buildSystem,
+        buildTargets: globals.buildTargets,
       );
     }
     final List<SupportedPlatform> platformsForMigrateConfig = <SupportedPlatform>[SupportedPlatform.root];
diff --git a/packages/flutter_tools/lib/src/commands/packages.dart b/packages/flutter_tools/lib/src/commands/packages.dart
@@ -307,6 +307,7 @@ class PackagesGetCommand extends FlutterCommand {
         await generateLocalizationsSyntheticPackage(
           environment: environment,
           buildSystem: globals.buildSystem,
+          buildTargets: globals.buildTargets,
         );
       } else if (rootProject.directory.childFile('l10n.yaml').existsSync()) {
         final Environment environment = Environment(
diff --git a/packages/flutter_tools/lib/src/dart/generate_synthetic_packages.dart b/packages/flutter_tools/lib/src/dart/generate_synthetic_packages.dart
@@ -8,11 +8,12 @@ import '../base/common.dart';
 import '../base/file_system.dart';
 import '../base/utils.dart';
 import '../build_system/build_system.dart';
-import '../build_system/targets/localizations.dart';
+import '../build_system/build_targets.dart';
 
 Future<void> generateLocalizationsSyntheticPackage({
   required Environment environment,
   required BuildSystem buildSystem,
+  required BuildTargets buildTargets,
 }) async {
 
   final FileSystem fileSystem = environment.fileSystem;
@@ -54,7 +55,7 @@ Future<void> generateLocalizationsSyntheticPackage({
   }
 
   final BuildResult result = await buildSystem.build(
-    const GenerateLocalizationsTarget(),
+    buildTargets.generateLocalizationsTarget,
     environment,
   );
 
diff --git a/packages/flutter_tools/lib/src/devfs.dart b/packages/flutter_tools/lib/src/devfs.dart
@@ -15,8 +15,8 @@ import 'base/logger.dart';
 import 'base/net.dart';
 import 'base/os.dart';
 import 'build_info.dart';
-import 'build_system/targets/scene_importer.dart';
-import 'build_system/targets/shader_compiler.dart';
+import 'build_system/tools/scene_importer.dart';
+import 'build_system/tools/shader_compiler.dart';
 import 'compile.dart';
 import 'convert.dart' show base64, utf8;
 import 'vmservice.dart';
diff --git a/packages/flutter_tools/lib/src/globals.dart b/packages/flutter_tools/lib/src/globals.dart
@@ -27,6 +27,7 @@ import 'base/terminal.dart';
 import 'base/time.dart';
 import 'base/user_messages.dart';
 import 'build_system/build_system.dart';
+import 'build_system/build_targets.dart';
 import 'cache.dart';
 import 'custom_devices/custom_devices_config.dart';
 import 'device.dart';
@@ -54,6 +55,7 @@ import 'version.dart';
 
 Artifacts? get artifacts => context.get<Artifacts>();
 BuildSystem get buildSystem => context.get<BuildSystem>()!;
+BuildTargets get buildTargets => context.get<BuildTargets>()!;
 Cache get cache => context.get<Cache>()!;
 CocoaPodsValidator? get cocoapodsValidator => context.get<CocoaPodsValidator>();
 Config get config => context.get<Config>()!;
diff --git a/packages/flutter_tools/lib/src/isolated/build_targets.dart b/packages/flutter_tools/lib/src/isolated/build_targets.dart
@@ -0,0 +1,32 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import '../base/file_system.dart';
+import '../build_system/build_system.dart';
+import '../build_system/build_targets.dart';
+import '../build_system/targets/common.dart';
+import '../build_system/targets/dart_plugin_registrant.dart';
+import '../build_system/targets/localizations.dart';
+import '../build_system/targets/web.dart';
+import '../web/compiler_config.dart';
+
+class BuildTargetsImpl extends BuildTargets {
+  const BuildTargetsImpl();
+
+  @override
+  Target get copyFlutterBundle => const CopyFlutterBundle();
+
+  @override
+  Target get releaseCopyFlutterBundle => const ReleaseCopyFlutterBundle();
+
+  @override
+  Target get generateLocalizationsTarget => const GenerateLocalizationsTarget();
+
+  @override
+  Target get dartPluginRegistrantTarget => const DartPluginRegistrantTarget();
+
+  @override
+  Target webServiceWorker(FileSystem fileSystem, List<WebCompilerConfig> compileConfigs) =>
+      WebServiceWorker(fileSystem, compileConfigs);
+}
diff --git a/packages/flutter_tools/lib/src/isolated/devfs_web.dart b/packages/flutter_tools/lib/src/isolated/devfs_web.dart
@@ -24,8 +24,8 @@ import '../base/logger.dart';
 import '../base/net.dart';
 import '../base/platform.dart';
 import '../build_info.dart';
-import '../build_system/targets/scene_importer.dart';
-import '../build_system/targets/shader_compiler.dart';
+import '../build_system/tools/scene_importer.dart';
+import '../build_system/tools/shader_compiler.dart';
 import '../bundle_builder.dart';
 import '../cache.dart';
 import '../compile.dart';
diff --git a/packages/flutter_tools/lib/src/resident_runner.dart b/packages/flutter_tools/lib/src/resident_runner.dart
@@ -24,10 +24,8 @@ import 'base/terminal.dart';
 import 'base/utils.dart';
 import 'build_info.dart';
 import 'build_system/build_system.dart';
-import 'build_system/targets/dart_plugin_registrant.dart';
-import 'build_system/targets/localizations.dart';
-import 'build_system/targets/scene_importer.dart';
-import 'build_system/targets/shader_compiler.dart';
+import 'build_system/tools/scene_importer.dart';
+import 'build_system/tools/shader_compiler.dart';
 import 'bundle.dart';
 import 'cache.dart';
 import 'compile.dart';
@@ -1260,8 +1258,8 @@ abstract class ResidentRunner extends ResidentHandlers {
     );
 
     final CompositeTarget compositeTarget = CompositeTarget(<Target>[
-      const GenerateLocalizationsTarget(),
-      const DartPluginRegistrantTarget(),
+      globals.buildTargets.generateLocalizationsTarget,
+      globals.buildTargets.dartPluginRegistrantTarget,
     ]);
 
     _lastBuild = await globals.buildSystem.buildIncremental(
diff --git a/packages/flutter_tools/lib/src/runner/flutter_command.dart b/packages/flutter_tools/lib/src/runner/flutter_command.dart
@@ -1707,6 +1707,7 @@ Run 'flutter -h' (or 'flutter <command> -h') for available flutter commands and
       await generateLocalizationsSyntheticPackage(
         environment: environment,
         buildSystem: globals.buildSystem,
+        buildTargets: globals.buildTargets,
       );
 
       await pub.get(
diff --git a/packages/flutter_tools/lib/src/web/compile.dart b/packages/flutter_tools/lib/src/web/compile.dart
@@ -13,7 +13,6 @@ import '../base/project_migrator.dart';
 import '../base/utils.dart';
 import '../build_info.dart';
 import '../build_system/build_system.dart';
-import '../build_system/targets/web.dart';
 import '../cache.dart';
 import '../flutter_plugins.dart';
 import '../globals.dart' as globals;
@@ -28,6 +27,15 @@ import 'migrations/scrub_generated_plugin_registrant.dart';
 
 export 'compiler_config.dart';
 
+/// Whether the application has web plugins.
+const String kHasWebPlugins = 'HasWebPlugins';
+
+/// Base href to set in index.html in flutter build command
+const String kBaseHref = 'baseHref';
+
+/// The caching strategy to use for service worker generation.
+const String kServiceWorkerStrategy = 'ServiceWorkerStrategy';
+
 class WebBuilder {
   WebBuilder({
     required Logger logger,
@@ -81,7 +89,7 @@ class WebBuilder {
     final Stopwatch sw = Stopwatch()..start();
     try {
       final BuildResult result = await _buildSystem.build(
-          WebServiceWorker(_fileSystem, compilerConfigs),
+          globals.buildTargets.webServiceWorker(_fileSystem, compilerConfigs),
           Environment(
             projectDir: _fileSystem.currentDirectory,
             outputDir: outputDirectory,
diff --git a/packages/flutter_tools/test/general.shard/build_system/targets/shader_compiler_test.dart b/packages/flutter_tools/test/general.shard/build_system/targets/shader_compiler_test.dart
@@ -9,7 +9,7 @@ import 'package:file_testing/file_testing.dart';
 import 'package:flutter_tools/src/artifacts.dart';
 import 'package:flutter_tools/src/base/logger.dart';
 import 'package:flutter_tools/src/build_info.dart';
-import 'package:flutter_tools/src/build_system/targets/shader_compiler.dart';
+import 'package:flutter_tools/src/build_system/tools/shader_compiler.dart';
 import 'package:flutter_tools/src/devfs.dart';
 
 import '../../../src/common.dart';
diff --git a/packages/flutter_tools/test/general.shard/cold_test.dart b/packages/flutter_tools/test/general.shard/cold_test.dart
@@ -7,7 +7,7 @@ import 'package:flutter_tools/src/base/file_system.dart';
 import 'package:flutter_tools/src/base/io.dart';
 import 'package:flutter_tools/src/base/platform.dart';
 import 'package:flutter_tools/src/build_info.dart';
-import 'package:flutter_tools/src/build_system/targets/shader_compiler.dart';
+import 'package:flutter_tools/src/build_system/tools/shader_compiler.dart';
 import 'package:flutter_tools/src/compile.dart';
 import 'package:flutter_tools/src/devfs.dart';
 import 'package:flutter_tools/src/device.dart';
diff --git a/packages/flutter_tools/test/general.shard/dart/generate_synthetic_packages_test.dart b/packages/flutter_tools/test/general.shard/dart/generate_synthetic_packages_test.dart
diff --git a/packages/flutter_tools/test/general.shard/devfs_test.dart b/packages/flutter_tools/test/general.shard/devfs_test.dart
diff --git a/packages/flutter_tools/test/general.shard/hot_test.dart b/packages/flutter_tools/test/general.shard/hot_test.dart
diff --git a/packages/flutter_tools/test/general.shard/resident_runner_test.dart b/packages/flutter_tools/test/general.shard/resident_runner_test.dart
diff --git a/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart b/packages/flutter_tools/test/general.shard/resident_web_runner_test.dart
diff --git a/packages/flutter_tools/test/general.shard/terminal_handler_test.dart b/packages/flutter_tools/test/general.shard/terminal_handler_test.dart
diff --git a/packages/flutter_tools/test/general.shard/web/devfs_web_test.dart b/packages/flutter_tools/test/general.shard/web/devfs_web_test.dart
diff --git a/packages/flutter_tools/test/integration.shard/shader_compiler_test.dart b/packages/flutter_tools/test/integration.shard/shader_compiler_test.dart
diff --git a/packages/flutter_tools/test/src/context.dart b/packages/flutter_tools/test/src/context.dart

Original file line number	Diff line number	Diff line change
`@@ -544,6 +544,7 @@ abstract class CreateBase extends FlutterCommand {`
`544`	`544`	`await generateLocalizationsSyntheticPackage(`
`545`	`545`	`environment: environment,`
`546`	`546`	`buildSystem: globals.buildSystem,`
	`547`	`+ buildTargets: globals.buildTargets,`
`547`	`548`	`);`
`548`	`549`	`}`
`549`	`550`	`final List<SupportedPlatform> platformsForMigrateConfig = <SupportedPlatform>[SupportedPlatform.root];`