Address first round of comments

pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart

@@ -988,3 +988,43 @@ extension on DateTime {
 extension on Uri {
   Uri get parent => File(toFilePath()).parent.uri;
 }
+
+extension OSArchitectures on OS {
+  Set<Architecture> get architectures => _osTargets[this]!;
+}
+
+const _osTargets = {
+  OS.android: {
+    Architecture.arm,
+    Architecture.arm64,
+    Architecture.ia32,
+    Architecture.x64,
+    Architecture.riscv64,
+  },
+  OS.fuchsia: {
+    Architecture.arm64,
+    Architecture.x64,
+  },
+  OS.iOS: {
+    Architecture.arm,
+    Architecture.arm64,
+    Architecture.x64,
+  },
+  OS.linux: {
+    Architecture.arm,
+    Architecture.arm64,
+    Architecture.ia32,
+    Architecture.riscv32,
+    Architecture.riscv64,
+    Architecture.x64,
+  },
+  OS.macOS: {
+    Architecture.arm64,
+    Architecture.x64,
+  },
+  OS.windows: {
+    Architecture.arm64,
+    Architecture.ia32,
+    Architecture.x64,
+  },
+};

pkgs/native_assets_cli/CHANGELOG.md

@@ -14,7 +14,8 @@
   has presence over other, etc)
 - Use `DART_HOOK_TESTING` prefix for environment variables used for testing on
   Dart CI
-- Use unified classes instead of two `{OS,...}` and `{OS,,...}Impl`
+- **Breaking change** Use unified classes instead of two `{OS,...}` and
+  move methods (e.g. from `OS`) to extensions (e.g. `OSLibraryNaming`)
 
 ## 0.8.0
 

pkgs/native_assets_cli/lib/native_assets_cli.dart

@@ -7,7 +7,7 @@
 library native_assets_cli;
 
 export 'src/api/asset.dart'
-    show Asset, DataAsset, NativeCodeAsset, OSLibraryNamingExt;
+    show Asset, DataAsset, NativeCodeAsset, OSLibraryNaming;
 export 'src/api/build.dart' show build;
 export 'src/api/build_config.dart' show BuildConfig, CCompilerConfig;
 export 'src/api/build_output.dart' show BuildOutput, LinkOutput;
@@ -16,7 +16,7 @@ export 'src/api/hook_config.dart' show HookConfig;
 export 'src/api/link.dart' show link;
 export 'src/api/link_config.dart' show LinkConfig;
 export 'src/api/linker.dart' show Linker;
-export 'src/architecture.dart' show Architecture, OSArchitecturesExt;
+export 'src/architecture.dart' show Architecture;
 export 'src/build_mode.dart' show BuildMode;
 export 'src/ios_sdk.dart' show IOSSdk;
 export 'src/link_mode.dart'

pkgs/native_assets_cli/lib/src/api/native_code_asset.dart

@@ -98,7 +98,7 @@ abstract final class NativeCodeAsset implements Asset {
   static const String type = 'native_code';
 }
 
-extension OSLibraryNamingExt on OS {
+extension OSLibraryNaming on OS {
   /// The default dynamic library file name on this os.
   String dylibFileName(String name) {
     final prefix = _dylibPrefix[this]!;

pkgs/native_assets_cli/lib/src/architecture.dart

@@ -5,15 +5,14 @@
 import 'dart:ffi' show Abi;
 import 'dart:io';
 
-import 'os.dart';
-
 /// A hardware architecture which the Dart VM can run on.
 final class Architecture {
-  /// This architecture as used in [Platform.version].
-  final String dartPlatform;
+  /// The name of this architecture.
+  final String name;
 
-  const Architecture._(this.dartPlatform);
+  const Architecture._(this.name);
 
+  /// The [Architecture] corresponding to the given [abi].
   factory Architecture.fromAbi(Abi abi) => _abiToArch[abi]!;
 
   /// The [arm](https://en.wikipedia.org/wiki/ARM_architecture_family)
@@ -69,58 +68,25 @@ final class Architecture {
     Abi.windowsX64: Architecture.x64,
   };
 
+  /// The name of this [Architecture].
+  ///
+  /// This returns a stable string that can be used to construct an
+  /// [Architecture] via [Architecture.fromString].
   @override
-  String toString() => dartPlatform;
+  String toString() => name;
 
   static final Map<String, Architecture> _architectureByName = {
-    for (var architecture in values) architecture.dartPlatform: architecture
+    for (var architecture in values) architecture.name: architecture
   };
 
-  factory Architecture.fromString(String target) =>
-      _architectureByName[target]!;
+  /// Creates an [Architecture] from the given [name].
+  ///
+  /// The name can be obtained from [Architecture.name] or
+  /// [Architecture.toString].
+  factory Architecture.fromString(String name) => _architectureByName[name]!;
 
   /// The current [Architecture].
   ///
   /// Read from the [Platform.version] string.
   static final Architecture current = _abiToArch[Abi.current()]!;
 }
-
-extension OSArchitecturesExt on OS {
-  Iterable<Architecture> get architectures => _osTargets[this]!;
-}
-
-const _osTargets = {
-  OS.android: {
-    Architecture.arm,
-    Architecture.arm64,
-    Architecture.ia32,
-    Architecture.x64,
-    Architecture.riscv64,
-  },
-  OS.fuchsia: {
-    Architecture.arm64,
-    Architecture.x64,
-  },
-  OS.iOS: {
-    Architecture.arm,
-    Architecture.arm64,
-    Architecture.x64,
-  },
-  OS.linux: {
-    Architecture.arm,
-    Architecture.arm64,
-    Architecture.ia32,
-    Architecture.riscv32,
-    Architecture.riscv64,
-    Architecture.x64,
-  },
-  OS.macOS: {
-    Architecture.arm64,
-    Architecture.x64,
-  },
-  OS.windows: {
-    Architecture.arm64,
-    Architecture.ia32,
-    Architecture.x64,
-  },
-};

pkgs/native_assets_cli/lib/src/build_mode.dart

@@ -36,9 +36,17 @@ final class BuildMode {
     release,
   ];
 
+  /// The name of this [BuildMode].
+  ///
+  /// This returns a stable string that can be used to construct a
+  /// [BuildMode] via [BuildMode.fromString].
   factory BuildMode.fromString(String target) =>
       values.firstWhere((e) => e.name == target);
 
+  /// The name of this [BuildMode].
+  ///
+  /// This returns a stable string that can be used to construct a
+  /// [BuildMode] via [BuildMode.fromString].
   @override
   String toString() => name;
 }

pkgs/native_assets_cli/lib/src/ios_sdk.dart

@@ -4,9 +4,9 @@
 
 /// For an iOS target, a build is either done for the device or the simulator.
 final class IOSSdk {
-  final String xcodebuildSdk;
+  final String type;
 
-  const IOSSdk._(this.xcodebuildSdk);
+  const IOSSdk._(this.type);
 
   /// The iphoneos SDK in Xcode.
   ///
@@ -26,9 +26,17 @@ final class IOSSdk {
     iPhoneSimulator,
   ];
 
-  factory IOSSdk.fromString(String target) =>
-      values.firstWhere((e) => e.xcodebuildSdk == target);
+  /// The type of this [IOSSdk].
+  ///
+  /// This returns a stable string that can be used to construct a
+  /// [IOSSdk] via [IOSSdk.fromString].
+  factory IOSSdk.fromString(String type) =>
+      values.firstWhere((e) => e.type == type);
 
+  /// The type of this [IOSSdk].
+  ///
+  /// This returns a stable string that can be used to construct a
+  /// [IOSSdk] via [IOSSdk.fromString].
   @override
-  String toString() => xcodebuildSdk;
+  String toString() => type;
 }

pkgs/native_assets_cli/lib/src/link_mode.dart

@@ -17,9 +17,12 @@ import 'api/build_config.dart';
 ///   * [DynamicLoadingSystem]
 ///
 /// See the documentation on the above classes.
-sealed class LinkMode {
-  const LinkMode();
+abstract final class LinkMode {
+  const LinkMode._();
 
+  /// Constructs a [LinkMode] from the given [json].
+  ///
+  /// The json is expected to be valid encoding obtained via [LinkMode.toJson].
   factory LinkMode.fromJson(Map<String, Object?> json) {
     final type = json['type'];
     return switch (type) {
@@ -33,6 +36,10 @@ sealed class LinkMode {
     };
   }
 
+  /// The json representation of this [LinkMode].
+  ///
+  /// The returned json is stable and can be used in [LinkMode.fromJson] to
+  /// obtain a [LinkMode] again.
   Map<String, Object?> toJson() => switch (this) {
         StaticLinking() => {'type': 'static'},
         LookupInProcess() => {'type': 'dynamic_loading_process'},
@@ -42,6 +49,7 @@ sealed class LinkMode {
             'type': 'dynamic_loading_system',
             'uri': system.uri.toFilePath(),
           },
+        _ => throw UnimplementedError('The link mode "$this" is not known'),
       };
 }
 
@@ -54,7 +62,9 @@ sealed class LinkMode {
 /// Note: Dynamic loading is not equal to dynamic linking. Dynamic linking
 /// would have to run the linker at compile-time, which is currently not
 /// supported in the Dart and Flutter SDK.
-sealed class DynamicLoading extends LinkMode {}
+abstract final class DynamicLoading extends LinkMode {
+  DynamicLoading._() : super._();
+}
 
 /// The dynamic library is bundled by Dart/Flutter at build time.
 ///
@@ -69,7 +79,7 @@ sealed class DynamicLoading extends LinkMode {}
 /// instead of a the full path. The file does not have to exist during a dry
 /// run.
 final class DynamicLoadingBundled extends DynamicLoading {
-  DynamicLoadingBundled._();
+  DynamicLoadingBundled._() : super._();
 
   static final DynamicLoadingBundled _singleton = DynamicLoadingBundled._();
 
@@ -86,9 +96,10 @@ final class DynamicLoadingBundled extends DynamicLoading {
 /// At runtime, the dynamic library will be loaded and the symbols will be
 /// looked up in this dynamic library.
 final class DynamicLoadingSystem extends DynamicLoading {
+  /// The [Uri] of the
   final Uri uri;
 
-  DynamicLoadingSystem(this.uri);
+  DynamicLoadingSystem(this.uri) : super._();
 
   static const _typeValue = 'dynamic_loading_system';
 
@@ -106,7 +117,7 @@ final class DynamicLoadingSystem extends DynamicLoading {
 /// The native code is loaded in the process and symbols are available through
 /// `DynamicLibrary.process()`.
 final class LookupInProcess extends DynamicLoading {
-  LookupInProcess._();
+  LookupInProcess._() : super._();
 
   static final LookupInProcess _singleton = LookupInProcess._();
 
@@ -119,7 +130,7 @@ final class LookupInProcess extends DynamicLoading {
 /// The native code is embedded in executable and symbols are available through
 /// `DynamicLibrary.executable()`.
 final class LookupInExecutable extends DynamicLoading {
-  LookupInExecutable._();
+  LookupInExecutable._() : super._();
 
   static final LookupInExecutable _singleton = LookupInExecutable._();
 
@@ -137,7 +148,7 @@ final class LookupInExecutable extends DynamicLoading {
 /// Not yet supported in the Dart and Flutter SDK.
 // TODO(https://github.com/dart-lang/sdk/issues/49418): Support static linking.
 final class StaticLinking extends LinkMode {
-  const StaticLinking._();
+  const StaticLinking._() : super._();
   factory StaticLinking() => _singleton;
 
   static const StaticLinking _singleton = StaticLinking._();

pkgs/native_assets_cli/lib/src/os.dart

@@ -6,10 +6,10 @@ import 'dart:io';
 
 /// An operating system the Dart VM runs on.
 final class OS {
-  /// This OS as used in [Platform.version]
-  final String dartPlatform;
+  /// The name of this operating system.
+  final String name;
 
-  const OS._(this.dartPlatform);
+  const OS._(this.name);
 
   /// The
   /// [Android](https://en.wikipedia.org/wiki/Android_%28operating_system%29)
@@ -53,15 +53,22 @@ final class OS {
 
   static const String configKey = 'target_os';
 
+  /// The name of this [OS].
+  ///
+  /// This returns a stable string that can be used to construct a
+  /// [OS] via [OS.fromString].
   @override
-  String toString() => dartPlatform;
+  String toString() => name;
 
   /// Mapping from strings as used in [OS.toString] to
   /// [OS]s.
   static final Map<String, OS> _stringToOS =
       Map.fromEntries(OS.values.map((os) => MapEntry(os.toString(), os)));
 
-  factory OS.fromString(String target) => _stringToOS[target]!;
+  /// Creates a [OS] from the given [name].
+  ///
+  /// The name can be obtained from [OS.name] or [OS.toString].
+  factory OS.fromString(String name) => _stringToOS[name]!;
 
   /// The current [OS].
   ///

pkgs/native_assets_cli/lib/src/target.dart

@@ -134,15 +134,11 @@ final class Target implements Comparable<Target> {
         Abi.windowsX64: OS.windows,
       }[abi]!;
 
-  String get _architectureString => architecture.dartPlatform;
-
-  String get _osString => os.dartPlatform;
-
   @override
   String toString() => dartVMToString();
 
   /// As used in [Platform.version].
-  String dartVMToString() => '${_osString}_$_architectureString';
+  String dartVMToString() => '${os.name}_${architecture.name}';
 
   /// Compares `this` to [other].
   ///

pkgs/native_assets_cli/test/model/target_test.dart

@@ -73,14 +73,4 @@ void main() {
       )),
     );
   });
-
-  test('OS.architectures', () {
-    expect(OS.android.architectures, [
-      Architecture.arm,
-      Architecture.arm64,
-      Architecture.ia32,
-      Architecture.x64,
-      Architecture.riscv64,
-    ]);
-  });
 }

pkgs/native_toolchain_c/lib/src/cbuilder/ctool.dart

@@ -16,8 +16,8 @@ abstract class CTool {
   /// Name of the library or executable to linkg.
   ///
   /// The filename will be decided by [LinkConfig.targetOS] and
-  /// [OSLibraryNamingExt.libraryFileName] or
-  /// [OSLibraryNamingExt.executableFileName].
+  /// [OSLibraryNaming.libraryFileName] or
+  /// [OSLibraryNaming.executableFileName].
   ///
   /// File will be placed in [LinkConfig.outputDirectory].
   final String name;