Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
mkustermann committed Oct 4, 2024
1 parent de2a89f commit 4b3f2d0
Show file tree
Hide file tree
Showing 13 changed files with 66 additions and 46 deletions.
21 changes: 14 additions & 7 deletions pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,22 @@ import 'build_planner.dart';

typedef DependencyMetadata = Map<String, Metadata>;

typedef HookValidator = Future<ValidationErrors> Function(
typedef _HookValidator = Future<ValidationErrors> Function(
HookConfig config, HookOutputImpl output);

// A callback that validates the output of a `hook/link.dart` invocation is
// valid (it may valid asset-type specific information).
typedef BuildValidator = Future<ValidationErrors> Function(
BuildConfig config, BuildOutput outup);

// A callback that validates the output of a `hook/link.dart` invocation is
// valid (it may valid asset-type specific information).
typedef LinkValidator = Future<ValidationErrors> Function(
LinkConfig config, LinkOutput output);

// A callback that validates assets emitted across all packages are valid / can
// be used together (it may valid asset-type specific information - e.g. that
// there are no classes in shared library filenames).
typedef ApplicationAssetValidator = Future<ValidationErrors> Function(
List<EncodedAsset> assets);

Expand Down Expand Up @@ -165,7 +172,7 @@ class NativeAssetsBuildRunner {
required Target target,
required Uri workingDirectory,
required BuildMode buildMode,
required HookValidator validator,
required _HookValidator validator,
required ApplicationAssetValidator applicationAssetValidator,
CCompilerConfig? cCompilerConfig,
IOSSdk? targetIOSSdk,
Expand Down Expand Up @@ -466,7 +473,7 @@ class NativeAssetsBuildRunner {
Future<_PackageBuildRecord> _runHookForPackageCached(
Hook hook,
HookConfigImpl config,
HookValidator validator,
_HookValidator validator,
Uri packageConfigUri,
Uri workingDirectory,
bool includeParentEnvironment,
Expand Down Expand Up @@ -535,7 +542,7 @@ class NativeAssetsBuildRunner {
Future<_PackageBuildRecord> _runHookForPackage(
Hook hook,
HookConfigImpl config,
HookValidator validator,
_HookValidator validator,
Uri packageConfigUri,
Uri workingDirectory,
bool includeParentEnvironment,
Expand Down Expand Up @@ -595,7 +602,7 @@ ${result.stdout}
final output = HookOutputImpl.readFromFile(file: config.outputFile) ??
HookOutputImpl();

final errors = await validate(config, output, packageLayout, validator);
final errors = await _validate(config, output, packageLayout, validator);
success &= errors.isEmpty;
if (errors.isNotEmpty) {
logger.severe('package:${config.packageName}` has invalid output.');
Expand Down Expand Up @@ -804,11 +811,11 @@ ${compileResult.stdout}
};
}

Future<ValidationErrors> validate(
Future<ValidationErrors> _validate(
HookConfigImpl config,
HookOutputImpl output,
PackageLayout packageLayout,
HookValidator validator,
_HookValidator validator,
) async {
final errors = config is BuildConfigImpl
? await validateBuildOutput(config, output)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ void main(List<String> args) async {
.toFilePath(windows: false)
.substring(config.packageRoot.toFilePath(windows: false).length);

output.addEncodedAsset(
output.dataAssets.add(
DataAsset(
package: packageName,
name: name,
file: dataAsset.uri,
).encode(),
),
linkInPackage: config.linkingEnabled ? packageName : null,
);
// TODO(https://github.com/dart-lang/native/issues/1208): Report
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,8 @@ void main(List<String> args) async {
final usedAssets = (usages.instancesOf(multiplyIdentifier) ?? []).map((e) =>
(e.instanceConstant.fields.values.first as StringConstant).value);

output.addEncodedAssets(config.encodedAssets
.where((e) => e.type == DataAsset.type)
.where(
(asset) => usedAssets.contains(DataAsset.fromEncoded(asset).name)));
output.dataAssets.addAll(config.dataAssets.all
.where((dataAsset) => usedAssets.contains(dataAsset.name)));
});
}

Expand Down
12 changes: 6 additions & 6 deletions pkgs/native_assets_cli/lib/native_assets_cli.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,19 @@ export 'src/code_assets/code_asset.dart'
show
BuildOutputCodeAssets,
CodeAsset,
CodeAssetsBuildOutputExt,
CodeAssetsLinkConfigExt,
CodeAssetsLinkOutputExt,
CodeAssetsBuildOutput,
CodeAssetsLinkConfig,
CodeAssetsLinkOutput,
LinkConfigCodeAssets,
LinkOutputCodeAssets,
OSLibraryNaming;
export 'src/data_assets/data_asset.dart'
show
BuildOutputDataAssets,
DataAsset,
DataAssetsBuildOutputExt,
DataAssetsLinkConfigExt,
DataAssetsLinkOutputExt,
DataAssetsBuildOutput,
DataAssetsLinkConfig,
DataAssetsLinkOutput,
LinkConfigDataAssets,
LinkOutputDataAssets;
export 'src/encoded_asset.dart' show EncodedAsset;
Expand Down
2 changes: 1 addition & 1 deletion pkgs/native_assets_cli/lib/src/api/build.dart
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ import 'build_output.dart';
/// ]);
/// }
///
/// output.addEncodedAsset(
/// output.codeAssets.add(
/// // TODO: Change to DataAsset once the Dart/Flutter SDK can consume it.
/// CodeAsset(
/// package: packageName,
Expand Down
24 changes: 24 additions & 0 deletions pkgs/native_assets_cli/lib/src/api/build_output.dart
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,18 @@ abstract final class BuildOutput {
/// If the [linkInPackage] argument is specified, the asset will not be
/// bundled during the build step, but sent as input to the link hook of the
/// specified package, where it can be further processed and possibly bundled.
///
/// Note to hook writers. Prefer using the `.add` method on the extension for
/// the specific asset type being added:
///
/// ```dart
/// main(List<String> arguments) async {
/// await build((config, output) {
/// output.codeAssets.add(CodeAsset(...));
/// output.dataAssets.add(DataAsset(...));
/// });
/// }
/// ```
void addEncodedAsset(EncodedAsset asset, {String? linkInPackage});

/// Adds [EncodedAsset]s produced by this build or dry run.
Expand All @@ -124,6 +136,18 @@ abstract final class BuildOutput {
/// bundled during the build step, but sent as input to the link hook of the
/// specified package, where they can be further processed and possibly
/// bundled.
///
/// Note to hook writers. Prefer using the `.addAll` method on the extension
/// for the specific asset type being added:
///
/// ```dart
/// main(List<String> arguments) async {
/// await build((config, output) {
/// output.codeAssets.addAll([CodeAsset(...), ...]);
/// output.dataAssets.addAll([DataAsset(...), ...]);
/// });
/// }
/// ```
void addEncodedAssets(Iterable<EncodedAsset> assets, {String? linkInPackage});

/// Adds file used by this build.
Expand Down
15 changes: 4 additions & 11 deletions pkgs/native_assets_cli/lib/src/api/test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ Future<void> testBuildHook({

await mainMethod(['--config=${buildConfigUri.toFilePath()}']);

final hookOutput = await _readOutput(buildConfig);
final hookOutput = await _readOutput(buildConfig) as BuildOutput;

check(buildConfig, hookOutput);

Expand All @@ -81,19 +81,12 @@ Future<void> testBuildHook({
for (final asset in allEncodedAssets) {
expect(buildConfig.supportedAssetTypes, contains(asset.type));
}
final dataAssets = allEncodedAssets
.where((e) => e.type == DataAsset.type)
.map(DataAsset.fromEncoded)
.toList();
for (final asset in dataAssets) {

for (final asset in hookOutput.dataAssets.all) {
final file = File.fromUri(asset.file);
expect(await file.exists(), true);
}
final codeAssets = allEncodedAssets
.where((e) => e.type == CodeAsset.type)
.map(CodeAsset.fromEncoded)
.toList();
for (final asset in codeAssets) {
for (final asset in hookOutput.codeAssets.all) {
if (asset.file != null) {
final file = File.fromUri(asset.file!);
expect(await file.exists(), true);
Expand Down
6 changes: 3 additions & 3 deletions pkgs/native_assets_cli/lib/src/code_assets/code_asset.dart
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ final class CodeAsset {
}

/// Build output extension for code assets.
extension CodeAssetsBuildOutputExt on BuildOutput {
extension CodeAssetsBuildOutput on BuildOutput {
BuildOutputCodeAssets get codeAssets => BuildOutputCodeAssets(this);
}

Expand All @@ -215,7 +215,7 @@ class BuildOutputCodeAssets {
}

/// Link output extension for code assets.
extension CodeAssetsLinkConfigExt on LinkConfig {
extension CodeAssetsLinkConfig on LinkConfig {
LinkConfigCodeAssets get codeAssets => LinkConfigCodeAssets(this);
}

Expand All @@ -230,7 +230,7 @@ class LinkConfigCodeAssets {
}

/// Link output extension for code assets.
extension CodeAssetsLinkOutputExt on LinkOutput {
extension CodeAssetsLinkOutput on LinkOutput {
LinkOutputCodeAssets get codeAssets => LinkOutputCodeAssets(this);
}

Expand Down
6 changes: 3 additions & 3 deletions pkgs/native_assets_cli/lib/src/data_assets/data_asset.dart
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ final class DataAsset {
}

/// Build output extension for data assets.
extension DataAssetsBuildOutputExt on BuildOutput {
extension DataAssetsBuildOutput on BuildOutput {
BuildOutputDataAssets get dataAssets => BuildOutputDataAssets(this);
}

Expand All @@ -117,7 +117,7 @@ class BuildOutputDataAssets {
}

/// Link output extension for data assets.
extension DataAssetsLinkConfigExt on LinkConfig {
extension DataAssetsLinkConfig on LinkConfig {
LinkConfigDataAssets get dataAssets => LinkConfigDataAssets(this);
}

Expand All @@ -132,7 +132,7 @@ class LinkConfigDataAssets {
}

/// Link output extension for data assets.
extension DataAssetsLinkOutputExt on LinkOutput {
extension DataAssetsLinkOutput on LinkOutput {
LinkOutputDataAssets get dataAssets => LinkOutputDataAssets(this);
}

Expand Down
3 changes: 2 additions & 1 deletion pkgs/native_assets_cli/lib/src/encoded_asset.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'package:collection/collection.dart';

import 'json_utils.dart';
import 'utils/map.dart';

/// An encoding of a particular asset type.
final class EncodedAsset {
Expand All @@ -27,7 +28,7 @@ final class EncodedAsset {
Map<String, Object?> toJson() => {
for (final key in encoding.keys) key: encoding[key],
_typeKey: type,
};
}..sortOnKey();

@override
String toString() => 'EncodedAsset($type, $encoding)';
Expand Down
5 changes: 2 additions & 3 deletions pkgs/native_assets_cli/lib/src/utils/map.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

extension MapSorting<K extends Comparable<K>, V extends Object> on Map<K, V> {
extension MapSorting<K extends Comparable<K>, V extends Object?> on Map<K, V> {
Map<K, V> sortOnKey() {
final result = <K, V>{};
final keysSorted = keys.toList()..sort();
for (final key in keysSorted) {
final value = this[key]!;
result[key] = value;
result[key] = this[key] as V;
}
return result;
}
Expand Down
6 changes: 2 additions & 4 deletions pkgs/native_assets_cli/test/example/local_asset_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,12 @@ void main() async {

final buildOutputUri = outputDirectory.resolve('build_output.json');
final buildOutput = HookOutputImpl.fromJsonString(
await File.fromUri(buildOutputUri).readAsString());
await File.fromUri(buildOutputUri).readAsString()) as BuildOutput;
final assets = buildOutput.encodedAssets;
final dependencies = buildOutput.dependencies;
if (dryRun) {
final codeAsset = buildOutput.codeAssets.all.first;
expect(assets.length, greaterThanOrEqualTo(1));
final first = assets.first;
expect(first.type, CodeAsset.type);
final codeAsset = CodeAsset.fromEncoded(first);
expect(await File.fromUri(codeAsset.file!).exists(), false);
expect(dependencies, <Uri>[]);
} else {
Expand Down
2 changes: 1 addition & 1 deletion pkgs/native_assets_cli/test/model/link_config_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ void main() async {
file: Uri.file('not there'),
architecture: Architecture.riscv64,
).encode(),
].cast<EncodedAsset>();
];

setUp(() async {
tempUri = (await Directory.systemTemp.createTemp()).uri;
Expand Down

0 comments on commit 4b3f2d0

Please sign in to comment.