Clean up build.yaml warnings. (#4207)

Author: davidmorgan

build_runner/CHANGELOG.md

@@ -1,5 +1,6 @@
 ## 2.8.1-wip
 
+- Improved warnings when an option is specified for an unknown builder.
 - Bug fix: require `args` 2.5.0.
 
 ## 2.8.0

build_runner/lib/src/bootstrap/build_script_generate.dart

@@ -4,7 +4,7 @@
 
 import 'dart:async';
 
-import 'package:build/build.dart' show AssetId, BuilderOptions;
+import 'package:build/build.dart';
 import 'package:build_config/build_config.dart';
 import 'package:code_builder/code_builder.dart';
 import 'package:dart_style/dart_style.dart';
@@ -110,37 +110,20 @@ Future<BuildScriptInfo> findBuildScriptOptions() async {
     }
   }
 
-  // TODO: Remove the dynamic calls here. We don't have a shared interface
-  // today and rely on dynamic calls instead which is bad.
-  bool isValidDefinition(dynamic definition) {
-    // Filter out builderDefinitions with relative imports that aren't
-    // from the root package, because they will never work.
+  bool isPackageImportOrForRoot(dynamic definition) {
     // ignore: avoid_dynamic_calls
     final import = definition.import as String;
-    if (import.startsWith('package:')) {
-      // Make sure package is known in packageGraph (when present),
-      // otherwise PackageNotFoundException will be thrown down the road
-      final pkg = AssetId.resolve(Uri.parse(import)).package;
-      if (packageGraph.allPackages.containsKey(pkg)) {
-        return true;
-      }
-      buildLog.warning(
-        'Could not load imported package "$pkg" for definition '
-        // ignore: avoid_dynamic_calls
-        '"${definition.key}".',
-      );
-      return false;
-    }
     // ignore: avoid_dynamic_calls
-    return definition.package == packageGraph.root.name;
+    final package = definition.package as String;
+    return import.startsWith('package:') || package == packageGraph.root.name;
   }
 
   final orderedConfigs = await Future.wait(
     orderedPackages.map(packageBuildConfig),
   );
   final builderDefinitions = orderedConfigs
       .expand((c) => c.builderDefinitions.values)
-      .where(isValidDefinition);
+      .where(isPackageImportOrForRoot);
 
   final rootBuildConfig = orderedConfigs.last;
   final orderedBuilders =
@@ -151,25 +134,7 @@ Future<BuildScriptInfo> findBuildScriptOptions() async {
 
   final postProcessBuilderDefinitions = orderedConfigs
       .expand((c) => c.postProcessBuilderDefinitions.values)
-      .where(isValidDefinition);
-
-  // Validate the builder keys in the global builder config, these should always
-  // refer to actual builders.
-  final allBuilderKeys = {
-    for (final definition in orderedBuilders) definition.key,
-  };
-  for (final globalBuilderConfig in rootBuildConfig.globalOptions.entries) {
-    void checkBuilderKey(String builderKey) {
-      if (allBuilderKeys.contains(builderKey)) return;
-      buildLog.warning(
-        'Invalid builder key `$builderKey` found in global_options config of '
-        'build.yaml. This configuration will have no effect.',
-      );
-    }
-
-    checkBuilderKey(globalBuilderConfig.key);
-    globalBuilderConfig.value.runsBefore.forEach(checkBuilderKey);
-  }
+      .where(isPackageImportOrForRoot);
 
   final applications = [
     for (final builder in orderedBuilders) _applyBuilder(builder),
@@ -320,14 +285,9 @@ Expression _rawStringList(List<String> strings) =>
 /// Returns the actual import to put in the generated script based on an import
 /// found in the build.yaml.
 String _buildScriptImport(String import) {
-  if (import.startsWith('package:')) {
-    return import;
-  } else if (import.startsWith('../') || import.startsWith('/')) {
-    buildLog.warning(
-      'The `../` import syntax in build.yaml is now deprecated, '
-      'instead do a normal relative import as if it was from the root of '
-      'the package. Found `$import` in your `build.yaml` file.',
-    );
+  if (import.startsWith('package:') ||
+      import.startsWith('../') ||
+      import.startsWith('/')) {
     return import;
   } else {
     return p.url.relative(import, from: p.url.dirname(scriptLocation));

build_runner/lib/src/build_plan/build_phases.dart

@@ -137,7 +137,7 @@ Future<BuildPhases> createBuildPhases(
   BuiltMap<String, BuiltMap<String, dynamic>> builderConfigOverrides,
   bool isReleaseMode,
 ) async {
-  validateBuilderConfig(
+  warnForUnknownBuilders(
     builderApplications,
     targetGraph.rootPackageConfig,
     builderConfigOverrides,
@@ -312,8 +312,8 @@ Map<String, List<BuilderApplication>> _applyWith(
 BuilderOptions _options(Map<String, dynamic>? options) =>
     options?.isEmpty ?? true ? BuilderOptions.empty : BuilderOptions(options!);
 
-/// Checks that all configuration is for valid builder keys.
-void validateBuilderConfig(
+/// Warns about configuration related to unknown builders.
+void warnForUnknownBuilders(
   Iterable<BuilderApplication> builders,
   BuildConfig rootPackageConfig,
   BuiltMap<String, BuiltMap<String, dynamic>> builderConfigOverrides,
@@ -322,27 +322,34 @@ void validateBuilderConfig(
   for (final key in builderConfigOverrides.keys) {
     if (!builderKeys.contains(key)) {
       buildLog.warning(
-        'Overriding configuration for `$key` but this is not a '
-        'known Builder',
+        'Ignoring options overrides for '
+        'unknown builder `$key`.',
       );
     }
   }
   for (final target in rootPackageConfig.buildTargets.values) {
     for (final key in target.builders.keys) {
       if (!builderKeys.contains(key)) {
         buildLog.warning(
-          'Configuring `$key` in target `${target.key}` but this '
-          'is not a known Builder.',
+          'Ignoring options for unknown builder `$key` '
+          'in target `${target.key}`.',
         );
       }
     }
   }
   for (final key in rootPackageConfig.globalOptions.keys) {
     if (!builderKeys.contains(key)) {
-      buildLog.warning(
-        'Configuring `$key` in global options but this is not a '
-        'known Builder.',
-      );
+      buildLog.warning('Ignoring `global_options` for unknown builder `$key`.');
+    }
+  }
+  for (final value in rootPackageConfig.globalOptions.values) {
+    for (final key in value.runsBefore) {
+      if (!builderKeys.contains(key)) {
+        buildLog.warning(
+          'Ignoring `runs_before` in `global_options` '
+          'referencing unknown builder `$key`.',
+        );
+      }
     }
   }
 }

build_runner/test/bootstrap/build_script_generate_test.dart

@@ -29,29 +29,6 @@ void main() {
     });
 
     group('of builder imports', () {
-      test('warn about deprecated ../ style imports', () async {
-        await d.dir('a', [
-          d.file('build.yaml', '''
-builders:
-  fake:
-    import: "../../../tool/builder.dart"
-    builder_factories: ["myFactory"]
-    build_extensions: {"foo": ["bar"]}
-'''),
-        ]).create();
-
-        final result = await runPub(
-          'a',
-          'run',
-          args: ['build_runner', 'build'],
-        );
-        expect(result.stderr, isEmpty);
-        expect(
-          result.stdout,
-          contains('The `../` import syntax in build.yaml is now deprecated'),
-        );
-      });
-
       test('support package relative imports', () async {
         await d.dir('a', [
           d.file('build.yaml', '''
@@ -69,13 +46,6 @@ builders:
           args: ['build_runner', 'build'],
         );
         expect(result.stderr, isEmpty);
-        expect(
-          result.stdout,
-          isNot(
-            contains('The `../` import syntax in build.yaml is now deprecated'),
-          ),
-        );
-
         await d.dir('a', [
           d.dir('.dart_tool', [
             d.dir('build', [
@@ -108,31 +78,6 @@ builders:
         expect(result.stderr, isEmpty);
         expect(result.stdout, contains('could not be parsed'));
       });
-
-      test('warn when import not present in packageGraph', () async {
-        await d.dir('a', [
-          d.file('build.yaml', '''
-builders:
-  fake:
-    import: "package:unknown_package/import.dart"
-    builder_factories: ["myFactory"]
-    build_extensions: {"foo": ["bar"]}
-'''),
-        ]).create();
-        final result = await runPub(
-          'a',
-          'run',
-          args: ['build_runner', 'build'],
-        );
-        expect(result.stderr, isEmpty);
-        expect(
-          result.stdout,
-          contains(
-            'Could not load imported package "unknown_package" '
-            'for definition "a:fake".',
-          ),
-        );
-      });
     });
 
     test('checks builder keys in global_options', () async {
@@ -150,13 +95,10 @@ global_options:
       expect(
         result.stdout,
         allOf(
+          contains('Ignoring `global_options` for unknown builder `a:a`.'),
           contains(
-            'Invalid builder key `a:a` found in global_options config of '
-            'build.yaml. This configuration will have no effect.',
-          ),
-          contains(
-            'Invalid builder key `b:b` found in global_options config of '
-            'build.yaml. This configuration will have no effect.',
+            'Ignoring `runs_before` in `global_options` '
+            'referencing unknown builder `b:b`.',
           ),
         ),
       );

build_runner/test/integration_tests/build_integration_test.dart

@@ -579,7 +579,12 @@ targets:
 
       final result = await runBuild();
 
-      expect(result, contains('not a known Builder'));
+      expect(
+        result,
+        contains(
+          'Ignoring options for unknown builder `bad:builder` in target `a:a`.',
+        ),
+      );
     });
 
     test('warns on invalid builder key in global options', () async {
@@ -608,7 +613,12 @@ global_options:
 
       final result = await runBuild();
 
-      expect(result, contains('not a known Builder'));
+      expect(
+        result,
+        contains(
+          'Ignoring `global_options` for unknown builder `bad:builder`.',
+        ),
+      );
     });
 
     test('warns on invalid builder key --define', () async {
@@ -632,7 +642,10 @@ global_options:
 
       final result = await runBuild(extraArgs: ['--define=bad:key=foo=bar']);
 
-      expect(result, contains('not a known Builder'));
+      expect(
+        result,
+        contains('Ignoring options overrides for unknown builder `bad:key`.'),
+      );
     });
   });