Avoid exposing CCompiler.*Key constants, address other comments

dart-lang · Sep 27, 2024 · e480b33 · e480b33
1 parent c213f66
commit e480b33
Show file tree

Hide file tree

Showing 8 changed files with 75 additions and 103 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
@@ -160,21 +160,17 @@ class NativeAssetsBuildRunner {
     // Specifically for running our tests on Dart CI with the test runner, we
     // recognize specific variables to setup the C Compiler configuration.
     if (cCompilerConfig == null) {
-      String? unparseKey(String key) =>
-          'DART_HOOK_TESTING_${key.replaceAll('.', '__').toUpperCase()}';
-
       final env = Platform.environment;
-      String? lookup(String key) => env[unparseKey(key)];
-
-      final cc = lookup(CCompilerConfig.ccConfigKeyFull);
-      final ar = lookup(CCompilerConfig.arConfigKeyFull);
-      final ld = lookup(CCompilerConfig.ldConfigKeyFull);
-      final envScript = lookup(CCompilerConfig.envScriptConfigKeyFull);
-      final envScriptArgs = lookup(CCompilerConfig.envScriptArgsConfigKeyFull)
-          ?.split(' ')
-          .map((arg) => arg.trim())
-          .where((arg) => arg.isNotEmpty)
-          .toList();
+      final cc = env['DART_HOOK_TESTING_C_COMPILER__CC'];
+      final ar = env['DART_HOOK_TESTING_C_COMPILER__AR'];
+      final ld = env['DART_HOOK_TESTING_C_COMPILER__LD'];
+      final envScript = env['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT'];
+      final envScriptArgs =
+          env['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT_ARGUMENTS']
+              ?.split(' ')
+              .map((arg) => arg.trim())
+              .where((arg) => arg.isNotEmpty)
+              .toList();
       final hasEnvScriptArgs =
           envScriptArgs != null && envScriptArgs.isNotEmpty;
 

diff --git a/pkgs/native_assets_builder/test/build_runner/build_runner_run_in_isolation_test.dart b/pkgs/native_assets_builder/test/build_runner/build_runner_run_in_isolation_test.dart
@@ -13,17 +13,17 @@ import 'helpers.dart';
 const Timeout longTimeout = Timeout(Duration(minutes: 5));
 
 void main() async {
-  String unparseKey(String key) =>
-      'DART_HOOK_TESTING_${key.replaceAll('.', '__').toUpperCase()}';
-
-  final arKey = unparseKey(CCompilerConfig.arConfigKeyFull);
-  final ccKey = unparseKey(CCompilerConfig.ccConfigKeyFull);
-  final ldKey = unparseKey(CCompilerConfig.ldConfigKeyFull);
-  final envScriptKey = unparseKey(CCompilerConfig.envScriptConfigKeyFull);
-  final envScriptArgsKey =
-      unparseKey(CCompilerConfig.envScriptArgsConfigKeyFull);
-
-  final cc = Platform.environment[ccKey]?.fileUri;
+  final env = Platform.environment;
+  final cc = env['DART_HOOK_TESTING_C_COMPILER__CC'];
+  final ar = env['DART_HOOK_TESTING_C_COMPILER__AR'];
+  final ld = env['DART_HOOK_TESTING_C_COMPILER__LD'];
+  final envScript = env['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT'];
+  final envScriptArgs =
+      env['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT_ARGUMENTS']
+          ?.split(' ')
+          .map((arg) => arg.trim())
+          .where((arg) => arg.isNotEmpty)
+          .toList();
 
   if (cc == null) {
     // We don't set any compiler paths on the GitHub CI.
@@ -45,8 +45,6 @@ void main() async {
 
       await runPubGet(workingDirectory: packageUri, logger: logger);
 
-      printOnFailure(
-          'Platform.environment[ccKey]: ${Platform.environment[ccKey]}');
       printOnFailure('cc: $cc');
 
       final result = await build(
@@ -55,11 +53,11 @@ void main() async {
         dartExecutable,
         // Manually pass in a compiler.
         cCompilerConfig: CCompilerConfig(
-          archiver: Platform.environment[arKey]?.fileUri,
-          compiler: cc,
-          envScript: Platform.environment[envScriptKey]?.fileUri,
-          envScriptArgs: Platform.environment[envScriptArgsKey]?.split(' '),
-          linker: Platform.environment[ldKey]?.fileUri,
+          archiver: ar?.fileUri,
+          compiler: cc.fileUri,
+          envScript: envScript?.fileUri,
+          envScriptArgs: envScriptArgs,
+          linker: ld?.fileUri,
         ),
         // Prevent any other environment variables.
         includeParentEnvironment: false,

diff --git a/pkgs/native_assets_builder/test/helpers.dart b/pkgs/native_assets_builder/test/helpers.dart
@@ -123,42 +123,36 @@ final pkgNativeAssetsBuilderUri = findPackageRoot('native_assets_builder');
 
 final testDataUri = pkgNativeAssetsBuilderUri.resolve('test_data/');
 
-String unparseKey(String key) =>
-    'DART_HOOK_TESTING_${key.replaceAll('.', '__').toUpperCase()}';
-
 /// Archiver provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _ar = Platform
-    .environment[unparseKey(internal.CCompilerConfig.arConfigKeyFull)]
-    ?.asFileUri();
+final Uri? _ar =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__AR']?.asFileUri();
 
 /// Compiler provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _cc = Platform
-    .environment[unparseKey(internal.CCompilerConfig.ccConfigKeyFull)]
-    ?.asFileUri();
+final Uri? _cc =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__CC']?.asFileUri();
 
 /// Linker provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _ld = Platform
-    .environment[unparseKey(internal.CCompilerConfig.ldConfigKeyFull)]
-    ?.asFileUri();
+final Uri? _ld =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__LD']?.asFileUri();
 
 /// Path to script that sets environment variables for [_cc], [_ld], and [_ar].
 ///
 /// Provided on Dart CI.
 final Uri? _envScript = Platform
-    .environment[unparseKey(internal.CCompilerConfig.envScriptConfigKeyFull)]
+    .environment['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT']
     ?.asFileUri();
 
 /// Arguments for [_envScript] provided by environment.
 ///
 /// Provided on Dart CI.
-final List<String>? _envScriptArgs = Platform.environment[
-        unparseKey(internal.CCompilerConfig.envScriptArgsConfigKeyFull)]
+final List<String>? _envScriptArgs = Platform
+    .environment['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT_ARGUMENTS']
     ?.split(' ');
 
 /// Configuration for the native toolchain.

diff --git a/pkgs/native_assets_cli/lib/src/c_compiler_config.dart b/pkgs/native_assets_cli/lib/src/c_compiler_config.dart
@@ -49,34 +49,16 @@ final class CCompilerConfig {
     );
   }
 
-  // TODO(https://github.com/dart-lang/native/issues/1599): The main reason
-  // these keys are exposed is due to usage on Dart CI testing infrastructure.
-  // If our infrastructure supplies the native toolchains via passing those down
-  // to the `dart build/run` via overrides we should no longer need to expose
-  // these here.
-  static const configKey = 'c_compiler';
-  static const arConfigKey = 'ar';
-  static const arConfigKeyFull = '$configKey.$arConfigKey';
-  static const ccConfigKey = 'cc';
-  static const ccConfigKeyFull = '$configKey.$ccConfigKey';
-  static const ldConfigKey = 'ld';
-  static const ldConfigKeyFull = '$configKey.$ldConfigKey';
-  static const envScriptConfigKey = 'env_script';
-  static const envScriptConfigKeyFull = '$configKey.$envScriptConfigKey';
-  static const envScriptArgsConfigKey = 'env_script_arguments';
-  static const envScriptArgsConfigKeyFull =
-      '$configKey.$envScriptArgsConfigKey';
-
   /// The json representation of this [CCompilerConfig].
   ///
   /// The returned json can be used in [CCompilerConfig.fromJson] to
   /// obtain a [CCompilerConfig] again.
   Map<String, Object> toJson() => {
-        if (archiver != null) arConfigKey: archiver!.toFilePath(),
-        if (compiler != null) ccConfigKey: compiler!.toFilePath(),
-        if (linker != null) ldConfigKey: linker!.toFilePath(),
-        if (envScript != null) envScriptConfigKey: envScript!.toFilePath(),
-        if (envScriptArgs != null) envScriptArgsConfigKey: envScriptArgs!,
+        if (archiver != null) _arConfigKey: archiver!.toFilePath(),
+        if (compiler != null) _ccConfigKey: compiler!.toFilePath(),
+        if (linker != null) _ldConfigKey: linker!.toFilePath(),
+        if (envScript != null) _envScriptConfigKey: envScript!.toFilePath(),
+        if (envScriptArgs != null) _envScriptArgsConfigKey: envScriptArgs!,
       }.sortOnKey();
 
   @override
@@ -106,24 +88,30 @@ final class CCompilerConfig {
 }
 
 Uri? _parseArchiver(Map<String, Object?> config) => config.optionalPath(
-      CCompilerConfig.arConfigKey,
+      _arConfigKey,
       mustExist: true,
     );
 
 Uri? _parseCompiler(Map<String, Object?> config) => config.optionalPath(
-      CCompilerConfig.ccConfigKey,
+      _ccConfigKey,
       mustExist: true,
     );
 
 Uri? _parseLinker(Map<String, Object?> config) => config.optionalPath(
-      CCompilerConfig.ldConfigKey,
+      _ldConfigKey,
       mustExist: true,
     );
 
 Uri? _parseEnvScript(Map<String, Object?> config, Uri? compiler) =>
     (compiler != null && compiler.toFilePath().endsWith('cl.exe'))
-        ? config.path(CCompilerConfig.envScriptConfigKey, mustExist: true)
+        ? config.path(_envScriptConfigKey, mustExist: true)
         : null;
 
 List<String>? _parseEnvScriptArgs(Map<String, Object?> config) =>
-    config.optionalStringList(CCompilerConfig.envScriptArgsConfigKey);
+    config.optionalStringList(_envScriptArgsConfigKey);
+
+const _arConfigKey = 'ar';
+const _ccConfigKey = 'cc';
+const _ldConfigKey = 'ld';
+const _envScriptConfigKey = 'env_script';
+const _envScriptArgsConfigKey = 'env_script_arguments';
diff --git a/pkgs/native_assets_cli/lib/src/model/hook_config.dart b/pkgs/native_assets_cli/lib/src/model/hook_config.dart
@@ -197,7 +197,7 @@ abstract class HookConfigImpl implements HookConfig {
           targetMacOSVersionConfigKey: targetMacOSVersion!,
         if (targetAndroidNdkApi != null)
           targetAndroidNdkApiConfigKey: targetAndroidNdkApi!,
-        if (cCompilerJson.isNotEmpty) CCompilerConfig.configKey: cCompilerJson,
+        if (cCompilerJson.isNotEmpty) _compilerConfigKey: cCompilerJson,
       },
       _linkModePreferenceConfigKey: linkModePreference.toString(),
     }.sortOnKey();
@@ -373,11 +373,11 @@ abstract class HookConfigImpl implements HookConfig {
   static CCompilerConfig parseCCompiler(
       Map<String, Object?> config, bool dryRun) {
     if (dryRun) {
-      _throwIfNotNullInDryRun<int>(config, CCompilerConfig.configKey);
+      _throwIfNotNullInDryRun<int>(config, _compilerConfigKey);
     }
 
     final cCompilerJson =
-        config.getOptional<Map<String, Object?>>(CCompilerConfig.configKey);
+        config.getOptional<Map<String, Object?>>(_compilerConfigKey);
     if (cCompilerJson == null) return CCompilerConfig();
 
     return CCompilerConfig.fromJson(cCompilerJson);
@@ -541,6 +541,7 @@ can _only_ depend on OS.''');
   static Version latestVersion = Version(1, 5, 0);
 }
 
+const String _compilerConfigKey = 'c_compiler';
 const String _buildModeConfigKey = 'build_mode';
 const String _targetOSConfigKey = 'target_os';
 const String _targetArchitectureKey = 'target_architecture';

diff --git a/pkgs/native_assets_cli/test/helpers.dart b/pkgs/native_assets_cli/test/helpers.dart
@@ -80,42 +80,36 @@ extension on Uri {
   String get name => pathSegments.where((e) => e != '').last;
 }
 
-String unparseKey(String key) =>
-    'DART_HOOK_TESTING_${key.replaceAll('.', '__').toUpperCase()}';
-
 /// Archiver provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _ar = Platform
-    .environment[unparseKey(internal.CCompilerConfig.arConfigKeyFull)]
-    ?.asFileUri();
+final Uri? _ar =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__AR']?.asFileUri();
 
 /// Compiler provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _cc = Platform
-    .environment[unparseKey(internal.CCompilerConfig.ccConfigKeyFull)]
-    ?.asFileUri();
+final Uri? _cc =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__CC']?.asFileUri();
 
 /// Linker provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _ld = Platform
-    .environment[unparseKey(internal.CCompilerConfig.ldConfigKeyFull)]
-    ?.asFileUri();
+final Uri? _ld =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__LD']?.asFileUri();
 
 /// Path to script that sets environment variables for [_cc], [_ld], and [_ar].
 ///
 /// Provided on Dart CI.
 final Uri? _envScript = Platform
-    .environment[unparseKey(internal.CCompilerConfig.envScriptConfigKeyFull)]
+    .environment['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT']
     ?.asFileUri();
 
 /// Arguments for [_envScript] provided by environment.
 ///
 /// Provided on Dart CI.
-final List<String>? _envScriptArgs = Platform.environment[
-        unparseKey(internal.CCompilerConfig.envScriptArgsConfigKeyFull)]
+final List<String>? _envScriptArgs = Platform
+    .environment['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT_ARGUMENTS']
     ?.split(' ');
 
 /// Configuration for the native toolchain.

diff --git a/pkgs/native_toolchain_c/CHANGELOG.md b/pkgs/native_toolchain_c/CHANGELOG.md
@@ -1,7 +1,7 @@
 ## 0.5.5-wip
 
 - Address analyzer info diagnostic about multi-line if requiring a block body.
-- Use unified classes instead of two `{OS,...}` and `{OS,,...}Impl`
+- Bump `package:native_assets_cli` to `0.9.0`.
 
 ## 0.5.4
 

diff --git a/pkgs/native_toolchain_c/test/helpers.dart b/pkgs/native_toolchain_c/test/helpers.dart
@@ -118,35 +118,36 @@ extension on Uri {
   String get name => pathSegments.where((e) => e != '').last;
 }
 
-String unparseKey(String key) =>
-    'DART_HOOK_TESTING_${key.replaceAll('.', '__').toUpperCase()}';
-
 /// Archiver provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _ar = Platform.environment[unparseKey('c_compiler.ar')]?.asFileUri();
+final Uri? _ar =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__AR']?.asFileUri();
 
 /// Compiler provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _cc = Platform.environment[unparseKey('c_compiler.cc')]?.asFileUri();
+final Uri? _cc =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__CC']?.asFileUri();
 
 /// Linker provided by the environment.
 ///
 /// Provided on Dart CI.
-final Uri? _ld = Platform.environment[unparseKey('c_compiler.ld')]?.asFileUri();
+final Uri? _ld =
+    Platform.environment['DART_HOOK_TESTING_C_COMPILER__LD']?.asFileUri();
 
 /// Path to script that sets environment variables for [_cc], [_ld], and [_ar].
 ///
 /// Provided on Dart CI.
-final Uri? _envScript =
-    Platform.environment[unparseKey('c_compiler.env_script')]?.asFileUri();
+final Uri? _envScript = Platform
+    .environment['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT']
+    ?.asFileUri();
 
 /// Arguments for [_envScript] provided by environment.
 ///
 /// Provided on Dart CI.
 final List<String>? _envScriptArgs = Platform
-    .environment[unparseKey('c_compiler.env_script_arguments')]
+    .environment['DART_HOOK_TESTING_C_COMPILER__ENV_SCRIPT_ARGUMENTS']
     ?.split(' ');
 
 /// Configuration for the native toolchain.