Skip to content

Commit

Permalink
Allow custom build/link configuration, decouple core CLI infrastructu…
Browse files Browse the repository at this point in the history
…re from code assets, data assets, ... (#1643)

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 purposes 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
    write/mutation only

  * 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
  successful 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 authored Nov 4, 2024
1 parent 84542e3 commit 5164777
Show file tree
Hide file tree
Showing 160 changed files with 4,344 additions and 6,170 deletions.
5 changes: 5 additions & 0 deletions pkgs/native_assets_builder/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
consistency of the sum of those parts. Effectively this means: Any asset that
doesn't have an explicit linker will get a NOP linker that emits as outputs
it's inputs.
- **Breaking change** Removes knowledge about code & data assets from
`package:native_assets_builder`. Users of this package can know hook into the
build/link hook configuration that is used and e.g. initialize code
configuration. Similarly users of this package now have to provide a callback
to verify the consistency of the used hook configuration.

## 0.8.3

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class NativeAssetsBuildPlanner {
);
}

(List<Package> packages, bool success) plan({
List<Package>? plan({
String? runPackageName,
}) {
final PackageGraph packageGraph;
Expand All @@ -62,7 +62,6 @@ class NativeAssetsBuildPlanner {
final packagesToBuild = packageMap.keys.toSet();
final stronglyConnectedComponents = packageGraph.computeStrongComponents();
final result = <Package>[];
var success = true;
for (final stronglyConnectedComponent in stronglyConnectedComponents) {
final stronglyConnectedComponentWithNativeAssets = [
for (final packageName in stronglyConnectedComponent)
Expand All @@ -73,13 +72,13 @@ class NativeAssetsBuildPlanner {
'Cyclic dependency for native asset builds in the following '
'packages: $stronglyConnectedComponentWithNativeAssets.',
);
success = false;
return null;
} else if (stronglyConnectedComponentWithNativeAssets.length == 1) {
result.add(
packageMap[stronglyConnectedComponentWithNativeAssets.single]!);
}
}
return (result, success);
return result;
}
}

Expand Down
Loading

0 comments on commit 5164777

Please sign in to comment.