Skip to content

Commit

Permalink
Allow custom asset types in the build system (#1623)
Browse files Browse the repository at this point in the history
This is the first PR that will increase the flexibility of the Dart
Build system as well as makes it more layered. The main points being
addressed in this PR are:

* A bundling tool has to specify which asset types it supports. So we
  make `supportedAssetTypes` required everywhere.
  => Across various layers in the code base we make this required
  => We remove a baked-in `[CodeAsset.type]` fallback in various places
  (Making this explicit increases size of LOCs in this CL due to the
   many tests)

* The core building infrastructure in `pkg/native_assets_builder/lib/*`
  no longer knows anything about `CodeAsset` or `DataAsset`. Instead it
  only knows about `EncodedAsset` which represents the type of an asset
  and it's json encoding.
  => The core still verifies certain protocol consistency (e.g. that a
  hook only emitted asset types that the config allows)
  => The higher levels pass the `supportedAssetTypes`.
  => The higher levels now also pass `buildValidator` / `linkValidator`
  that validate per-asset related things (e.g. the id of a data asset to
  start with the package name) for a given package.
  => The higher levels now also pass a `applicationAssetValidator` that
  validates consistency of assets across all packages (e.g. uniqueness
  of dylib filenames across all dependencies)
  => This centralizes the logic about per-asset-type information in the
  bundling tool (i.e. user of `package:native_assets_builder`).
  => Bundling tools now have to expand `CodeAsset`s in dry-run to all
  architectures.

* The bundling tool (e.g. `flutter build`) doesn't have to implement
  validation logic itself, instead it will support certain asset types
  by importing the "bundling logic" for that asset types (which includes
  - for now - validation logic).

* All the validation code now simply returns `ValidationError` which is
  a list of errors. If the list is empty then there were no errors.
  => This removes redundancy between a `success` boolean and the list of errors
     (it was successsfull iff there were no errors)
  => This simplifies code that aggregates errors from different sources.

* Moves the `fromJson` / `toJson` towards be purely read/write a json schema
  and are not responsible for anything else.
  => The validation routines are responsible for validating semantics.

* The hook writer API now changes to work on per-asset-type specific
  extension methods.
  For example what used to be
  ```
    output.addAsset(DataAsset(...));
  ```
  is now
  ```
    output.dataAssets.add(DataAsset(...));
  ```

  The `BuildOutput.encodedAsset` getter is (with this PR) temporarily visible
  to hook writers, which could use it like this:
  ```
    output.encodedAssets.add(DataAsset(...).encode());
  ```
  but a following refactoring that increases the flexibility of the
  protocol more will also get rid of the user-visible `encodedAsset`
  getter (if we choose so). So this is only temporarily exposed.

* `hook/link.dart` scripts have to mark any input files it's output
  depends on in the `LinkOutput.addDependency`. If the set of inputs is
  the same but one input's file changed then a `hook/link.dart`
  - doesn't have to rerun if it just drops all assets or outputs them
    unchanged
  - has to re-run if it copies a input, changes it's contents, merge
    with other inputs, etc
  => A `hook/link.dart` should - just like a `hook/build.dart` add
    dependencies if the inputs are used to produce the output
  => Before this was somewhat hard-coded into the
  `package:native_assets_builder`.
  • Loading branch information
mkustermann authored Oct 7, 2024
1 parent 0bf27dc commit d8a84ea
Show file tree
Hide file tree
Showing 116 changed files with 1,473 additions and 1,035 deletions.
5 changes: 4 additions & 1 deletion pkgs/native_assets_builder/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
dart-lang/native repository to make it clear those are not intended to be used
by end-users.
- Remove link-dry-run concept as it's unused by Flutter Tools & Dart SDK
- Bump `native_assets_cli` to `0.9.0`
- Bump `native_assets_cli` to `0.9.0`.
- **Breaking change**: Remove asset-type specific logic from `package:native_assets_builder`.
Bundling tools have to now supply `supportedAssetTypes` and corresponding
validation routines.

## 0.8.3

Expand Down
146 changes: 72 additions & 74 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,6 +23,25 @@ import 'build_planner.dart';

typedef DependencyMetadata = Map<String, Metadata>;

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);

/// The programmatic API to be used by Dart launchers to invoke native builds.
///
/// These methods are invoked by launchers such as dartdev (for `dart run`)
Expand Down Expand Up @@ -58,6 +77,8 @@ class NativeAssetsBuildRunner {
required Target target,
required Uri workingDirectory,
required BuildMode buildMode,
required BuildValidator buildValidator,
required ApplicationAssetValidator applicationAssetValidator,
CCompilerConfig? cCompilerConfig,
IOSSdk? targetIOSSdk,
int? targetIOSVersion,
Expand All @@ -66,10 +87,14 @@ class NativeAssetsBuildRunner {
required bool includeParentEnvironment,
PackageLayout? packageLayout,
String? runPackageName,
Iterable<String>? supportedAssetTypes,
required Iterable<String> supportedAssetTypes,
required bool linkingEnabled,
}) async =>
_run(
validator: (HookConfig config, HookOutputImpl output) =>
buildValidator(config as BuildConfig, output as BuildOutput),
applicationAssetValidator: (assets) async =>
linkingEnabled ? [] : applicationAssetValidator(assets),
hook: Hook.build,
linkModePreference: linkModePreference,
target: target,
Expand Down Expand Up @@ -103,6 +128,8 @@ class NativeAssetsBuildRunner {
required Target target,
required Uri workingDirectory,
required BuildMode buildMode,
required LinkValidator linkValidator,
required ApplicationAssetValidator applicationAssetValidator,
CCompilerConfig? cCompilerConfig,
IOSSdk? targetIOSSdk,
int? targetIOSVersion,
Expand All @@ -112,10 +139,14 @@ class NativeAssetsBuildRunner {
PackageLayout? packageLayout,
Uri? resourceIdentifiers,
String? runPackageName,
Iterable<String>? supportedAssetTypes,
required Iterable<String> supportedAssetTypes,
required BuildResult buildResult,
}) async =>
_run(
validator: (HookConfig config, HookOutputImpl output) =>
linkValidator(config as LinkConfig, output as LinkOutput),
applicationAssetValidator: (assets) async =>
applicationAssetValidator(assets),
hook: Hook.link,
linkModePreference: linkModePreference,
target: target,
Expand All @@ -141,6 +172,8 @@ class NativeAssetsBuildRunner {
required Target target,
required Uri workingDirectory,
required BuildMode buildMode,
required _HookValidator validator,
required ApplicationAssetValidator applicationAssetValidator,
CCompilerConfig? cCompilerConfig,
IOSSdk? targetIOSSdk,
int? targetIOSVersion,
Expand All @@ -150,7 +183,7 @@ class NativeAssetsBuildRunner {
PackageLayout? packageLayout,
Uri? resourceIdentifiers,
String? runPackageName,
Iterable<String>? supportedAssetTypes,
required Iterable<String> supportedAssetTypes,
BuildResult? buildResult,
bool? linkingEnabled,
}) async {
Expand Down Expand Up @@ -236,6 +269,7 @@ class NativeAssetsBuildRunner {
final (hookOutput, packageSuccess) = await _runHookForPackageCached(
hook,
config,
validator,
packageLayout.packageConfigUri,
workingDirectory,
includeParentEnvironment,
Expand All @@ -246,17 +280,14 @@ class NativeAssetsBuildRunner {
metadata[config.packageName] = hookOutput.metadata;
}

// Note the caller will need to check whether there are no duplicates
// between the build and link hook.
final validateResult = validateNoDuplicateDylibs(hookResult.assets);
if (validateResult.isNotEmpty) {
for (final error in validateResult) {
logger.severe(error);
}
hookResult = hookResult.copyAdd(HookOutputImpl(), false);
}
final errors = await applicationAssetValidator(hookResult.encodedAssets);
if (errors.isEmpty) return hookResult;

return hookResult;
logger.severe('Application asset verification failed:');
for (final error in errors) {
logger.severe('- $error');
}
return HookResult.failure();
}

static Future<HookConfigImpl> _cliConfig(
Expand All @@ -272,7 +303,7 @@ class NativeAssetsBuildRunner {
int? targetAndroidNdkApi,
int? targetIOSVersion,
int? targetMacOSVersion,
Iterable<String>? supportedAssetTypes,
Iterable<String> supportedAssetTypes,
Hook hook,
Uri? resourceIdentifiers,
BuildResult? buildResult,
Expand Down Expand Up @@ -331,7 +362,7 @@ class NativeAssetsBuildRunner {
cCompiler: cCompilerConfig,
targetAndroidNdkApi: targetAndroidNdkApi,
recordedUsagesFile: resourcesFile?.uri,
assets: buildResult!.assetsForLinking[package.name] ?? [],
encodedAssets: buildResult!.encodedAssetsForLinking[package.name] ?? [],
supportedAssetTypes: supportedAssetTypes,
linkModePreference: linkModePreference,
);
Expand Down Expand Up @@ -370,9 +401,10 @@ class NativeAssetsBuildRunner {
required Uri workingDirectory,
required bool includeParentEnvironment,
required bool linkingEnabled,
required BuildValidator buildValidator,
PackageLayout? packageLayout,
String? runPackageName,
Iterable<String>? supportedAssetTypes,
required Iterable<String> supportedAssetTypes,
}) async {
const hook = Hook.build;
packageLayout ??= await PackageLayout.fromRootPackageRoot(workingDirectory);
Expand Down Expand Up @@ -413,7 +445,7 @@ class NativeAssetsBuildRunner {
continue;
}
// TODO(https://github.com/dart-lang/native/issues/1321): Should dry runs be cached?
var (buildOutput, packageSuccess) = await runUnderDirectoriesLock(
final (buildOutput, packageSuccess) = await runUnderDirectoriesLock(
[
Directory.fromUri(config.outputDirectoryShared.parent),
Directory.fromUri(config.outputDirectory.parent),
Expand All @@ -423,6 +455,8 @@ class NativeAssetsBuildRunner {
() => _runHookForPackage(
hook,
config,
(HookConfig config, HookOutputImpl output) =>
buildValidator(config as BuildConfig, output as BuildOutput),
packageConfigUri,
workingDirectory,
includeParentEnvironment,
Expand All @@ -431,38 +465,15 @@ class NativeAssetsBuildRunner {
packageLayout!,
),
);
buildOutput = _expandArchsCodeAssets(buildOutput);
hookResult = hookResult.copyAdd(buildOutput, packageSuccess);
}
return hookResult;
}

HookOutputImpl _expandArchsCodeAssets(HookOutputImpl buildOutput) {
final assets = <Asset>[];
for (final asset in buildOutput.assets) {
switch (asset) {
case CodeAsset _:
if (asset.architecture != null) {
// Backwards compatibility, if an architecture is provided use it.
assets.add(asset);
} else {
// Dry run does not report architecture. Dart VM branches on OS
// and Target when looking up assets, so populate assets for all
// architectures.
for (final architecture in asset.os.architectures) {
assets.add(asset.copyWith(architecture: architecture));
}
}
case DataAsset _:
assets.add(asset);
}
}
return buildOutput.copyWith(assets: assets);
}

Future<_PackageBuildRecord> _runHookForPackageCached(
Hook hook,
HookConfigImpl config,
_HookValidator validator,
Uri packageConfigUri,
Uri workingDirectory,
bool includeParentEnvironment,
Expand Down Expand Up @@ -497,25 +508,13 @@ class NativeAssetsBuildRunner {
final lastBuilt = hookOutput.timestamp.roundDownToSeconds();
final dependenciesLastChange =
await hookOutput.dependenciesModel.lastModified();
late final DateTime assetsLastChange;
if (hook == Hook.link) {
assetsLastChange = await (config as LinkConfigImpl)
.assets
.map((a) => a.file)
.whereType<Uri>()
.map((u) => u.fileSystemEntity)
.lastModified();
}
if (lastBuilt.isAfter(dependenciesLastChange) &&
lastBuilt.isAfter(hookLastSourceChange) &&
(hook == Hook.build || lastBuilt.isAfter(assetsLastChange))) {
lastBuilt.isAfter(hookLastSourceChange)) {
logger.info(
[
'Skipping ${hook.name} for ${config.packageName} in $outDir.',
'Last build on $lastBuilt.',
'Last dependencies change on $dependenciesLastChange.',
if (hook == Hook.link)
'Last assets for linking change on $assetsLastChange.',
'Last hook change on $hookLastSourceChange.',
].join(' '),
);
Expand All @@ -528,6 +527,7 @@ class NativeAssetsBuildRunner {
return await _runHookForPackage(
hook,
config,
validator,
packageConfigUri,
workingDirectory,
includeParentEnvironment,
Expand All @@ -542,6 +542,7 @@ class NativeAssetsBuildRunner {
Future<_PackageBuildRecord> _runHookForPackage(
Hook hook,
HookConfigImpl config,
_HookValidator validator,
Uri packageConfigUri,
Uri workingDirectory,
bool includeParentEnvironment,
Expand Down Expand Up @@ -601,16 +602,12 @@ ${result.stdout}
final output = HookOutputImpl.readFromFile(file: config.outputFile) ??
HookOutputImpl();

final validateResult = await validate(
config,
output,
packageLayout,
);
success &= validateResult.success;
if (!validateResult.success) {
final errors = await _validate(config, output, packageLayout, validator);
success &= errors.isEmpty;
if (errors.isNotEmpty) {
logger.severe('package:${config.packageName}` has invalid output.');
}
for (final error in validateResult.errors) {
for (final error in errors) {
logger.severe('- $error');
}
return (output, success);
Expand Down Expand Up @@ -758,7 +755,7 @@ ${compileResult.stdout}
required OS targetOS,
required LinkModePreference linkMode,
required Uri buildParentDir,
Iterable<String>? supportedAssetTypes,
required Iterable<String> supportedAssetTypes,
required bool? linkingEnabled,
}) async {
const hook = Hook.build;
Expand Down Expand Up @@ -814,32 +811,33 @@ ${compileResult.stdout}
};
}

Future<ValidateResult> validate(
Future<ValidationErrors> _validate(
HookConfigImpl config,
HookOutputImpl output,
PackageLayout packageLayout,
_HookValidator validator,
) async {
var (errors: errors, success: success) = config is BuildConfigImpl
? await validateBuild(config, output)
: await validateLink(config as LinkConfigImpl, output);
final errors = config is BuildConfigImpl
? await validateBuildOutput(config, output)
: await validateLinkOutput(config as LinkConfig, output);
errors.addAll(await validator(config, output));

if (config is BuildConfigImpl) {
final packagesWithLink =
(await packageLayout.packagesWithAssets(Hook.link))
.map((p) => p.name);
for (final targetPackage in output.assetsForLinking.keys) {
for (final targetPackage in output.encodedAssetsForLinking.keys) {
if (!packagesWithLink.contains(targetPackage)) {
for (final asset in output.assetsForLinking[targetPackage]!) {
success &= false;
for (final asset in output.encodedAssetsForLinking[targetPackage]!) {
errors.add(
'Asset "${asset.id}" is sent to package "$targetPackage" for'
'Asset "$asset" is sent to package "$targetPackage" for'
' linking, but that package does not have a link hook.',
);
}
}
}
}
return (errors: errors, success: success);
return errors;
}

Future<(List<Package> plan, PackageGraph? dependencyGraph, bool success)>
Expand Down Expand Up @@ -878,9 +876,9 @@ ${compileResult.stdout}
// Link hooks are skipped if no assets for linking are provided.
buildPlan = [];
final skipped = <String>[];
final assetsForLinking = buildResult?.assetsForLinking;
final encodedAssetsForLinking = buildResult?.encodedAssetsForLinking;
for (final package in packagesWithHook) {
if (assetsForLinking![package.name]?.isNotEmpty ?? false) {
if (encodedAssetsForLinking![package.name]?.isNotEmpty ?? false) {
buildPlan.add(package);
} else {
skipped.add(package.name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ import '../../native_assets_builder.dart';
/// the dependency tree of the entry point application.
abstract interface class BuildDryRunResult {
/// The native assets produced by the hooks, which should be bundled.
List<Asset> get assets;
List<EncodedAsset> get encodedAssets;

/// Whether all hooks completed without errors.
///
/// All error messages are streamed to [NativeAssetsBuildRunner.logger].
bool get success;

/// The native assets produced by the hooks, which should be linked.
Map<String, List<Asset>> get assetsForLinking;
Map<String, List<EncodedAsset>> get encodedAssetsForLinking;
}
4 changes: 2 additions & 2 deletions pkgs/native_assets_builder/lib/src/model/build_result.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ abstract class BuildResult {
bool get success;

/// The native assets produced by the hooks, which should be bundled.
List<Asset> get assets;
List<EncodedAsset> get encodedAssets;

/// The native assets produced by the hooks, which should be linked.
Map<String, List<Asset>> get assetsForLinking;
Map<String, List<EncodedAsset>> get encodedAssetsForLinking;
}
Loading

0 comments on commit d8a84ea

Please sign in to comment.