-
Notifications
You must be signed in to change notification settings - Fork 69
[native_assets_cli] BuildOutput extension: addDataAssetDirectories #2097
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
Changes from 28 commits
80af89c
28e4dd2
ec9b98c
ba20b2d
80f7576
42d26a3
6795c87
703a5c5
6c91856
7da0b8f
303e701
4e99e19
d78a647
2836592
82a4b05
09cbb6b
a9b87b9
379a76e
b76ec8f
768827c
590811f
8f29e61
7b0d8fb
ca389e5
6423e27
56cbc0b
3cf0742
7719d15
a60dff5
af8218c
c21eba0
8034a81
7424003
7d6c35d
2c316ea
bc33b02
57d6d41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,11 @@ | ||
// Copyright (c) 2024, 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. | ||
import 'dart:io'; | ||
|
||
import 'package:native_assets_cli/data_assets.dart'; | ||
import 'package:native_assets_cli/native_assets_cli.dart'; | ||
|
||
void main(List<String> args) async { | ||
await build(args, (input, output) async { | ||
final assetDirectory = Directory.fromUri( | ||
input.packageRoot.resolve('assets/'), | ||
); | ||
// If assets are added, rerun hook. | ||
output.addDependency(assetDirectory.uri); | ||
|
||
await for (final dataAsset in assetDirectory.list()) { | ||
if (dataAsset is! File) { | ||
continue; | ||
} | ||
|
||
// The file path relative to the package root, with forward slashes. | ||
final name = dataAsset.uri | ||
.toFilePath(windows: false) | ||
.substring(input.packageRoot.toFilePath(windows: false).length); | ||
|
||
output.assets.data.add( | ||
DataAsset(package: input.packageName, name: name, file: dataAsset.uri), | ||
); | ||
// TODO(https://github.com/dart-lang/native/issues/1208): Report | ||
// dependency on asset. | ||
output.addDependency(dataAsset.uri); | ||
} | ||
await output.addDataAssetDirectories(['assets'], input: input); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,10 +8,10 @@ import 'dart:io'; | |
import 'package:crypto/crypto.dart' show sha256; | ||
import 'package:pub_semver/pub_semver.dart'; | ||
|
||
import '../data_assets.dart'; | ||
import 'api/deprecation_messages.dart'; | ||
import 'encoded_asset.dart'; | ||
import 'hook/syntax.g.dart' as syntax; | ||
import 'metadata.dart'; | ||
import 'utils/datetime.dart'; | ||
import 'utils/json.dart'; | ||
|
||
|
@@ -404,6 +404,78 @@ class BuildOutputBuilder extends HookOutputBuilder { | |
syntax.BuildOutput.fromJson(super._syntax.json); | ||
} | ||
|
||
extension AddDataAssetsDirectoryExtension on BuildOutputBuilder { | ||
MichealReed marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// Extension on [BuildOutput] to handle data asset directories and files. | ||
/// | ||
/// This extension provides a convenient way for build hooks to add | ||
/// [DataAsset] dependencies from one or more directories or individual files. | ||
MichealReed marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// If any specified path does not exist, a [FileSystemException] is thrown. | ||
/// Any error during the directory listing is caught and rethrown with | ||
/// additional context. | ||
/// | ||
/// When recursive is set to true, the method will also add all subdirectories | ||
/// and their files as dependencies. | ||
Future<void> addDataAssetDirectories( | ||
List<String> paths, { | ||
required BuildInput input, | ||
MichealReed marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bool recursive = false, | ||
}) async { | ||
final packageName = input.packageName; | ||
final packageRoot = input.packageRoot; | ||
final rootPath = packageRoot.toFilePath(windows: false); | ||
|
||
String assetName(Uri assetUri) => | ||
assetUri.toFilePath(windows: false).substring(rootPath.length); | ||
|
||
for (final path in paths) { | ||
final resolvedUri = packageRoot.resolve('$packageName/$path'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Files created in the tests were showing up under $packageName, is there something else moving the assets to this path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was kind of expecting this function to do what pkgs/native_assets_builder/test_data/simple_data_asset/hook/build.dart does, and that file being refactored to simply a call to this function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can test_data/simple_data_asset be tested? It depends on a nonexistent dart:asset and ByteAsset. Pushed a fix for name that mirrors that example, we still must resolve the relative $packageName/$path to find the file though. Do you want this to be refactored for compatibility with system absolute paths or relative only? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way to test for now is to run
I don't understand, if I change the helper function to
I think relative paths in the package is fine for now. That's the most common use case. (For desktop applications that use an LLM that's installed on the system, I can imagine wanting to support absolute paths. So we could add such support in the future. We should be able to distinguish a string containing an absolute path from a relative path.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be good now, the data assets validation tests makeDataBuildInput was not resolving the packageRoot correctly, which caused the failure in the validation test. Now both simple_data_asset and the validation tests pass with resolve(path). |
||
final directory = Directory.fromUri(resolvedUri); | ||
final file = File.fromUri(resolvedUri); | ||
|
||
if (await directory.exists()) { | ||
try { | ||
await for (final entity in directory.list( | ||
recursive: recursive, | ||
followLinks: false, | ||
)) { | ||
if (entity is File) { | ||
assets.data.add( | ||
DataAsset( | ||
package: packageName, | ||
name: assetName(entity.uri), | ||
file: entity.uri, | ||
), | ||
); | ||
addDependency(entity.uri); | ||
} | ||
} | ||
} on FileSystemException catch (e) { | ||
throw FileSystemException( | ||
'Error reading directory "$path": ${e.message}', | ||
directory.path, | ||
e.osError, | ||
); | ||
} | ||
} else if (await file.exists()) { | ||
assets.data.add( | ||
DataAsset( | ||
package: packageName, | ||
name: assetName(file.uri), | ||
file: file.uri, | ||
), | ||
); | ||
addDependency(file.uri); | ||
} else { | ||
throw FileSystemException( | ||
'Path does not exist', | ||
resolvedUri.toFilePath(windows: Platform.isWindows), | ||
); | ||
} | ||
} | ||
} | ||
} | ||
|
||
extension type EncodedAssetBuildOutputBuilder._(BuildOutputBuilder _output) { | ||
/// Adds [EncodedAsset]s produced by this build. | ||
/// | ||
|
Uh oh!
There was an error while loading. Please reload this page.