Rename and define FlutterManifest.generateLocalizations. (flutter#1…

…60401) Part of flutter#102983. `<FlutterManifest>.generateSyntheticPackage` _never_ meant generate a synthetic package 😒, it only meant "we _might_ need to generate a synthetic package because localizations are being generated and the default, unless otherwise specified, is to generate a synthetic package". Renamed as `generateLocalizations` and added some strategic TODOs in places where removing the `package:flutter_gen` feature (flutter#102983) will allow us to cleanup this erroneous code and technical debt. Simplified a bit code (just a refactor) in the process, and fixes a bug that `flutter packages get` would generate internationalization files even if `flutter: generate: true` was not present in `pubspec.yaml` that was revealed as part of fixing this up. /cc @sigurdm.
engine-flutter-autoroll · Dec 17, 2024 · 27ef196 · 27ef196
1 parent eb926ea
commit 27ef196
Show file tree

Hide file tree

Showing 8 changed files with 85 additions and 71 deletions.
diff --git a/packages/flutter_tools/lib/src/commands/packages.dart b/packages/flutter_tools/lib/src/commands/packages.dart
@@ -266,46 +266,28 @@ class PackagesGetCommand extends FlutterCommand {
       rootProject = FlutterProject.fromDirectory(globals.fs.directory(target));
       _rootProject = rootProject;
 
-      if (rootProject.manifest.generateSyntheticPackage) {
-        final Environment environment = Environment(
-          artifacts: globals.artifacts!,
-          logger: globals.logger,
-          cacheDir: globals.cache.getRoot(),
-          engineVersion: globals.flutterVersion.engineRevision,
-          fileSystem: globals.fs,
-          flutterRootDir: globals.fs.directory(Cache.flutterRoot),
-          outputDir: globals.fs.directory(getBuildDirectory()),
-          processManager: globals.processManager,
-          platform: globals.platform,
-          usage: globals.flutterUsage,
-          analytics: analytics,
-          projectDir: rootProject.directory,
-          packageConfigPath: packageConfigPath(),
-          generateDartPluginRegistry: true,
-        );
-
-        await generateLocalizationsSyntheticPackage(
-          environment: environment,
-          buildSystem: globals.buildSystem,
-          buildTargets: globals.buildTargets,
-        );
-      } else if (rootProject.directory.childFile('l10n.yaml').existsSync()) {
-        final Environment environment = Environment(
-          artifacts: globals.artifacts!,
-          logger: globals.logger,
-          cacheDir: globals.cache.getRoot(),
-          engineVersion: globals.flutterVersion.engineRevision,
-          fileSystem: globals.fs,
-          flutterRootDir: globals.fs.directory(Cache.flutterRoot),
-          outputDir: globals.fs.directory(getBuildDirectory()),
-          processManager: globals.processManager,
-          platform: globals.platform,
-          usage: globals.flutterUsage,
-          analytics: analytics,
-          projectDir: rootProject.directory,
-          packageConfigPath: packageConfigPath(),
-          generateDartPluginRegistry: true,
-        );
+      final Environment environment = Environment(
+        artifacts: globals.artifacts!,
+        logger: globals.logger,
+        cacheDir: globals.cache.getRoot(),
+        engineVersion: globals.flutterVersion.engineRevision,
+        fileSystem: globals.fs,
+        flutterRootDir: globals.fs.directory(Cache.flutterRoot),
+        outputDir: globals.fs.directory(getBuildDirectory()),
+        processManager: globals.processManager,
+        platform: globals.platform,
+        usage: globals.flutterUsage,
+        analytics: analytics,
+        projectDir: rootProject.directory,
+        packageConfigPath: packageConfigPath(),
+        generateDartPluginRegistry: true,
+      );
+      if (rootProject.manifest.generateLocalizations && !await generateLocalizationsSyntheticPackage(
+        environment: environment,
+        buildSystem: globals.buildSystem,
+        buildTargets: globals.buildTargets,
+      )) {
+        // If localizations were enabled, but we are not using synthetic packages.
         final BuildResult result = await globals.buildSystem.build(
           const GenerateLocalizationsTarget(),
           environment,

diff --git a/packages/flutter_tools/lib/src/commands/widget_preview.dart b/packages/flutter_tools/lib/src/commands/widget_preview.dart
@@ -247,11 +247,9 @@ class WidgetPreviewStartCommand extends FlutterCommand
       outputMode: PubOutputMode.summaryOnly,
     );
 
-    if (rootProject.manifest.generateSyntheticPackage) {
-      maybeAddFlutterGenToPackageConfig(
-        rootProject: rootProject,
-      );
-    }
+    maybeAddFlutterGenToPackageConfig(
+      rootProject: rootProject,
+    );
   }
 
   /// Manually adds an entry for package:flutter_gen to the preview scaffold's
@@ -264,10 +262,19 @@ class WidgetPreviewStartCommand extends FlutterCommand
   /// isn't actually flutter_gen, which pub doesn't really like, and using the
   /// actual package name will break applications which import
   /// package:flutter_gen.
+  @visibleForTesting
   void maybeAddFlutterGenToPackageConfig({
     required FlutterProject rootProject,
   }) {
-    if (!rootProject.manifest.generateSyntheticPackage) {
+    // TODO(matanlurey): Remove this once flutter_gen is removed.
+    //
+    // This is actually incorrect logic; the presence of a `generate: true`
+    // does *NOT* mean that we need to add `flutter_gen` to the package config,
+    // and never did, but the name of the manifest field was labeled and
+    // described incorrectly.
+    //
+    // Tracking removal: https://github.com/flutter/flutter/issues/102983.
+    if (!rootProject.manifest.generateLocalizations) {
       return;
     }
     final FlutterProject widgetPreviewScaffoldProject =

diff --git a/packages/flutter_tools/lib/src/dart/generate_synthetic_packages.dart b/packages/flutter_tools/lib/src/dart/generate_synthetic_packages.dart
@@ -11,7 +11,13 @@ import '../build_system/build_system.dart';
 import '../build_system/build_targets.dart';
 import '../features.dart';
 
-Future<void> generateLocalizationsSyntheticPackage({
+/// Generates the `package:flutter_gen` synthetic package.
+///
+/// If the package has been configured *not* to use synthetic packages, this
+/// method is a NO-OP and returns `false`.
+///
+/// Returns `true` if the package was generated, or `false` it was not.
+Future<bool> generateLocalizationsSyntheticPackage({
   required Environment environment,
   required BuildSystem buildSystem,
   required BuildTargets buildTargets,
@@ -25,7 +31,7 @@ Future<void> generateLocalizationsSyntheticPackage({
   // root project directory, check to see if a synthetic package should
   // be generated for gen_l10n.
   if (!l10nYamlFile.existsSync()) {
-    return;
+    return false;
   }
 
   final YamlNode yamlNode = loadYamlNode(l10nYamlFile.readAsStringSync());
@@ -50,11 +56,11 @@ Future<void> generateLocalizationsSyntheticPackage({
     // synthetic-package is null.
     final bool? isSyntheticL10nPackage = value as bool?;
     if (isSyntheticL10nPackage == false || isSyntheticL10nPackage == null && featureFlags.isExplicitPackageDependenciesEnabled) {
-      return;
+      return false;
     }
   } else if (featureFlags.isExplicitPackageDependenciesEnabled) {
     // synthetic-packages: true was not set and it is no longer the default.
-    return;
+    return false;
   }
 
   if (featureFlags.isExplicitPackageDependenciesEnabled) {
@@ -88,4 +94,6 @@ Future<void> generateLocalizationsSyntheticPackage({
       '${result.exceptions.values.map<Object?>((ExceptionMeasurement e) => e.exception).join('\n\n')}',
     );
   }
+
+  return true;
 }
diff --git a/packages/flutter_tools/lib/src/dart/pub.dart b/packages/flutter_tools/lib/src/dart/pub.dart
@@ -798,10 +798,21 @@ class _DefaultPub implements Pub {
         _fileSystem,
       ));
 
-    if (!project.manifest.generateSyntheticPackage) {
+    // TODO(matanlurey): Remove this once flutter_gen is removed.
+    //
+    // This is actually incorrect logic; the presence of a `generate: true`
+    // does *NOT* mean that we need to add `flutter_gen` to the package config,
+    // and never did, but the name of the manifest field was labeled and
+    // described incorrectly.
+    //
+    // Tracking removal: https://github.com/flutter/flutter/issues/102983.
+    if (!project.manifest.generateLocalizations) {
       return;
     }
 
+    // TODO(matanlurey): Remove this once flutter_gen is removed.
+    //
+    // See https://github.com/dart-lang/pub/issues/4471.
     if (!_fileSystem.path.equals(packageConfigFile.parent.parent.path, project.directory.path)) {
       throwToolExit('`generate: true` is not supported within workspaces.');
     }

diff --git a/packages/flutter_tools/lib/src/flutter_manifest.dart b/packages/flutter_tools/lib/src/flutter_manifest.dart
@@ -2,6 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+/// @docImport 'localizations/gen_l10n.dart' as gen_l10n;
+library;
+
 import 'package:meta/meta.dart';
 import 'package:pub_semver/pub_semver.dart';
 import 'package:yaml/yaml.dart';
@@ -425,24 +428,19 @@ class FlutterManifest {
     return results;
   }
 
-  /// Whether a synthetic flutter_gen package should be generated.
+  /// Whether localization Dart files should be generated.
   ///
-  /// This can be provided to the [Pub] interface to inject a new entry
-  /// into the package_config.json file which points to `.dart_tool/flutter_gen`.
+  /// **NOTE**: This method was previously called `generateSyntheticPackage`,
+  /// which was incorrect; the presence of `generate: true` in `pubspec.yaml`
+  /// does _not_ imply a synthetic package (and never did); additional
+  /// introspection is required to determine whether a synthetic package is
+  /// required.
   ///
-  /// This allows generated source code to be imported using a package
-  /// alias.
-  late final bool generateSyntheticPackage = _computeGenerateSyntheticPackage();
-  bool _computeGenerateSyntheticPackage() {
-    if (!_flutterDescriptor.containsKey('generate')) {
-      return false;
-    }
-    final Object? value = _flutterDescriptor['generate'];
-    if (value is! bool) {
-      return false;
-    }
-    return value;
-  }
+  /// See also:
+  ///
+  ///   * [Deprecate and remove synthethic `package:flutter_gen`](https://github.com/flutter/flutter/issues/102983)
+  ///   * [gen_l10n.generateLocalizations]
+  late final bool generateLocalizations = _flutterDescriptor['generate'] == true;
 
   String? get defaultFlavor => _flutterDescriptor['default-flavor'] as String?;
 

diff --git a/packages/flutter_tools/lib/src/localizations/gen_l10n.dart b/packages/flutter_tools/lib/src/localizations/gen_l10n.dart
@@ -34,7 +34,7 @@ Future<LocalizationsGenerator> generateLocalizations({
     fileSystem: projectDir.fileSystem,
     logger: logger,
   );
-  if (options.syntheticPackage && (flutterManifest == null || !flutterManifest.generateSyntheticPackage)) {
+  if (options.syntheticPackage && (flutterManifest == null || !flutterManifest.generateLocalizations)) {
     throwToolExit(
       'Attempted to generate localizations code without having '
       'the flutter: generate flag turned on.'

diff --git a/packages/flutter_tools/test/commands.shard/permeable/packages_test.dart b/packages/flutter_tools/test/commands.shard/permeable/packages_test.dart
@@ -420,6 +420,14 @@ workspace:
         .childFile('app_en.arb')
         ..createSync(recursive: true)
         ..writeAsStringSync('{ "hello": "Hello world!" }');
+      String pubspecFileContent = projectDir.childFile('pubspec.yaml').readAsStringSync();
+      pubspecFileContent = pubspecFileContent.replaceFirst(RegExp(r'\nflutter\:'), '''
+flutter:
+  generate: true
+''');
+      projectDir
+        .childFile('pubspec.yaml')
+        .writeAsStringSync(pubspecFileContent);
       projectDir
         .childFile('l10n.yaml')
         .writeAsStringSync('synthetic-package: false');

diff --git a/packages/flutter_tools/test/general.shard/flutter_manifest_test.dart b/packages/flutter_tools/test/general.shard/flutter_manifest_test.dart
@@ -101,7 +101,7 @@ flutter:
       logger: logger,
     )!;
 
-    expect(flutterManifest.generateSyntheticPackage, true);
+    expect(flutterManifest.generateLocalizations, true);
   });
 
   testWithoutContext('FlutterManifest can parse invalid generate key', () async {
@@ -118,7 +118,7 @@ flutter:
       logger: logger,
     )!;
 
-    expect(flutterManifest.generateSyntheticPackage, false);
+    expect(flutterManifest.generateLocalizations, false);
   });
 
   testWithoutContext('FlutterManifest knows if generate is disabled', () async {
@@ -135,7 +135,7 @@ flutter:
       logger: logger,
     )!;
 
-    expect(flutterManifest.generateSyntheticPackage, false);
+    expect(flutterManifest.generateLocalizations, false);
   });
 
   testWithoutContext('FlutterManifest has one font family with one asset', () async {