From 36da1b52c479ae8045f763034a45912a92cecf6d Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Tue, 3 Dec 2024 18:20:27 +0100 Subject: [PATCH 1/2] [native_assets_builder] Report dart sources as dependencies of hooks --- .github/workflows/native.yaml | 2 +- .../lib/src/build_runner/build_runner.dart | 83 ++++++++++++------- .../dependencies_hash_file.dart | 14 ++-- .../lib/src/model/hook_result.dart | 5 +- .../build_runner/build_dependencies_test.dart | 27 ++++-- .../build_runner_caching_test.dart | 8 +- .../build_runner_failure_test.dart | 8 +- .../build_runner_non_root_package_test.dart | 4 +- .../build_runner/parse_dep_file_test.dart | 20 +++++ 9 files changed, 118 insertions(+), 53 deletions(-) create mode 100644 pkgs/native_assets_builder/test/build_runner/parse_dep_file_test.dart diff --git a/.github/workflows/native.yaml b/.github/workflows/native.yaml index 26eb0676de..2aab3d2653 100644 --- a/.github/workflows/native.yaml +++ b/.github/workflows/native.yaml @@ -7,7 +7,7 @@ permissions: read-all on: pull_request: - branches: [main] + # No `branches:` to enable stacked PRs on GitHub. paths: - ".github/workflows/native.yaml" - "pkgs/native_assets_builder/**" diff --git a/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart b/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart index b19c2a47d7..ae95e79000 100644 --- a/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart +++ b/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart @@ -157,7 +157,7 @@ class NativeAssetsBuildRunner { 'Build configuration for ${package.name} contains errors', errors); } - final hookOutput = await _runHookForPackageCached( + final (hookOutput, hookDeps) = await _runHookForPackageCached( Hook.build, config, (config, output) => @@ -168,7 +168,7 @@ class NativeAssetsBuildRunner { packageLayout, ); if (hookOutput == null) return null; - hookResult = hookResult.copyAdd(hookOutput); + hookResult = hookResult.copyAdd(hookOutput, hookDeps!); globalMetadata[package.name] = (hookOutput as BuildOutput).metadata; } @@ -259,7 +259,7 @@ class NativeAssetsBuildRunner { 'Link configuration for ${package.name} contains errors', errors); } - final hookOutput = await _runHookForPackageCached( + final (hookOutput, hookDeps) = await _runHookForPackageCached( Hook.link, config, (config, output) => @@ -270,7 +270,7 @@ class NativeAssetsBuildRunner { packageLayout, ); if (hookOutput == null) return null; - hookResult = hookResult.copyAdd(hookOutput); + hookResult = hookResult.copyAdd(hookOutput, hookDeps!); } final errors = await applicationAssetValidator(hookResult.encodedAssets); @@ -437,12 +437,12 @@ class NativeAssetsBuildRunner { ), ); if (buildOutput == null) return null; - hookResult = hookResult.copyAdd(buildOutput); + hookResult = hookResult.copyAdd(buildOutput, [/*dry run is not cached*/]); } return hookResult; } - Future _runHookForPackageCached( + Future<(HookOutput?, List?)> _runHookForPackageCached( Hook hook, HookConfig config, _HookValidator validator, @@ -464,7 +464,7 @@ class NativeAssetsBuildRunner { final ( compileSuccess, hookKernelFile, - hookHashesFile, + hookHashes, ) = await _compileHookForPackageCached( config.packageName, config.outputDirectory, @@ -473,7 +473,7 @@ class NativeAssetsBuildRunner { workingDirectory, ); if (!compileSuccess) { - return null; + return (null, null); } final buildOutputFile = @@ -497,7 +497,7 @@ ${hook.outputName} contained a format error. Contents: ${buildOutputFile.readAsStringSync()}. ${e.message} '''); - return null; + return (null, null); } final outdatedDependency = @@ -510,7 +510,7 @@ ${e.message} ); // All build flags go into [outDir]. Therefore we do not have to // check here whether the config is equal. - return output; + return (output, hookHashes.fileSystemEntities); } logger.info( 'Rerunning ${hook.name} for ${config.packageName}' @@ -538,7 +538,7 @@ ${e.message} [ ...result.dependencies, // Also depend on the hook source code. - hookHashesFile.uri, + hookHashes.file.uri, ], lastModifiedCutoffTime, environment, @@ -547,7 +547,7 @@ ${e.message} logger.severe('File modified during build. Build must be rerun.'); } } - return result; + return (result, hookHashes.fileSystemEntities); }, ); } @@ -685,7 +685,7 @@ ${e.message} /// /// TODO(https://github.com/dart-lang/native/issues/1578): Compile only once /// instead of per config. This requires more locking. - Future<(bool success, File kernelFile, File cacheFile)> + Future<(bool success, File kernelFile, DependenciesHashFile cacheFile)> _compileHookForPackageCached( String packageName, Uri outputDirectory, @@ -721,7 +721,7 @@ ${e.message} } if (!mustCompile) { - return (true, kernelFile, dependenciesHashFile); + return (true, kernelFile, dependenciesHashes); } final success = await _compileHookForPackage( @@ -734,7 +734,7 @@ ${e.message} ); if (!success) { await dependenciesHashFile.delete(); - return (success, kernelFile, dependenciesHashFile); + return (success, kernelFile, dependenciesHashes); } final dartSources = await _readDepFile(depFile); @@ -751,19 +751,7 @@ ${e.message} logger.severe('File modified during build. Build must be rerun.'); } - return (success, kernelFile, dependenciesHashFile); - } - - Future> _readDepFile(File depFile) async { - // Format: `path/to/my.dill: path/to/my.dart, path/to/more.dart` - final depFileContents = await depFile.readAsString(); - final dartSources = depFileContents - .trim() - .split(' ') - .skip(1) // ':' - .map(Uri.file) - .toList(); - return dartSources; + return (success, kernelFile, dependenciesHashes); } Future _compileHookForPackage( @@ -927,3 +915,42 @@ ${compileResult.stdout} extension on Uri { Uri get parent => File(toFilePath()).parent.uri; } + +/// Parses depfile contents. +/// +/// Format: `path/to/my.dill: path/to/my.dart, path/to/more.dart` +/// +/// However, the spaces in paths are escaped with backslashes, and the +/// backslashes are escaped with backslashes: +/// +/// ```dart +/// String _escapePath(String path) { +/// return path.replaceAll('\\', '\\\\').replaceAll(' ', '\\ '); +/// } +/// ``` +List parseDepFileInputs(String contents) { + final output = contents.split(': ').first; + contents = contents.substring(output.length + ': '.length).trim(); + final inputs = []; + var currentWord = ''; + var escape = false; + for (var i = 0; i < contents.length; i++) { + if (contents[i] == r'\' && !escape) { + escape = true; + } else if (contents[i] == ' ' && !escape) { + inputs.add(currentWord); + currentWord = ''; + } else { + currentWord += contents[i]; + escape = false; + } + } + inputs.add(currentWord); + return inputs; +} + +Future> _readDepFile(File depFile) async { + final depFileContents = await depFile.readAsString(); + final dartSources = parseDepFileInputs(depFileContents); + return dartSources.map(Uri.file).toList(); +} diff --git a/pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart b/pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart index 2b1c49259f..94449bf049 100644 --- a/pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart +++ b/pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart @@ -13,19 +13,21 @@ import '../utils/uri.dart'; class DependenciesHashFile { DependenciesHashFile({ - required File file, - }) : _file = file; + required this.file, + }); - final File _file; + final File file; FileSystemHashes _hashes = FileSystemHashes(); + List get fileSystemEntities => _hashes.files.map((e) => e.path).toList(); + Future _readFile() async { - if (!await _file.exists()) { + if (!await file.exists()) { _hashes = FileSystemHashes(); return; } final jsonObject = - (json.decode(utf8.decode(await _file.readAsBytes())) as Map) + (json.decode(utf8.decode(await file.readAsBytes())) as Map) .cast(); _hashes = FileSystemHashes.fromJson(jsonObject); } @@ -70,7 +72,7 @@ class DependenciesHashFile { return modifiedAfterTimeStamp; } - Future _persist() => _file.writeAsString(json.encode(_hashes.toJson())); + Future _persist() => file.writeAsString(json.encode(_hashes.toJson())); /// Reads the file with hashes and reports if there is an outdated file, /// directory or environment variable. diff --git a/pkgs/native_assets_builder/lib/src/model/hook_result.dart b/pkgs/native_assets_builder/lib/src/model/hook_result.dart index 033d635db1..7f3f9de9f0 100644 --- a/pkgs/native_assets_builder/lib/src/model/hook_result.dart +++ b/pkgs/native_assets_builder/lib/src/model/hook_result.dart @@ -39,7 +39,7 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult { dependencies: dependencies ?? [], ); - HookResult copyAdd(HookOutput hookOutput) { + HookResult copyAdd(HookOutput hookOutput, List dependencies) { final mergedMaps = mergeMaps( encodedAssetsForLinking, hookOutput is BuildOutput @@ -59,8 +59,9 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult { ], encodedAssetsForLinking: mergedMaps, dependencies: [ - ...dependencies, + ...this.dependencies, ...hookOutput.dependencies, + ...dependencies, ]..sort(_uriCompare), ); } diff --git a/pkgs/native_assets_builder/test/build_runner/build_dependencies_test.dart b/pkgs/native_assets_builder/test/build_runner/build_dependencies_test.dart index fd248d658f..5035e2e8de 100644 --- a/pkgs/native_assets_builder/test/build_runner/build_dependencies_test.dart +++ b/pkgs/native_assets_builder/test/build_runner/build_dependencies_test.dart @@ -50,13 +50,28 @@ void main() async { expect(result.encodedAssets.length, 2); expect( result.dependencies, - [ - tempUri.resolve('native_add/').resolve('src/native_add.c'), - tempUri - .resolve('native_subtract/') - .resolve('src/native_subtract.c'), - ], + containsAll([ + tempUri.resolve('native_add/src/native_add.c'), + tempUri.resolve('native_subtract/src/native_subtract.c'), + if (!Platform.isWindows) ...[ + tempUri.resolve('native_add/hook/build.dart'), + tempUri.resolve('native_subtract/hook/build.dart'), + ], + ]), ); + if (Platform.isWindows) { + expect( + // https://github.com/dart-lang/sdk/issues/59657 + // Deps file on windows sometimes have lowercase drive letters. + // File.exists will work, but Uri equality doesn't. + result.dependencies + .map((e) => Uri.file(e.toFilePath().toLowerCase())), + containsAll([ + tempUri.resolve('native_add/hook/build.dart'), + tempUri.resolve('native_subtract/hook/build.dart'), + ].map((e) => Uri.file(e.toFilePath().toLowerCase()))), + ); + } } }); }); diff --git a/pkgs/native_assets_builder/test/build_runner/build_runner_caching_test.dart b/pkgs/native_assets_builder/test/build_runner/build_runner_caching_test.dart index 851458b142..179ad57729 100644 --- a/pkgs/native_assets_builder/test/build_runner/build_runner_caching_test.dart +++ b/pkgs/native_assets_builder/test/build_runner/build_runner_caching_test.dart @@ -44,9 +44,9 @@ void main() async { ); expect( result.dependencies, - [ + contains( packageUri.resolve('src/native_add.c'), - ], + ), ); } @@ -80,9 +80,9 @@ void main() async { ); expect( result.dependencies, - [ + contains( packageUri.resolve('src/native_add.c'), - ], + ), ); } }); diff --git a/pkgs/native_assets_builder/test/build_runner/build_runner_failure_test.dart b/pkgs/native_assets_builder/test/build_runner/build_runner_failure_test.dart index 7b9566828d..005affa26e 100644 --- a/pkgs/native_assets_builder/test/build_runner/build_runner_failure_test.dart +++ b/pkgs/native_assets_builder/test/build_runner/build_runner_failure_test.dart @@ -39,9 +39,9 @@ void main() async { symbols: ['add']); expect( result.dependencies, - [ + contains( packageUri.resolve('src/native_add.c'), - ], + ), ); } @@ -95,9 +95,9 @@ void main() async { symbols: ['add']); expect( result.dependencies, - [ + contains( packageUri.resolve('src/native_add.c'), - ], + ), ); } }); diff --git a/pkgs/native_assets_builder/test/build_runner/build_runner_non_root_package_test.dart b/pkgs/native_assets_builder/test/build_runner/build_runner_non_root_package_test.dart index 825bbe66a3..e9ac02e0a6 100644 --- a/pkgs/native_assets_builder/test/build_runner/build_runner_non_root_package_test.dart +++ b/pkgs/native_assets_builder/test/build_runner/build_runner_non_root_package_test.dart @@ -55,9 +55,9 @@ void main() async { expect(result.encodedAssets, isNotEmpty); expect( result.dependencies, - [ + contains( packageUri.resolve('src/native_add.c'), - ], + ), ); expect( logMessages.join('\n'), diff --git a/pkgs/native_assets_builder/test/build_runner/parse_dep_file_test.dart b/pkgs/native_assets_builder/test/build_runner/parse_dep_file_test.dart new file mode 100644 index 0000000000..02897123e6 --- /dev/null +++ b/pkgs/native_assets_builder/test/build_runner/parse_dep_file_test.dart @@ -0,0 +1,20 @@ +// Copyright (c) 2023, 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 'package:native_assets_builder/src/build_runner/build_runner.dart'; +import 'package:test/test.dart'; + +void main() { + test('parseDepFileInputs', () { + expect( + parseDepFileInputs( + r'C:\\Program\ Files\\foo.dill: C:\\Program\ Files\\foo.dart C:\\Program\ Files\\bar.dart', + ), + [ + r'C:\Program Files\foo.dart', + r'C:\Program Files\bar.dart', + ], + ); + }); +} From 59ba0b7772e096e73fbfce4678892211f8e43592 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Fri, 6 Dec 2024 12:42:21 +0100 Subject: [PATCH 2/2] address comments --- .../lib/src/build_runner/build_runner.dart | 99 +++++++++++-------- .../lib/src/model/hook_result.dart | 6 +- 2 files changed, 61 insertions(+), 44 deletions(-) diff --git a/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart b/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart index ae95e79000..15984a5c56 100644 --- a/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart +++ b/pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart @@ -157,7 +157,7 @@ class NativeAssetsBuildRunner { 'Build configuration for ${package.name} contains errors', errors); } - final (hookOutput, hookDeps) = await _runHookForPackageCached( + final result = await _runHookForPackageCached( Hook.build, config, (config, output) => @@ -167,8 +167,9 @@ class NativeAssetsBuildRunner { null, packageLayout, ); - if (hookOutput == null) return null; - hookResult = hookResult.copyAdd(hookOutput, hookDeps!); + if (result == null) return null; + final (hookOutput, hookDeps) = result; + hookResult = hookResult.copyAdd(hookOutput, hookDeps); globalMetadata[package.name] = (hookOutput as BuildOutput).metadata; } @@ -259,7 +260,7 @@ class NativeAssetsBuildRunner { 'Link configuration for ${package.name} contains errors', errors); } - final (hookOutput, hookDeps) = await _runHookForPackageCached( + final result = await _runHookForPackageCached( Hook.link, config, (config, output) => @@ -269,8 +270,9 @@ class NativeAssetsBuildRunner { resourceIdentifiers, packageLayout, ); - if (hookOutput == null) return null; - hookResult = hookResult.copyAdd(hookOutput, hookDeps!); + if (result == null) return null; + final (hookOutput, hookDeps) = result; + hookResult = hookResult.copyAdd(hookOutput, hookDeps); } final errors = await applicationAssetValidator(hookResult.encodedAssets); @@ -403,18 +405,15 @@ class NativeAssetsBuildRunner { final config = BuildConfig(configBuilder.json); final packageConfigUri = packageLayout.packageConfigUri; - final ( - compileSuccess, - hookKernelFile, - _, - ) = await _compileHookForPackageCached( + final hookCompileResult = await _compileHookForPackageCached( config.packageName, config.outputDirectory, config.packageRoot.resolve('hook/${hook.scriptName}'), packageConfigUri, workingDirectory, ); - if (!compileSuccess) return null; + if (hookCompileResult == null) return null; + final (hookKernelFile, _) = hookCompileResult; // TODO(https://github.com/dart-lang/native/issues/1321): Should dry runs be cached? final buildOutput = await runUnderDirectoriesLock( @@ -442,7 +441,7 @@ class NativeAssetsBuildRunner { return hookResult; } - Future<(HookOutput?, List?)> _runHookForPackageCached( + Future<(HookOutput, List)?> _runHookForPackageCached( Hook hook, HookConfig config, _HookValidator validator, @@ -461,20 +460,17 @@ class NativeAssetsBuildRunner { timeout: singleHookTimeout, logger: logger, () async { - final ( - compileSuccess, - hookKernelFile, - hookHashes, - ) = await _compileHookForPackageCached( + final hookCompileResult = await _compileHookForPackageCached( config.packageName, config.outputDirectory, config.packageRoot.resolve('hook/${hook.scriptName}'), packageConfigUri, workingDirectory, ); - if (!compileSuccess) { - return (null, null); + if (hookCompileResult == null) { + return null; } + final (hookKernelFile, hookHashes) = hookCompileResult; final buildOutputFile = File.fromUri(config.outputDirectory.resolve(hook.outputName)); @@ -497,7 +493,7 @@ ${hook.outputName} contained a format error. Contents: ${buildOutputFile.readAsStringSync()}. ${e.message} '''); - return (null, null); + return null; } final outdatedDependency = @@ -533,6 +529,7 @@ ${e.message} if (await dependenciesHashFile.exists()) { await dependenciesHashFile.delete(); } + return null; } else { final modifiedDuringBuild = await dependenciesHashes.hashDependencies( [ @@ -685,7 +682,7 @@ ${e.message} /// /// TODO(https://github.com/dart-lang/native/issues/1578): Compile only once /// instead of per config. This requires more locking. - Future<(bool success, File kernelFile, DependenciesHashFile cacheFile)> + Future<(File kernelFile, DependenciesHashFile cacheFile)?> _compileHookForPackageCached( String packageName, Uri outputDirectory, @@ -721,7 +718,7 @@ ${e.message} } if (!mustCompile) { - return (true, kernelFile, dependenciesHashes); + return (kernelFile, dependenciesHashes); } final success = await _compileHookForPackage( @@ -733,8 +730,7 @@ ${e.message} depFile, ); if (!success) { - await dependenciesHashFile.delete(); - return (success, kernelFile, dependenciesHashes); + return null; } final dartSources = await _readDepFile(depFile); @@ -751,7 +747,7 @@ ${e.message} logger.severe('File modified during build. Build must be rerun.'); } - return (success, kernelFile, dependenciesHashes); + return (kernelFile, dependenciesHashes); } Future _compileHookForPackage( @@ -799,6 +795,12 @@ ${compileResult.stdout} ''', ); success = false; + if (await depFile.exists()) { + await depFile.delete(); + } + if (await kernelFile.exists()) { + await kernelFile.delete(); + } } return success; } @@ -929,24 +931,39 @@ extension on Uri { /// } /// ``` List parseDepFileInputs(String contents) { - final output = contents.split(': ').first; + final output = contents.substring(0, contents.indexOf(': ')); contents = contents.substring(output.length + ': '.length).trim(); - final inputs = []; - var currentWord = ''; - var escape = false; - for (var i = 0; i < contents.length; i++) { - if (contents[i] == r'\' && !escape) { - escape = true; - } else if (contents[i] == ' ' && !escape) { - inputs.add(currentWord); - currentWord = ''; - } else { - currentWord += contents[i]; - escape = false; + final pathsEscaped = _splitOnNonEscapedSpaces(contents); + return pathsEscaped.map(_unescapeDepsPath).toList(); +} + +String _unescapeDepsPath(String path) => + path.replaceAll(r'\ ', ' ').replaceAll(r'\\', r'\'); + +List _splitOnNonEscapedSpaces(String contents) { + var index = 0; + final result = []; + while (index < contents.length) { + final start = index; + while (index < contents.length) { + final u = contents.codeUnitAt(index); + if (u == ' '.codeUnitAt(0)) { + break; + } + if (u == r'\'.codeUnitAt(0)) { + index++; + if (index == contents.length) { + throw const FormatException('malformed, ending with backslash'); + } + final v = contents.codeUnitAt(index); + assert(v == ' '.codeUnitAt(0) || v == r'\'.codeUnitAt(0)); + } + index++; } + result.add(contents.substring(start, index)); + index++; } - inputs.add(currentWord); - return inputs; + return result; } Future> _readDepFile(File depFile) async { diff --git a/pkgs/native_assets_builder/lib/src/model/hook_result.dart b/pkgs/native_assets_builder/lib/src/model/hook_result.dart index 7f3f9de9f0..655688a190 100644 --- a/pkgs/native_assets_builder/lib/src/model/hook_result.dart +++ b/pkgs/native_assets_builder/lib/src/model/hook_result.dart @@ -39,7 +39,7 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult { dependencies: dependencies ?? [], ); - HookResult copyAdd(HookOutput hookOutput, List dependencies) { + HookResult copyAdd(HookOutput hookOutput, List hookDependencies) { final mergedMaps = mergeMaps( encodedAssetsForLinking, hookOutput is BuildOutput @@ -59,9 +59,9 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult { ], encodedAssetsForLinking: mergedMaps, dependencies: [ - ...this.dependencies, - ...hookOutput.dependencies, ...dependencies, + ...hookOutput.dependencies, + ...hookDependencies, ]..sort(_uriCompare), ); }