Skip to content

[hooks] Run link hooks in order #2417

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[hooks] Run link hooks in order #2417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
15cf956
Run link hooks in order
mosuem Jul 28, 2025
3f14d47
change version
mosuem Jul 28, 2025
d75cdf0
Switch back to assets
mosuem Jul 28, 2025
226fd38
add to test data
mosuem Jul 28, 2025
91bdfe1
normalized
mosuem Jul 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkgs/code_assets/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## 0.19.5-wip
## 0.20.0-wip

- Bump `package:hooks` to 0.20.0.

Expand Down
16 changes: 12 additions & 4 deletions pkgs/code_assets/lib/src/code_assets/config.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,19 @@ final class LinkOutputCodeAssetBuilder {

LinkOutputCodeAssetBuilder._(this._output);

/// Adds the given [asset] to the link hook output.
void add(CodeAsset asset) => _output.addEncodedAsset(asset.encode());
/// Adds the given [asset] to the hook output with [routing].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Routing deserve some better documentation. For a developer seeing this for the first time it is unclear what routing is and there are no breadcrumbs in the docs to find out more about it.

Questions the docs should answer:

  • What is routing?
  • Why/When should I set it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think documentation is lacking in general - so far there are little to no starting points for developers. I will also add some here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking through this, it seems kind of well documented though.

void add(CodeAsset asset, {LinkAssetRouting routing = const ToAppBundle()}) =>
_output.addEncodedAsset(asset.encode(), routing: routing);

/// Adds the given [assets] to the link hook output.
void addAll(Iterable<CodeAsset> assets) => assets.forEach(add);
/// Adds the given [assets] to the hook output with [routing].
void addAll(
Iterable<CodeAsset> assets, {
LinkAssetRouting routing = const ToAppBundle(),
}) {
for (final asset in assets) {
add(asset, routing: routing);
}
}
}

/// Extension to initialize code specific configuration on link/build inputs.
Expand Down
9 changes: 4 additions & 5 deletions pkgs/code_assets/lib/src/code_assets/validation.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ Future<ValidationErrors> validateCodeAssetBuildOutput(
output.assets.encodedAssets,
[
...output.assets.encodedAssetsForBuild,
for (final assetList in output.assets.encodedAssetsForLinking.values)
...assetList,
...output.assets.encodedAssetsForLinking.values.expand((assets) => assets),
],
output,
true,
Expand All @@ -106,7 +105,7 @@ Future<ValidationErrors> validateCodeAssetLinkOutput(
input,
input.config.code,
output.assets.encodedAssets,
[],
output.assets.encodedAssetsForLink.values.expand((assets) => assets),
output,
false,
);
Expand Down Expand Up @@ -135,8 +134,8 @@ Future<ValidationErrors> validateCodeAssetInApplication(
Future<ValidationErrors> _validateCodeAssetBuildOrLinkOutput(
HookInput input,
CodeConfig codeConfig,
List<EncodedAsset> encodedAssetsBundled,
List<EncodedAsset> encodedAssetsNotBundled,
Iterable<EncodedAsset> encodedAssetsBundled,
Iterable<EncodedAsset> encodedAssetsNotBundled,
HookOutput output,
bool isBuild,
) async {
Expand Down
2 changes: 1 addition & 1 deletion pkgs/code_assets/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: >-
This library contains the hook protocol specification for bundling native code
with Dart packages.

version: 0.19.5-wip
version: 0.20.0-wip

repository: https://github.com/dart-lang/native/tree/main/pkgs/code_assets

Expand Down
50 changes: 46 additions & 4 deletions pkgs/code_assets/test/code_assets/config_test.dart
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected to see some tests that call the add/addAll API while providing a value to the new routing parameter to test that its plumped through correctly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be covered by the tests of pkgs/hooks_runner/test_data/flag_app

Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,17 @@ void main() async {
'extensions': {'code_assets': codeConfig},
if (hookType == 'build') 'linking_enabled': false,
},
if (hookType == 'link')
'assets_from_linking': [
{
'type': 'code_assets/code',
'encoding': {
'file': 'not there',
'id': 'package:my_package/name2',
'link_mode': {'type': 'dynamic_loading_bundle'},
},
},
],
'out_dir_shared': outputDirectoryShared.toFilePath(),
'out_file': outFile.toFilePath(),
'package_name': packageName,
Expand Down Expand Up @@ -168,7 +179,18 @@ void main() async {
outputFile: outFile,
outputDirectoryShared: outputDirectoryShared,
)
..setupLink(assets: assets, recordedUsesFile: null)
..setupLink(
assets: assets,
recordedUsesFile: null,
assetsFromLinking: [
CodeAsset(
name: 'name2',
package: 'my_package',
file: Uri.file('not there'),
linkMode: DynamicLoadingBundled(),
).encode(),
],
)
..addExtension(
CodeAssetExtension(
targetOS: OS.android,
Expand Down Expand Up @@ -286,9 +308,29 @@ void main() async {
predicate(
(e) =>
e is FormatException &&
e.message.contains(
"No value was provided for 'assets.0.encoding.link_mode'.",
),
e.message.contains("""
No value was provided for 'assets.0.encoding.link_mode'."""),
),
),
);
});

test('LinkInput.assets_from_linking.0.encoding.key missing', () {
final input = inputJson(hookType: 'link');
traverseJson<Map<String, Object?>>(input, [
'assets_from_linking',
0,
'encoding',
]).remove('link_mode');
expect(
() =>
LinkInput(input).assets.assetsFromLinking.first.asCodeAsset.linkMode,
throwsA(
predicate(
(e) =>
e is FormatException &&
e.message.contains("""
No value was provided for 'assets_from_linking.0.encoding.link_mode'."""),
),
),
);
Expand Down
2 changes: 1 addition & 1 deletion pkgs/data_assets/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## 0.19.2-wip
## 0.20.0-wip

- Bump `package:hooks` to 0.20.0.

Expand Down
47 changes: 28 additions & 19 deletions pkgs/data_assets/lib/src/data_assets/config.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,23 @@ extension BuildOutputBuilderAddDataAssetsDirectories on BuildOutputBuilder {
List<String> paths, {
required BuildInput input,
bool recursive = false,
AssetRouting routing = const ToAppBundle(),
}) async {
String assetName(Uri assetUri) => assetUri
.toFilePath(windows: false)
.substring(input.packageRoot.toFilePath().length);
.substring(input.packageRoot.toFilePath(windows: false).length);

void addAsset(File file) {
assets.data.add(
DataAsset(
package: input.packageName,
name: assetName(file.uri),
file: file.uri,
),
routing: routing,
);
addDependency(file.uri);
}

for (final path in paths) {
final resolvedUri = input.packageRoot.resolve(path);
Expand All @@ -45,15 +58,10 @@ extension BuildOutputBuilderAddDataAssetsDirectories on BuildOutputBuilder {
followLinks: false,
)) {
if (entity is File) {
assets.data.add(
DataAsset(
package: input.packageName,
name: assetName(entity.uri),
file: entity.uri,
),
);
addAsset(entity);
} else {
addDependency(entity.uri);
}
addDependency(entity.uri);
}
} on FileSystemException catch (e) {
throw FileSystemException(
Expand All @@ -63,14 +71,7 @@ extension BuildOutputBuilderAddDataAssetsDirectories on BuildOutputBuilder {
);
}
} else if (await file.exists()) {
assets.data.add(
DataAsset(
package: input.packageName,
name: assetName(file.uri),
file: file.uri,
),
);
addDependency(file.uri);
addAsset(file);
} else {
throw FileSystemException(
'Path does not exist',
Expand Down Expand Up @@ -140,10 +141,18 @@ final class LinkOutputDataAssetsBuilder {
LinkOutputDataAssetsBuilder(this._output);

/// Adds the given [asset] to the link hook output.
void add(DataAsset asset) => _output.addEncodedAsset(asset.encode());
void add(DataAsset asset, {LinkAssetRouting routing = const ToAppBundle()}) =>
_output.addEncodedAsset(asset.encode(), routing: routing);

/// Adds the given [assets] to the link hook output.
void addAll(Iterable<DataAsset> assets) => assets.forEach(add);
void addAll(
Iterable<DataAsset> assets, {
LinkAssetRouting routing = const ToAppBundle(),
}) {
for (final asset in assets) {
add(asset, routing: routing);
}
}
}

/// Provides access to [DataAsset]s from a build hook output.
Expand Down
2 changes: 1 addition & 1 deletion pkgs/data_assets/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: >-
This library contains the hook protocol specification for bundling data assets
with Dart packages.

version: 0.19.2-wip
version: 0.20.0-wip

repository: https://github.com/dart-lang/native/tree/main/pkgs/data_assets

Expand Down
2 changes: 2 additions & 0 deletions pkgs/hooks/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

* **Breaking change** Rename `EncodedAsset.jsonPath` to
`EncodedAsset.encodingJsonPath`. This field only governs the `EncodedAsset.encoding` field, not the whole object.
* Enable passing metadata from link hooks of a package to the link hooks in
dependencies, by fixing the link hook execution order.

## 0.19.5

Expand Down
25 changes: 16 additions & 9 deletions pkgs/hooks/doc/schema/shared/shared_definitions.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,6 @@
"items": {
"$ref": "#/definitions/Asset"
}
},
"assets_for_linking": {
"type": "object",
"additionalProperties": {
"type": "array",
"items": {
"$ref": "#/definitions/Asset"
}
}
}
},
"allOf": [
Expand Down Expand Up @@ -206,6 +197,15 @@
"$ref": "#/definitions/Asset"
}
},
"assets_for_linking": {
"type": "object",
"additionalProperties": {
"type": "array",
"items": {
"$ref": "#/definitions/Asset"
}
}
},
"dependencies": {
"type": "array",
"items": {
Expand Down Expand Up @@ -250,6 +250,12 @@
"$ref": "#/definitions/Asset"
}
},
"assets_from_linking": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please cover this in pkgs/hooks/test/json_schema/schema_test.dart
Also, the guard for assets_for_linking should not skip link output anymore.

You'll need to also add test data for the schema in pkgs/hooks/test/data

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand that test - is there documentation for what it is testing somewhere? Or do you know what has to be changed to make it pass?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test loads data, checks no validation errors, then modifies the data, and checks that the validation finds an error.

All the tests print the schema and the data file used in the test name:

file:///Users/dacoharkes/src/dart-lang/native/pkgs/hooks/doc/schema/hook/link_input.generated.schema.json file:///Users/dacoharkes/src/dart-lang/native/pkgs/hooks/test/data/link_input.json assets.assets_from_linking.1.encoding.key missing

You can run the tests in the debugger to figure out why they are failing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Or, eyeball it with git blame on the history of the test file 😆 )

"type": "array",
"items": {
"$ref": "#/definitions/Asset"
}
},
"resource_identifiers": {
"$ref": "#/definitions/absolutePath"
}
Expand All @@ -261,6 +267,7 @@
]
},
"LinkOutput": {
"type": "object",
"allOf": [
{
"$ref": "#/definitions/HookOutput"
Expand Down
2 changes: 2 additions & 0 deletions pkgs/hooks/lib/hooks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export 'src/config.dart'
HookOutputBuilder,
HookOutputFailure,
InfraError,
LinkAssetRouting,
LinkConfig,
LinkConfigBuilder,
LinkInput,
Expand All @@ -52,6 +53,7 @@ export 'src/config.dart'
LinkOutputBuilder,
LinkOutputFailure,
LinkOutputMaybeFailure,
LinkOutputMetadataBuilder,
PackageMetadata,
ToAppBundle,
ToBuildHooks,
Expand Down
Loading
Loading