-
Notifications
You must be signed in to change notification settings - Fork 68
[native_assets_cli] Unify Metadata
with Asset
s
#2164
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
Conversation
PR HealthBreaking changes ✔️
Changelog Entry ✔️
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.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
e2e2ef3
to
2f4d1c1
Compare
d6a45fa
to
5514561
Compare
We reserve the right to invoke build hooks & link hooks once or multiple times. If we invoke it multiple times we may add different values for @dcharkes Is the new |
You can output any asset type in |
5f12140
to
b5729b9
Compare
5514561
to
6041be6
Compare
b5729b9
to
3da9128
Compare
fff25a7
to
3525c01
Compare
3da9128
to
0e1f6e1
Compare
3525c01
to
28009f3
Compare
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
@@ -32,7 +32,10 @@ void main(List<String> arguments) async { | |||
output.assets.code.add( | |||
tempBuildOutput.assets.code.single, | |||
// Send dylib to linking if linking is enabled. | |||
linkInPackage: input.config.linkingEnabled ? packageName : null, | |||
routing: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an idea: We could avoid the input.config.linkingEnabled
bool calls by instead having something like an input.config.routing
, which can be a LinkingEnabledRouter
which has a toLinker
method, or a LinkingNotEnabledRouter
which only has bundleInApp or
toBuilder`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having a input.config.routing
. 👍 That makes the API symmetric again.
I'm not sure that having to take the value from the input would make a very nice API. Especially for ToBuild
or BundleInApp
where you don't need to check the config. Let me give it a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a spin, but make the call sites even more verbose.
routing: switch (input.config.routing) {
final LinkingEnabledRouter r => r.toLinkHook(packageName),
LinkingDisabledRouter() => const ToAppBundle(),
},
Then the following is more readable:
routing: switch (input.config.routing) {
LinkingEnabledRouter() => ToLinkHook(packageName),
LinkingDisabledRouter() => const ToAppBundle(),
},
Current solution (this doesn't show the routing
)
routing: switch (input.config.linkingEnabled) {
true => ToLinkHook(packageName),
false => const ToAppBundle(),
},
With routing
added:
routing: switch (input.config.routing.linkHooksEnabled) {
true => ToLinkHook(packageName),
false => const ToAppBundle(),
},
True false is not very pretty. Let's try a when
routing: switch (input.config.routing) {
final routing when routing.linkHooksEnabled => ToLinkHook(packageName),
_ => const ToAppBundle(),
},
Also not that great.
Then the following is more readable:
routing: switch (input.config.routing) {
LinkHooksEnabled() => ToLinkHook(packageName),
LinkHooksDisabled() => const ToAppBundle(),
},
But it only works if that's the only option we ever want to add to the routing. It's only the Dart API, so we could do it, it's easy to refactor.
(P.S. If we want to go the route with the method, we should probably do the same with LinkModePreference
and LinkMode
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try this in a more holistic way for everywhere where the input-output constraint could be made more explicit.
pkgs/native_assets_builder/test_data/reusable_dynamic_library/README.md
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/test_data/reuse_dynamic_library/hook/build.dart
Show resolved
Hide resolved
pkgs/native_assets_builder/test_data/reuse_dynamic_library/hook/build.dart
Outdated
Show resolved
Hide resolved
0e1f6e1
to
cf34082
Compare
#2163) Bug: #1251 This PR contains the JSON spec for: * #2164 For more details see that PR. This PR adds the syntax for passing assets from build hooks to build hooks. build output `assets_for_build`: ```json { "assets": [], "assets_for_build": [ { "encoding": { "a_key": "some_value" }, "some_key": "some_value", "type": "some_asset_type" } ], "assets_for_linking": {}, } ``` build input `assets` per package name: ```json { "assets": { "some_package": [ { "some_key": "some_value", "type": "some_asset_type" } ] }, "config": {}, } ``` Secondly, this PR adds a syntax for metadata assets (assets that cannot be bundled but just ferry information between different hooks). ```json { "encoding": { "key": "foo", "value": "bar" }, "type": "hooks/metadata" }, ``` The schemas now check these. And this is covered by tests. Changes to the generated schemas: * Override the new build input `asset` and build output `assets_for_build` with the `CodeAsset` and `DataAsset` in the generated extension schemas. Notable changes to the syntax generator: * Enable generating tagged enum classes if there are two fields (a tag, and some `encoding`) in the super class. * Support `Object` fields (used for `MetadataAsset.value`).
803df09
to
22b0a86
Compare
Updating the dart-lang/native dependencies to the ones published yesterday. Incorporates a fix for: * dart-lang/native#2149 * Note that a bunch of tests were relying on ignoring build assets. The tests have been fixed. And the new `AssetRouting` syntax in the build hook for sending assets to the link hook: * dart-lang/native#2164 Does not include support for user-defines in the pubspec yet. Will do this in a follow up PR. * dart-lang/native#2165
Bug: #1251
This PR adds support for two things:
Routing
assets from build hook to build hooks.MetadataAsset
for sending metadata between hooks.This enables two use cases:
Example:
pkgs/native_assets_builder/test_data/package_with_metadata/hook/build.dart
+pkgs/native_assets_builder/test_data/package_reading_metadata/hook/build.dart
Example:
pkgs/native_assets_builder/test_data/reusable_dynamic_library/hook/build.dart
+pkgs/native_assets_builder/test_data/reuse_dynamic_library/hook/build.dart
To accommodate this change the
String? linkInPackage
is replaced withRouting routing
everywhere. This has three subtypes:BundleInApp()
, equivalent tolinkInPackage: null
,Tolinker(String packageName)
, equivalent tolinkInPackage: packageName
, andToBuild()
which makes the asset available in theBuildInput
of the direct dependencies.Note that assets routed to other build hooks via
ToBuild()
also need to be routed toBundleInApp()
. If another package uses the asset at runtime, it cannot re-bundle it itself because that would lead to double bundling if two packages use such asset.(This leads to a weird quirk where we now have
List<Routing>
as a parameter forCBuilder
because we want the asset to be added in two places. We should probably redesignCBuilder.run
to not use theHookInput
/HookOutput
directly.)