Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[native_assets_cli] Unify metadata and assets #1251

Open
dcharkes opened this issue Jul 2, 2024 · 5 comments
Open

[native_assets_cli] Unify metadata and assets #1251

dcharkes opened this issue Jul 2, 2024 · 5 comments
Labels
P2 A bug or feature request we're likely to work on package:native_assets_cli

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented Jul 2, 2024

We currently have 3 types of output of a build hook:

  • assets (for bundling)
  • assets (for link hooks)
  • metadata (for other build hooks)

The most common use cases for metadata are:

  • Lazily downloading a native toolchain and communicating the path to that toolchain. (For example the Android NDK (Installing NDK automatically #976) or the Rust compiler.)
  • Building a toolchain (for example your custom svg-to-tesselated or 3dmodel-to-shader)

Maybe there are other use cases but, we haven't come up with them yet.

If there's a use case for structured data, it might be fine to just put that structured data in a json file and make it a data asset.

Unifying these 3 types of assets would:

  1. Simplify the API. (Currently we have addAsset(...) for assets for bundling and assetAsset(linkIn: 'some_package', ...) for sending it to a link hook, and addMetadata(...) for sending info to another build hook.)
  2. Make dependency tracking per "asset" work properly for executables send to other build hooks ([native_assets_cli] Dependencies should be per asset #1208).

Then we need to come up with some name for these 3 types of "buildables".

  • Probably we should only the things used for bundling "asset"s?

Thanks @mosuem @mkustermann @liamappelbe @HosseinYousefi for the input! And please leave any other thoughts and suggestions.

@dcharkes dcharkes added P1 A high priority bug; for example, a single project is unusable or has many test failures package:native_assets_cli labels Jul 2, 2024
@dcharkes
Copy link
Collaborator Author

dcharkes commented Jul 8, 2024

@bdero Just a heads up, we might break the metadata soon (or temporarily remove it until we find an API that we want to add to do this).

As a workaround, you could try to build the compiler in my_compiler/hook/build.dart and do dart run my_compiler:main to run my_compiler/bin/main.dart which would do the compilation inside my_package_using_compiler/hook/build.dart.

@dcharkes
Copy link
Collaborator Author

I thought I had posted an API sketch here earlier, but apparently I didn't.

My current way of thinking is:

  void addAsset(
    Asset asset, {
    Iterable<Uri>? dependencies,
    bool forBuildHooks, // if true, not bundled, but available to build hooks that have this build hook as direct dependency
    String? linkInPackage, // if set, not bundled directly but sent to specific link hook
  });

This way we can properly track dependencies per asset no matter where these assets are wired to:

auto-submit bot pushed a commit that referenced this issue Jul 15, 2024
We might not address this issue fully before stable.

* #1251

But we're quite sure to remove the current metadata API, so deprecate it.
@mosuem
Copy link
Member

mosuem commented Jul 16, 2024

The API is starting to look a bit overloaded with arguments. I wonder if there is a way to simplify this. Maybe use asset types instead? This would make asset creation a bit more nested.

class MetadataAsset {
  final Asset asset;
}

class LinkableAsset {
  final String targetPackage;
  final Asset asset;
}

@lrhn, as you like to simplify APIs :)

@dcharkes
Copy link
Collaborator Author

The API is starting to look a bit overloaded with arguments. I wonder if there is a way to simplify this. Maybe use asset types instead? This would make asset creation a bit more nested.

class MetadataAsset {
  final Asset asset;
}

class LinkableAsset {
  final String targetPackage;
  final Asset asset;
}

@lrhn, as you like to simplify APIs :)

Or maybe we should have a "Routing" (name tbd) param instead. That would also give us a place to document the behavior.

addAsset(
  Asset asset, {
  Routing routing = const Routing.bundleInApp,
});

final class Routing {
  /// This asset will be bundled in the app by the Dart or Flutter SDK.
  static const Routing bundleInApp;

  /// This asset is available in build hooks of which the packages have a
  /// direct dependency on the package with this build hook.
  static const Routing forBuildHooks;

  /// 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.
  factory Routing.linkInPackage(String packageName)
}

@dcharkes dcharkes added P2 A bug or feature request we're likely to work on and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Jul 30, 2024
@dcharkes dcharkes added this to the Native Assets v1.0 milestone Aug 30, 2024
@dcharkes
Copy link
Collaborator Author

Circling back on this issue after we have decided that:

  1. Metadata is not part of checksums ([native_assets_cli] HookConfigBuilder.checksum should exclude some keys #1803).
  2. Hooks are invoked multiple times (per asset type, and for code assets per target arch), and they can only output asset types that are in buildAssetTypes ([native_assets_cli] Semantics of one vs multiple hook invocations #1739).
  3. We don't track dependencies per asset ([native_assets_cli] Dependencies should be per asset #1208).

Looking at the suggestion to reuse Assets for Metadata, I don't believe the list of use cases necessarily fits the asset types:

  1. Data assets happen to be usable for structured data, but they don't need to be accessible at runtime from Dart via an asset API, so they don't need an asset-id. (The asset id can be "used" for access, but metadata is already namespaced per package.)
  2. Code assets will most likely not be used as a metadata. If anything one might want to pass an executable (not a dylib or static library).

Moving metadata to be inside files rather than structured data in the JSON would still have the benefit of better caching: if the metadata is not read, then the metadata file does not have to be added to dependencies, and the cache is not invalidated. However, I'm not entirely sure how often such invalidation would happen in practise. A build hook only sees the metadata from a direct dependency. And most direct dependencies will be published packages that only change if the dependencies are bumped in the pubspec (in which case the Dart source change and hooks are rerun).

So it might be fine to just keep the current approach (encoding the metadata in json). If someone really has a super big meta data, they can always chose to encode a file-path in the BuildOutput and then read the file in other build hooks.

That being said, due to the routing similarities between metadata (build hook to direct dependendee build hook) and assets for linking (build hook to specific link hook), it would still feel nice to unify the concepts/align APIs. (And most likely using files instead of arbitrary json fits better if we try to unify the APIs.)

Non-satisfactory attempt:

// build or link hook -> bundle in app
output.assets.code.add(
  CodeAsset(...),
);
// build hook -> link hook
output.assets.code.add(
  CodeAsset(...),
  linkInPackage: 'foo',
);
// build hook -> build hook
output.assets.metadata.add(
  MetaDataAsset(
    key,
    value,
  ),
);

If we embrace just encoding in JSON, we probably should change the API to be more in line with how input.assets and output.assets work. Something along the lines of having input.metadata and output.metadata and having add/set/get methods on there:

input.metadata[package]?[key];
output.metadata[key] = value;
output.metadata.addAll(...);

For reference, current API:

input.metadatum(package, key);
input.metadata(package);
output.addMetadatum(key, value);
output.addMetadata(Map<String, Object> metadata);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on package:native_assets_cli
Projects
Status: No status
Development

No branches or pull requests

2 participants