[native_assets_cli] Move BuildMode to [native_toolchain_c] (#1800)

The first in a series of refactorings to address: * #1738 We align `BuildMode` with `OptimizationLevel`, it's configured by the hook author in the build hook. Likely, every package should ship with max optimization level and build mode set to release. Only while developing a package (when it is the root package or path-depsed in) would the build mode be set to debug. This will require a manual roll into dartdev and flutter tools due to the removal of `BuildMode` from the `native_assets_builder` API. The native assets builder will still emit the field in the JSON, until we can bump the `native_assets_cli` SDK constraint to 3.7 stable. Then only people with older versions of the `native_assets_cli` package but a newer SDK break.
dart-lang · Dec 10, 2024 · 00aae9e · 00aae9e
1 parent d37df9c
commit 00aae9e
Show file tree

Hide file tree

Showing 42 changed files with 122 additions and 195 deletions.
diff --git a/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart b/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
@@ -93,7 +93,6 @@ class NativeAssetsBuildRunner {
     required BuildValidator buildValidator,
     required ApplicationAssetValidator applicationAssetValidator,
     required OS targetOS,
-    required BuildMode buildMode,
     required Uri workingDirectory,
     PackageLayout? packageLayout,
     String? runPackageName,
@@ -124,7 +123,6 @@ class NativeAssetsBuildRunner {
         ..setupHookConfig(
           targetOS: targetOS,
           buildAssetTypes: buildAssetTypes,
-          buildMode: buildMode,
           packageName: package.name,
           packageRoot: packageLayout.packageRoot(package.name),
         )
@@ -200,7 +198,6 @@ class NativeAssetsBuildRunner {
     required LinkConfigValidator configValidator,
     required LinkValidator linkValidator,
     required OS targetOS,
-    required BuildMode buildMode,
     required Uri workingDirectory,
     required ApplicationAssetValidator applicationAssetValidator,
     PackageLayout? packageLayout,
@@ -225,7 +222,6 @@ class NativeAssetsBuildRunner {
         ..setupHookConfig(
           targetOS: targetOS,
           buildAssetTypes: buildAssetTypes,
-          buildMode: buildMode,
           packageName: package.name,
           packageRoot: packageLayout.packageRoot(package.name),
         );

diff --git a/pkgs/native_assets_builder/test/build_runner/build_runner_reusability_test.dart b/pkgs/native_assets_builder/test/build_runner/build_runner_reusability_test.dart
@@ -36,7 +36,6 @@ void main() async {
       await buildRunner.build(
         configCreator: configCreator,
         targetOS: OS.current,
-        buildMode: BuildMode.release,
         workingDirectory: packageUri,
         linkingEnabled: false,
         buildAssetTypes: [],
@@ -46,7 +45,6 @@ void main() async {
       );
       await buildRunner.build(
         configCreator: configCreator,
-        buildMode: BuildMode.release,
         targetOS: OS.current,
         workingDirectory: packageUri,
         linkingEnabled: false,

diff --git a/pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart b/pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart
@@ -21,7 +21,6 @@ void main(List<String> args) async {
     dartExecutable: dartExecutable,
   ).build(
     configCreator: BuildConfigBuilder.new,
-    buildMode: BuildMode.release,
     targetOS: target.os,
     workingDirectory: packageUri,
     linkingEnabled: false,

diff --git a/pkgs/native_assets_builder/test/build_runner/concurrency_test_helper.dart b/pkgs/native_assets_builder/test/build_runner/concurrency_test_helper.dart
@@ -32,7 +32,6 @@ void main(List<String> args) async {
         cCompilerConfig: dartCICompilerConfig,
         targetMacOSVersion: OS.current == OS.macOS ? defaultMacOSVersion : null,
       ),
-    buildMode: BuildMode.release,
     targetOS: OS.current,
     workingDirectory: packageUri,
     linkingEnabled: false,

diff --git a/pkgs/native_assets_builder/test/build_runner/helpers.dart b/pkgs/native_assets_builder/test/build_runner/helpers.dart
@@ -71,7 +71,6 @@ Future<BuildResult?> build(
         return configBuilder;
       },
       configValidator: configValidator,
-      buildMode: BuildMode.release,
       targetOS: targetOS,
       workingDirectory: packageUri,
       packageLayout: packageLayout,
@@ -137,7 +136,6 @@ Future<LinkResult?> link(
         return configBuilder;
       },
       configValidator: configValidator,
-      buildMode: BuildMode.release,
       targetOS: target?.os ?? OS.current,
       workingDirectory: packageUri,
       packageLayout: packageLayout,
@@ -195,7 +193,6 @@ Future<(BuildResult?, LinkResult?)> buildAndLink(
             targetAndroidNdkApi: targetAndroidNdkApi,
           ),
         configValidator: buildConfigValidator,
-        buildMode: BuildMode.release,
         targetOS: target?.os ?? OS.current,
         workingDirectory: packageUri,
         packageLayout: packageLayout,
@@ -228,7 +225,6 @@ Future<(BuildResult?, LinkResult?)> buildAndLink(
             targetAndroidNdkApi: targetAndroidNdkApi,
           ),
         configValidator: linkConfigValidator,
-        buildMode: BuildMode.release,
         targetOS: target?.os ?? OS.current,
         workingDirectory: packageUri,
         packageLayout: packageLayout,

diff --git a/pkgs/native_assets_builder/test/test_data/native_dynamic_linking_test.dart b/pkgs/native_assets_builder/test/test_data/native_dynamic_linking_test.dart
@@ -32,15 +32,16 @@ void main() async {
 
       final configBuilder = BuildConfigBuilder()
         ..setupHookConfig(
-            packageName: name,
-            packageRoot: testPackageUri,
-            targetOS: OS.current,
-            buildAssetTypes: [CodeAsset.type],
-            buildMode: BuildMode.debug)
+          packageName: name,
+          packageRoot: testPackageUri,
+          targetOS: OS.current,
+          buildAssetTypes: [CodeAsset.type],
+        )
         ..setupBuildConfig(dryRun: false, linkingEnabled: false)
         ..setupBuildRunConfig(
-            outputDirectory: outputDirectory,
-            outputDirectoryShared: outputDirectoryShared)
+          outputDirectory: outputDirectory,
+          outputDirectoryShared: outputDirectoryShared,
+        )
         ..setupCodeConfig(
           targetArchitecture: Architecture.current,
           linkModePreference: LinkModePreference.dynamic,

diff --git a/pkgs/native_assets_builder/test/test_data/transformer_test.dart b/pkgs/native_assets_builder/test/test_data/transformer_test.dart
@@ -47,15 +47,16 @@ void main() async {
       Future<void> runBuild(Architecture architecture) async {
         final configBuilder = BuildConfigBuilder()
           ..setupHookConfig(
-              packageName: packageName,
-              packageRoot: packageUri,
-              targetOS: OS.current,
-              buildAssetTypes: [DataAsset.type],
-              buildMode: BuildMode.debug)
+            packageName: packageName,
+            packageRoot: packageUri,
+            targetOS: OS.current,
+            buildAssetTypes: [DataAsset.type],
+          )
           ..setupBuildConfig(dryRun: false, linkingEnabled: false)
           ..setupBuildRunConfig(
-              outputDirectory: outputDirectory,
-              outputDirectoryShared: outputDirectoryShared)
+            outputDirectory: outputDirectory,
+            outputDirectoryShared: outputDirectoryShared,
+          )
           ..setupCodeConfig(
             targetArchitecture: architecture,
             linkModePreference: LinkModePreference.dynamic,

diff --git a/pkgs/native_assets_builder/test_data/native_dynamic_linking/hook/build.dart b/pkgs/native_assets_builder/test_data/native_dynamic_linking/hook/build.dart
@@ -19,6 +19,7 @@ void main(List<String> args) async {
         sources: [
           'src/debug.c',
         ],
+        buildMode: BuildMode.debug,
       ),
       CBuilder.library(
         name: 'math',
@@ -27,6 +28,7 @@ void main(List<String> args) async {
           'src/math.c',
         ],
         libraries: ['debug'],
+        buildMode: BuildMode.debug,
       ),
       CBuilder.library(
         name: 'add',
@@ -35,6 +37,7 @@ void main(List<String> args) async {
           'src/add.c',
         ],
         libraries: ['math'],
+        buildMode: BuildMode.debug,
       )
     ];
 

diff --git a/pkgs/native_assets_cli/CHANGELOG.md b/pkgs/native_assets_cli/CHANGELOG.md
@@ -5,6 +5,10 @@
   missing. This enables (1) code to run if an asset is missing but that code is
   not invoked at runtime, and (2) doing fallback implementations in Dart if an
   asset is missing.
+- **Breaking change** Move `BuildMode` to `package:native_toolchain_c`. This way
+  it can be controlled in the build hook together with the `OptimizationLevel`.
+  Most likely, every package should ship with `release`. `BuildMode.debug`
+  should only be used while developing the package locally.
 - Update pubspec.yaml of examples to use 0.9.0 of `package:native_assets_cli`.
 - Consolidate [CodeAsset] specific things into `lib/src/code_assets/*`
 

diff --git a/pkgs/native_assets_cli/lib/code_assets.dart b/pkgs/native_assets_cli/lib/code_assets.dart
@@ -10,7 +10,6 @@ export 'native_assets_cli.dart'
         EncodedAsset,
         EncodedAssetBuildOutputBuilder,
         EncodedAssetLinkOutputBuilder;
-export 'src/build_mode.dart' show BuildMode;
 export 'src/code_assets/architecture.dart' show Architecture;
 export 'src/code_assets/c_compiler_config.dart' show CCompilerConfig;
 export 'src/code_assets/code_asset.dart' show CodeAsset, OSLibraryNaming;

diff --git a/pkgs/native_assets_cli/lib/native_assets_cli.dart b/pkgs/native_assets_cli/lib/native_assets_cli.dart
@@ -10,7 +10,6 @@ export 'src/api/build.dart' show build;
 export 'src/api/builder.dart' show Builder;
 export 'src/api/link.dart' show link;
 export 'src/api/linker.dart' show Linker;
-export 'src/build_mode.dart' show BuildMode;
 export 'src/config.dart'
     show
         BuildConfig,

diff --git a/pkgs/native_assets_cli/lib/src/code_assets/testing.dart b/pkgs/native_assets_cli/lib/src/code_assets/testing.dart
@@ -16,7 +16,6 @@ Future<void> testCodeBuildHook({
   // ignore: inference_failure_on_function_return_type
   required Function(List<String> arguments) mainMethod,
   required FutureOr<void> Function(BuildConfig, BuildOutput) check,
-  BuildMode? buildMode,
   Architecture? targetArchitecture,
   OS? targetOS,
   IOSSdk? targetIOSSdk,
@@ -46,7 +45,6 @@ Future<void> testCodeBuildHook({
       expect(await validateCodeAssetBuildOutput(config, output), isEmpty);
       await check(config, output);
     },
-    buildMode: buildMode,
     targetOS: targetOS,
     buildAssetTypes: buildAssetTypes,
     linkingEnabled: linkingEnabled,

diff --git a/pkgs/native_assets_cli/lib/src/config.dart b/pkgs/native_assets_cli/lib/src/config.dart
@@ -9,7 +9,6 @@ import 'package:crypto/crypto.dart' show sha256;
 import 'package:pub_semver/pub_semver.dart';
 
 import 'api/deprecation_messages.dart';
-import 'build_mode.dart';
 import 'code_assets/architecture.dart';
 import 'encoded_asset.dart';
 import 'json_utils.dart';
@@ -65,13 +64,6 @@ sealed class HookConfig {
   /// The operating system being compiled for.
   final OS targetOS;
 
-  /// The [BuildMode] that the code should be compiled in.
-  ///
-  /// Currently [BuildMode.debug] and [BuildMode.release] are the only modes.
-  ///
-  /// Not available during a dry run.
-  final BuildMode? _buildMode;
-
   /// The asset types that the invoker of this hook supports.
   final List<String> buildAssetTypes;
 
@@ -91,18 +83,7 @@ sealed class HookConfig {
         targetOS = OS.fromString(json.string(_targetOSConfigKey)),
         buildAssetTypes = json.optionalStringList(_buildAssetTypesKey) ??
             json.optionalStringList(_supportedAssetTypesKey) ??
-            const [],
-        _buildMode = switch (json.optionalString(_buildModeConfigKey)) {
-          String value => BuildMode.fromString(value),
-          null => null,
-        };
-
-  BuildMode get buildMode {
-    if (_buildMode == null) {
-      throw StateError('Build mode should not be accessed in dry-run mode.');
-    }
-    return _buildMode;
-  }
+            const [];
 
   @override
   String toString() => const JsonEncoder.withIndent('  ').convert(json);
@@ -118,16 +99,12 @@ sealed class HookConfigBuilder {
     required String packageName,
     required OS targetOS,
     required List<String> buildAssetTypes,
-    required BuildMode? buildMode,
   }) {
     json[_packageNameConfigKey] = packageName;
     json[_packageRootConfigKey] = packageRoot.toFilePath();
     json[_targetOSConfigKey] = targetOS.toString();
     json[_buildAssetTypesKey] = buildAssetTypes;
     json[_supportedAssetTypesKey] = buildAssetTypes;
-    if (buildMode != null) {
-      json[_buildModeConfigKey] = buildMode.toString();
-    }
   }
 
   /// Constructs a checksum for a [BuildConfig].
@@ -159,7 +136,8 @@ sealed class HookConfigBuilder {
 }
 
 const _targetOSConfigKey = 'target_os';
-const _buildModeConfigKey = 'build_mode';
+// TODO: Bump min-SDK constraint to 3.7 and remove once stable.
+const _buildModeConfigKeyDeprecated = 'build_mode';
 const _metadataConfigKey = 'metadata';
 const _outDirConfigKey = 'out_dir';
 const _outDirSharedConfigKey = 'out_dir_shared';
@@ -207,6 +185,10 @@ final class BuildConfigBuilder extends HookConfigBuilder {
     json[_dependencyMetadataKey] = {
       for (final key in metadata.keys) key: metadata[key]!.toJson(),
     };
+    // TODO: Bump min-SDK constraint to 3.7 and remove once stable.
+    if (!dryRun) {
+      json[_buildModeConfigKeyDeprecated] = 'release';
+    }
   }
 
   void setupBuildRunConfig({
@@ -237,6 +219,8 @@ final class LinkConfigBuilder extends HookConfigBuilder {
     required List<EncodedAsset> assets,
   }) {
     json[_assetsKey] = [for (final asset in assets) asset.toJson()];
+    // TODO: Bump min-SDK constraint to 3.7 and remove once stable.
+    json[_buildModeConfigKeyDeprecated] = 'release';
   }
 
   void setupLinkRunConfig({

diff --git a/pkgs/native_assets_cli/lib/src/model/build_config_CHANGELOG.md b/pkgs/native_assets_cli/lib/src/model/build_config_CHANGELOG.md
@@ -7,6 +7,11 @@
   Compatibility with older SDKs: Create a sibling dir next to the output
   directory. This does not facilitate caching, but should not break the hook.
   Compatibility with older hooks: These will not read this field.
+- `BuildConfig.buildMode` is removed. Instead it is specified by hook writers
+  in the `CBuilder` constructors.
+  Compatibility with older SDKs: The new hooks will not read the field.
+  Compatibility with older hooks: The field is now always passed as 'release'
+  until we can bump the SDK constraint in `package:native_assets_cli`.
 
 ## 1.5.0
 

diff --git a/pkgs/native_assets_cli/lib/test.dart b/pkgs/native_assets_cli/lib/test.dart
@@ -22,7 +22,6 @@ Future<void> testBuildHook({
   required FutureOr<void> Function(List<String> arguments) mainMethod,
   required FutureOr<void> Function(BuildConfig config, BuildOutput output)
       check,
-  BuildMode? buildMode,
   OS? targetOS,
   List<String>? buildAssetTypes,
   bool? linkingEnabled,
@@ -44,7 +43,6 @@ Future<void> testBuildHook({
           packageName: _readPackageNameFromPubspec(),
           targetOS: targetOS ?? OS.current,
           buildAssetTypes: buildAssetTypes ?? [],
-          buildMode: buildMode ?? BuildMode.release,
         )
         ..setupBuildConfig(
           dryRun: false,

diff --git a/pkgs/native_assets_cli/test/api/build_test.dart b/pkgs/native_assets_cli/test/api/build_test.dart
@@ -36,7 +36,6 @@ void main() async {
         packageName: packageName,
         targetOS: OS.iOS,
         buildAssetTypes: ['foo'],
-        buildMode: BuildMode.release,
       )
       ..setupBuildConfig(
         dryRun: false,

diff --git a/pkgs/native_assets_cli/test/build_config_test.dart b/pkgs/native_assets_cli/test/build_config_test.dart
@@ -41,7 +41,6 @@ void main() async {
         packageName: packageName,
         packageRoot: packageRootUri,
         targetOS: OS.android,
-        buildMode: BuildMode.release,
         buildAssetTypes: ['my-asset-type'],
       )
       ..setupBuildConfig(
@@ -87,7 +86,6 @@ void main() async {
     expect(config.packageName, packageName);
     expect(config.packageRoot, packageRootUri);
     expect(config.targetOS, OS.android);
-    expect(config.buildMode, BuildMode.release);
     expect(config.buildAssetTypes, ['my-asset-type']);
 
     expect(config.linkingEnabled, false);
@@ -101,7 +99,6 @@ void main() async {
         packageName: packageName,
         packageRoot: packageRootUri,
         targetOS: OS.android,
-        buildMode: null, // not available in dry run
         buildAssetTypes: ['my-asset-type'],
       )
       ..setupBuildConfig(
@@ -138,7 +135,6 @@ void main() async {
     expect(config.packageRoot, packageRootUri);
     expect(config.targetOS, OS.android);
     expect(config.buildAssetTypes, ['my-asset-type']);
-    expect(() => config.buildMode, throwsStateError);
 
     expect(config.linkingEnabled, true);
     expect(config.dryRun, true);
@@ -210,7 +206,6 @@ void main() async {
           'package_root': packageRootUri.toFilePath(),
           'target_os': 'android',
           'linking_enabled': true,
-          'build_mode': BuildMode.release.name,
           'build_asset_types': ['my-asset-type'],
           'dependency_metadata': {
             'bar': {'key': 'value'},