Skip to content

Commit

Permalink
(not to be committed yet / not done yet) Allow custom build/link conf…
Browse files Browse the repository at this point in the history
…iguration

This PR allows users of `package:native_assets_builder` to supply custom
build/link configuration. This allows e.g. flutter to add additional
configuration to `hook/{build,link}.dart` scripts that require flutter
specific things.

As opposed to earlier PRs that merged `Foo` and `FooImpl` we have a
different approach for build/link config/output as they are something
different:

The current API (even before the recent refactoring to it) allows read
and write access to e.g. assets, ...). We remove this capability as this
is conceptually problematic:

- Currently those API classes are both mutable and at the same time
  support operator==/hashCode:
  => This is problematic as inserting such an object into a set/map and
  then modifying it means one can later on not find it anymore. Mutable
  objects can have operator==/hashCode iff it doesn't change (e.g. the
  default implementation based on identity). Otherwise objects should be
  immutable if they want to support operator==/hashCode.
  => For our puroses we have no need for operator==/hashCode and
  therefore remove this (problematic) capability.

- Currently those API classes are serving both the hook writers and the
  bundling tool. The bundling tool would use Impl versions of those, but
  in the end operate on the same structures.

We now change this to be using the builder pattern: The code that
  * creates build/link config/output will use a builder object that allows
    mutationonly

  * consumes build/link config/output will uses a view object that only
    allows read access

We then make those build/link config/output objects flexible in the
sense that

  * a bundling tool can add more configuration to build/link
    configuration
  * a hook can consume this additional configuration

To support this we

a) Make the builders operate on a json map that allows incrementally add more
   things to the build/link config.

  => The bundling tool gives `package:native_assets_builder` a function
  that creates the initial configuration which allows it to add
  bundling-tool specific configs (e.g. flutter specific things).
  => Initializing the configs is split up into groups that belong together
  => Named with `{Build,Link}ConfigBuilder.setup*()` methods which
     initialize things that conceptually belong together.
  => A bundling tool can then e.g. add code asset specifics via
    `configBuilder.setupCodeConfig(...)`

b) Make the hooks operate on a json map that allows viewing this
  additional information via extension methods.

  => A hook can use e.g. `config.codeConfig.cCompiler`

Since not all bundling tools may want support code assets (web builds),
the code asset specific configuration is now moved out of the core packages
and is one such bundling-tool specific configuration.

  => Hook writers can now access it via `config.codeConfig.*`
  => This sets up the stage to change the CLI protocol to move
      those code-asset specific things into a subtree of the config
      (e.g. json['code_asset_config'])
  => Then we can allow hook writers to easily detect it by introducing a
     `config.hasCodeConfig` getter.

We make various smaller other changes, e.g.

* The `package:native_assets_builder` APIs now either return a result on
  successfull build or `null` in case of error.
  => This makes callers have to check (due to nullable type of result)
    whether build/link succeeded or not (easy to forget to check for a
    `result.success` boolean - as evidenced a number of tests that
    didn't check this `success` boolean)
  => It avoids returning a result that is only partial (i.e. has some
  assets but not all due to some builds failing)

* The `Architecture` is now moved to be a code-asset specific
  configuration
  => It makes sense for code assets.
  => It wouldn't make sense for e.g. web builds where there's no code
     assets available.
  => For now we keep the target operating system, but even that's
     somewhat problematic for web builds.

* We no longer need the (temporary) `output.{code,data}Assets.all`
  getters
  => This is now natural due to the seperation via builder pattern.

Overall:
* The changes on the bundling tool side are rather minimal:
```diff
     final buildResult = await nativeAssetsBuildRunner.build(
+      configCreator: () => BuildConfigBuilder()
+        ..setupCodeConfig(
+          linkModePreference: LinkModePreference.dynamic,
+          targetArchitecture: target.architecture,
+          targetMacOSVersion: targetMacOSVersion,
+          cCompilerConfig:  _getCCompilerConfig(),
+        ),
       workingDirectory: workingDirectory,
-      target: target,
-      linkModePreference: LinkModePreference.dynamic,
+      targetOS: target.os,
       buildMode: BuildMode.release,
       includeParentEnvironment: true,
-      targetMacOSVersion: targetMacOSVersion,
       linkingEnabled: true,
       supportedAssetTypes: [
         CodeAsset.type,
@@ -160,7 +168,7 @@ class BuildCommand extends DartdevCommand {
         ...await validateCodeAssetsInApplication(assets),
       ],
     );
-    if (!buildResult.success) {
+    if (buildResult == null) {
       stderr.writeln('Native assets build failed.');
       return 255;
     }
@
```

* The changes on the hook writer side are rather minimal as well, e.g.:
```
-      final iosVersion = config.targetIOSVersion;
+      final iosVersion = config.codeConfig.targetIOSVersion;
```

The main changes are all encapsulated within the
`package:native_assets_builder` and `package:native_assets_cli` and the
*many* tests that need updating.
  • Loading branch information
mkustermann committed Oct 10, 2024
1 parent d7cdac4 commit 9a00960
Show file tree
Hide file tree
Showing 101 changed files with 3,060 additions and 5,466 deletions.
615 changes: 273 additions & 342 deletions pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,12 @@

import 'package:native_assets_cli/native_assets_cli_internal.dart';

import '../../native_assets_builder.dart';

/// The result of executing the build hooks in dry run mode from all packages in
/// the dependency tree of the entry point application.
abstract interface class BuildDryRunResult {
/// The native assets produced by the hooks, which should be bundled.
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<EncodedAsset>> get encodedAssetsForLinking;
}
7 changes: 0 additions & 7 deletions pkgs/native_assets_builder/lib/src/model/build_result.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,12 @@

import 'package:native_assets_cli/native_assets_cli_internal.dart';

import '../build_runner/build_runner.dart';

/// The result of executing build hooks from all packages in the dependency tree
/// of the entry point application.
abstract class BuildResult {
/// The files used by the hooks.
List<Uri> get dependencies;

/// 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 bundled.
List<EncodedAsset> get encodedAssets;

Expand Down
41 changes: 14 additions & 27 deletions pkgs/native_assets_builder/lib/src/model/hook_result.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,61 +22,48 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult {
@override
final List<Uri> dependencies;

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

HookResult._({
required this.encodedAssets,
required this.encodedAssetsForLinking,
required this.dependencies,
required this.success,
});

factory HookResult({
List<EncodedAsset>? encodedAssets,
Map<String, List<EncodedAsset>>? encodedAssetsForLinking,
List<Uri>? dependencies,
bool success = true,
}) =>
HookResult._(
encodedAssets: encodedAssets ?? [],
encodedAssetsForLinking: encodedAssetsForLinking ?? {},
dependencies: dependencies ?? [],
success: success,
);

factory HookResult.failure() => HookResult(success: false);

HookResult copyAdd(HookOutputImpl hookOutput, bool hookSuccess) {
final mergedMaps =
mergeMaps(encodedAssetsForLinking, hookOutput.encodedAssetsForLinking,
value: (encodedAssets1, encodedAssets2) => [
...encodedAssets1,
...encodedAssets2,
]);
HookResult copyAdd(HookOutput hookOutput) {
final mergedMaps = mergeMaps(
encodedAssetsForLinking,
hookOutput is BuildOutput
? hookOutput.encodedAssetsForLinking
: <String, List<EncodedAsset>>{},
value: (encodedAssets1, encodedAssets2) => [
...encodedAssets1,
...encodedAssets2,
]);
final hookOutputAssets = (hookOutput is BuildOutput)
? hookOutput.encodedAssets
: (hookOutput as LinkOutput).encodedAssets;
return HookResult(
encodedAssets: [
...encodedAssets,
...hookOutput.encodedAssets,
...hookOutputAssets,
],
encodedAssetsForLinking: mergedMaps,
dependencies: [
...dependencies,
...hookOutput.dependencies,
]..sort(_uriCompare),
success: success && hookSuccess,
);
}

HookResult withSuccess(bool success) => HookResult(
encodedAssets: encodedAssets,
encodedAssetsForLinking: encodedAssetsForLinking,
dependencies: dependencies,
success: success,
);
}

int _uriCompare(Uri u1, Uri u2) => u1.toString().compareTo(u2.toString());
7 changes: 0 additions & 7 deletions pkgs/native_assets_builder/lib/src/model/link_result.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

import 'package:native_assets_cli/native_assets_cli_internal.dart';

import '../build_runner/build_runner.dart';

/// The result of executing the link hooks in dry run mode from all packages in
/// the dependency tree of the entry point application.
abstract interface class LinkResult {
Expand All @@ -15,9 +13,4 @@ abstract interface class LinkResult {

/// The files used by the hooks.
List<Uri> get dependencies;

/// Whether all hooks completed without errors.
///
/// All error messages are streamed to [NativeAssetsBuildRunner.logger].
bool get success;
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ void main() async {
// Trigger a build, should invoke build for libraries with native assets.
{
final logMessages = <String>[];
final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
capturedLogs: logMessages,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
expect(
logMessages.join('\n'),
stringContainsInOrder(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ void main() async {
applicationAssetValidator: validateCodeAssetsInApplication,
);
final fullLog = logMessages.join('\n');
expect(result.success, false);
expect(result, isNull);
expect(
fullLog,
contains('does not start with "package:wrong_namespace_asset/"'),
Expand Down Expand Up @@ -66,7 +66,7 @@ void main() async {
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
expect(result.success, true);
expect(result, isNotNull);
}
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,22 @@ void main() async {
logger: logger,
);

final dryRunResult = await buildDryRun(
final dryRunResult = (await buildDryRun(
packageUri,
logger,
dartExecutable,
linkingEnabled: false,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
);
final buildResult = await build(
))!;
final buildResult = (await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;

expect(dryRunResult.encodedAssets.length, 1);
expect(buildResult.encodedAssets.length, 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ void main() async {
applicationAssetValidator: validateCodeAssetsInApplication,
);
final fullLog = logMessages.join('\n');
expect(result.success, false);
expect(result, isNull);
if (package == 'wrong_build_output_3') {
// Should re-execute the process on second run.
expect(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ void main() async {

{
final logMessages = <String>[];
final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
capturedLogs: logMessages,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
expect(
logMessages.join('\n'),
contains(
Expand All @@ -54,15 +54,15 @@ void main() async {

{
final logMessages = <String>[];
final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
capturedLogs: logMessages,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
expect(
logMessages.join('\n'),
contains('Skipping build for native_add'),
Expand Down Expand Up @@ -99,14 +99,14 @@ void main() async {
await Future<void>.delayed(const Duration(seconds: 1));

{
final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
await expectSymbols(
asset: CodeAsset.fromEncoded(result.encodedAssets.single),
symbols: ['add']);
Expand All @@ -118,14 +118,14 @@ void main() async {
);

{
final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
await expectSymbols(
asset: CodeAsset.fromEncoded(result.encodedAssets.single),
symbols: ['add', 'subtract'],
Expand All @@ -152,14 +152,14 @@ void main() async {
// cached.
await Future<void>.delayed(const Duration(seconds: 1));

final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
{
final compiledHook = logMessages
.where((m) =>
Expand All @@ -179,14 +179,14 @@ void main() async {
targetUri: packageUri);

{
final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
{
final compiledHook = logMessages
.where((m) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ void main() async {
buildValidator: (config, output) async => [],
);
final fullLog = logMessages.join('\n');
expect(result.success, false);
expect(result, isNull);
expect(
fullLog,
contains(
Expand All @@ -52,7 +52,7 @@ void main() async {
applicationAssetValidator: (_) async => [],
);
final fullLog = logMessages.join('\n');
expect(result.success, false);
expect(result, isNull);
expect(
fullLog,
contains(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ void main() async {
);

{
final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
expect(result.encodedAssets.length, 1);
await expectSymbols(
asset: CodeAsset.fromEncoded(result.encodedAssets.single),
Expand Down Expand Up @@ -60,7 +60,7 @@ void main() async {
applicationAssetValidator: validateCodeAssetsInApplication,
);
final fullLog = logMessages.join('\n');
expect(result.success, false);
expect(result, isNull);
expect(fullLog, contains('To reproduce run:'));
final reproCommand = fullLog
.split('\n')
Expand All @@ -78,14 +78,14 @@ void main() async {
);

{
final result = await build(
final result = (await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetsInApplication,
);
))!;
expect(result.encodedAssets.length, 1);
await expectSymbols(
asset: CodeAsset.fromEncoded(result.encodedAssets.single),
Expand Down
Loading

0 comments on commit 9a00960

Please sign in to comment.