[native_assets_cli] Refactor into `BuildInput` #1871

dcharkes · 2025-01-07T21:58:43Z

This PR changes the public API according to #1738, and should have minimal behavior changes.

Everything in HookConfig causes a different input.outputDirectory, all the other elements in the input do not.

This leads to the packageName no longer influencing the checksum, so the default output directory has been changed to .dart_tool/native_assets_builder/$packageName/$checksum.

Follow up PRs:

Serialize to a new JSON format
get rid of cCompiler envScript

Design:

[native_assets_cli] Move BuildConfig.targetOS to BuildConfig.codeConfig.targetOS, remove it entirely or extend to cover web #1738

github-actions · 2025-01-07T22:00:41Z

PR Health

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?

Changelog Entry ✔️

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package	Leaked API symbols

License Headers ✔️

// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// 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.

Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

coveralls · 2025-01-07T22:11:11Z

coverage: 87.893% (-0.008%) from 87.901%
when pulling a8bc599 on input-refactor
into 3b66738 on main.

mkustermann · 2025-01-08T08:41:05Z

In general I think this is good, I haven't looked at the code yet, but from commit message

    input.targetConfig.linkingEnabled; // build only
    input.targetConfig.dryRun; // build only, deleted soon
    // c.2. per asset
    input.targetConfig.buildAssetTypes;
    input.targetConfig.codeConfig.linkModePreference;
    input.targetConfig.codeConfig.targetArchitecture;
    input.targetConfig.codeConfig.targetOS;
    input.targetConfig.codeConfig.androidConfig.targetNdkApi;
    input.targetConfig.codeConfig.iOSConfig.targetSdk;
    input.targetConfig.codeConfig.iOSConfig.targetVersion;
    input.targetConfig.codeConfig.macOSConfig.targetVersion;

input.targetConfig.buildAssetTypes does sound a bit strange. The "target" terminology we somewhat already use for targetOS / targetArchitecture / targetPlatform / .... I'd probably prefer input.buildConfig but since we know it's a BuildInput already why not just input.config that has a type of BuildConfig / LinkConfig?

input.targetConfig.codeConfig.targetArchitecture also sounds strange as we have "config" twice in the name and "target" twice in the name. Now that we do not have the ambituity in linker input of input.code (code assets) vs input.codeConfig anymore, we could drop the "config" and just go with input.config.code.{ios,android,...} (of type {Build,Link}Input.{Build,Link}Config.CodeConfig.{IOSConfig,AndroidConfig,...}).

Lastly, given these changes and choice of checksuming, I think we should probably rename input.outputDirectory to something that indicates it's per-config, possibly input.configSpecificOutputDirectory (or better name) and input.sharedOutputDirectory - it's then clear to any reader that the former is per-config and the ladder is not.

so the default output directory has been changed to .dart_tool/native_assets_builder/$packageName/$checksum.

That may be a good thing, so users can easily see what takes up most space and can selectively rm -rf .dart_tool/native_assets_builder/<problematic-package>/

dcharkes · 2025-01-08T09:15:54Z

Done.

The API now looks like this:

void main(List<String> args) async {
  await build(args, (input, output) async {
    // a. shared
    input.packageName;
    input.packageRoot;
    input.outputDirectory;
    input.outputDirectoryShared;
    // b. hook-specific
    input.metadata; // build only
    input.assets.code; // link only
    input.assets.data; // link only
    input.recordedUsagesFile; // link only
    // c. target config
    // c.1. per hook
    input.config.linkingEnabled; // build only
    input.config.dryRun; // build only, deleted soon
    // c.2. per asset
    input.config.buildAssetTypes;
    input.config.code.linkModePreference;
    input.config.code.targetArchitecture;
    input.config.code.targetOS;
    input.config.code.android.targetNdkApi;
    input.config.code.iOS.targetSdk;
    input.config.code.iOS.targetVersion;
    input.config.code.macOS.targetVersion;
    input.config.code.cCompiler?.archiver;
    input.config.code.cCompiler?.compiler;
    input.config.code.cCompiler?.linker;

    output.assets.code.add(CodeAsset(
      package: 'package',
      name: 'name',
      linkMode: DynamicLoadingBundled(),
      os: input.config.code.targetOS,
      architecture: input.config.code.targetArchitecture,
      file: input.outputDirectory.resolve('foo'),
    ));
    output.assets.data.add(
      DataAsset(
        file: input.outputDirectory.resolve('foo'),
        name: 'name',
        package: 'package',
      ),
      linkInPackage: 'foo',
    );
    output.addDependency(input.packageRoot.resolve('x.txt'));
  });
}

I also dropped the Config suffixes in the setup method names and parameters.

    final inputBuilder = LinkInputBuilder()
      ..setupShared(
        packageName: packageName,
        packageRoot: packageRootUri,
        outputDirectory: outDirUri,
        outputDirectoryShared: outputDirectoryShared,
      )
      ..setupLink(
        assets: assets,
        recordedUsesFile: null,
      )
      ..config.setupShared(buildAssetTypes: [CodeAsset.type])
      ..config.setupCode(
        targetOS: OS.android,
        targetArchitecture: Architecture.arm64,
        android: AndroidConfig(targetNdkApi: 30),
        linkModePreference: LinkModePreference.preferStatic,
        cCompiler: CCompilerConfig(
          compiler: fakeClang,
          linker: fakeLd,
          archiver: fakeAr,
          envScript: fakeVcVars,
          envScriptArgs: ['arg0', 'arg1'],
        ),
      );

Lastly, given these changes and choice of checksumming, I think we should probably rename input.outputDirectory to something that indicates it's per-config, possibly input.configSpecificOutputDirectory (or better name) and input.sharedOutputDirectory - it's then clear to any reader that the former is per-config and the ladder is not.

I can't really come up with good names. I'd like to avoid making the names longer and also avoid making the names unrelated. Maybe adding documentation is enough. I believe that most users should be using outputDirectory so that should have the most plain name.

mkustermann

A few comments, the main one is about layering and buildAssetTypes.

mkustermann · 2025-01-08T11:09:08Z

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

    required BuildValidator buildValidator,
    required ApplicationAssetValidator applicationAssetValidator,
    required Uri workingDirectory,
    PackageLayout? packageLayout,
    String? runPackageName,
-    required List<String> buildAssetTypes,


Why did you remove this?

The current code has separation based on how the protocol is layered:

The optional things / protocol extensions are handled by embedder by calling out to it (e.g. code config)

The core protocol is handled in package:native_assets_builder, this field is core protocol support

So the build and link methods should take all things it needs for the core protocol, thereby ensuring those are setup as required by core protocol.

By moving it to the embedder, the package:native_assets_builder can no longer guarantee the core protocol fields are setup, it relies on the embedder to handle core protocol functionality - which doesn't seem right to me.

Oops, I forgot to revert this when I added buildAssetTypes back in. Thanks!

mkustermann · 2025-01-08T11:14:53Z

pkgs/native_assets_builder/test/build_runner/concurrency_shared_test_helper.dart

-      ..setupCodeConfig(
+    inputCreator: () => BuildInputBuilder()
+      ..config.setupShared(buildAssetTypes: [
+        CodeAsset.type,


Before it was DataAsset only, why did you have to add CodeAsset here?

We shouldn't be calling setupCode below without the CodeAsset.type here.
We need the setupCode to ensure differences in the directories.

I guess it's technically correct to add more config that's never read by anyone, but it's messy.

Maybe the setupCode call should be removed instead if it worked with data assets only?

There's 3 things that should be in sync

the buildAssetTypes

the config

the validation

With your changes we support code assets but it doesn't validate them, see

buildValidator: (input, output) async => await validateDataAssetBuildOutput(input, output),

If we work with data assets only, then we don't exercise the fact that we can have both identical and non-identical output directories. This test is doing directory locking and concurrency.

Adding the code validator

pkgs/native_assets_builder/test_data/package_reading_metadata/hook/build.dart

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

pkgs/native_assets_cli/lib/src/data_assets/config.dart

pkgs/native_assets_cli/lib/src/validation.dart

pkgs/native_assets_cli/tool/rename.dart

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

pkgs/native_assets_builder/test_data/use_all_api/pubspec.yaml

mkustermann

lgtm with comment

dcharkes · 2025-01-08T12:20:51Z

lgtm with comment

Thanks for the quick roundtrip @mkustermann! 🙏

I don't see a comment, did you forget to press send?

dcharkes · 2025-01-08T12:23:29Z

I don't see a comment, did you forget to press send?

Oh, it's in an existing conversation. Yay to GitHub UI. 🙈

dcharkes added 10 commits January 7, 2025 15:35

[native_assets_cli] Cleanup setup methods

a95412d

[native_assets_cli] Cleanup setup methods 2

2025963

[native_assets_cli] Rename various Config to Input

65cf085

[native_assets_cli] Introduce TargetConfig

f83db84

[native_assets_builder] OutputBuilder.assets

b2b761a

[native_assets_cli] Output.assets

5d2a46a

[native_assets_cli] LinkInput.assets

c621d60

[native_assets_cli] Move linkingEnabled to TargetConfig

9cb9244

[native_assets_cli] InputBuilder.targetConfig..setup

1785de5

[native_assets_cli] InputBuilder.targetConfig..setup

dd5e5af

github-actions bot added package:native_assets_cli package:native_toolchain_c package:native_assets_builder labels Jan 7, 2025

format

daddccf

dcharkes requested a review from mkustermann January 7, 2025 22:50

dcharkes added 5 commits January 8, 2025 09:47

[native_assets_cli] Drop Config suffixes

92f5e7f

[native_assets_cli] Rename TargetConfig

43c5828

[native_assets_cli] More renames

cdf7097

Add example files that try to use the full API

97ca2ea

Add documentation

b668b05

mkustermann reviewed Jan 8, 2025

View reviewed changes

dcharkes added 2 commits January 8, 2025 13:03

address comments

686f6b3

address comment

fb0ff70

dcharkes requested a review from mkustermann January 8, 2025 12:08

dcharkes marked this pull request as ready for review January 8, 2025 12:08

mkustermann approved these changes Jan 8, 2025

View reviewed changes

changelog

68028dc

add code validators

a8bc599

dcharkes added the autosubmit label Jan 8, 2025

auto-submit bot merged commit e4c4bd6 into main Jan 8, 2025
41 checks passed

auto-submit bot deleted the input-refactor branch January 8, 2025 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native_assets_cli] Refactor into `BuildInput` #1871

[native_assets_cli] Refactor into `BuildInput` #1871

dcharkes commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025

coveralls commented Jan 7, 2025 •

edited

Loading

mkustermann commented Jan 8, 2025

dcharkes commented Jan 8, 2025

mkustermann left a comment

mkustermann Jan 8, 2025

dcharkes Jan 8, 2025

mkustermann Jan 8, 2025

dcharkes Jan 8, 2025

mkustermann Jan 8, 2025

dcharkes Jan 8, 2025

dcharkes Jan 8, 2025

mkustermann left a comment

dcharkes commented Jan 8, 2025

dcharkes commented Jan 8, 2025

[native_assets_cli] Refactor into BuildInput #1871

[native_assets_cli] Refactor into BuildInput #1871

Conversation

dcharkes commented Jan 7, 2025 • edited Loading

github-actions bot commented Jan 7, 2025

PR Health

coveralls commented Jan 7, 2025 • edited Loading

mkustermann commented Jan 8, 2025

dcharkes commented Jan 8, 2025

mkustermann left a comment

Choose a reason for hiding this comment

mkustermann Jan 8, 2025

Choose a reason for hiding this comment

dcharkes Jan 8, 2025

Choose a reason for hiding this comment

mkustermann Jan 8, 2025

Choose a reason for hiding this comment

dcharkes Jan 8, 2025

Choose a reason for hiding this comment

mkustermann Jan 8, 2025

Choose a reason for hiding this comment

dcharkes Jan 8, 2025

Choose a reason for hiding this comment

dcharkes Jan 8, 2025

Choose a reason for hiding this comment

mkustermann left a comment

Choose a reason for hiding this comment

dcharkes commented Jan 8, 2025

dcharkes commented Jan 8, 2025

[native_assets_cli] Refactor into `BuildInput` #1871

[native_assets_cli] Refactor into `BuildInput` #1871

dcharkes commented Jan 7, 2025 •

edited

Loading

coveralls commented Jan 7, 2025 •

edited

Loading