diff --git a/.github/workflows/native.yaml b/.github/workflows/native.yaml index 26eb0676d..2aab3d265 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 b19c2a47d..15984a5c5 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 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); + 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 = 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); + 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( @@ -437,12 +436,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, @@ -461,20 +460,17 @@ class NativeAssetsBuildRunner { timeout: singleHookTimeout, logger: logger, () async { - final ( - compileSuccess, - hookKernelFile, - hookHashesFile, - ) = await _compileHookForPackageCached( + final hookCompileResult = await _compileHookForPackageCached( config.packageName, config.outputDirectory, config.packageRoot.resolve('hook/${hook.scriptName}'), packageConfigUri, workingDirectory, ); - if (!compileSuccess) { + if (hookCompileResult == null) { return null; } + final (hookKernelFile, hookHashes) = hookCompileResult; final buildOutputFile = File.fromUri(config.outputDirectory.resolve(hook.outputName)); @@ -510,7 +506,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}' @@ -533,12 +529,13 @@ ${e.message} if (await dependenciesHashFile.exists()) { await dependenciesHashFile.delete(); } + return null; } else { final modifiedDuringBuild = await dependenciesHashes.hashDependencies( [ ...result.dependencies, // Also depend on the hook source code. - hookHashesFile.uri, + hookHashes.file.uri, ], lastModifiedCutoffTime, environment, @@ -547,7 +544,7 @@ ${e.message} logger.severe('File modified during build. Build must be rerun.'); } } - return result; + return (result, hookHashes.fileSystemEntities); }, ); } @@ -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, File cacheFile)> + Future<(File kernelFile, DependenciesHashFile cacheFile)?> _compileHookForPackageCached( String packageName, Uri outputDirectory, @@ -721,7 +718,7 @@ ${e.message} } if (!mustCompile) { - return (true, kernelFile, dependenciesHashFile); + return (kernelFile, dependenciesHashes); } final success = await _compileHookForPackage( @@ -733,8 +730,7 @@ ${e.message} depFile, ); if (!success) { - await dependenciesHashFile.delete(); - return (success, kernelFile, dependenciesHashFile); + return null; } final dartSources = await _readDepFile(depFile); @@ -751,19 +747,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 (kernelFile, dependenciesHashes); } Future _compileHookForPackage( @@ -811,6 +795,12 @@ ${compileResult.stdout} ''', ); success = false; + if (await depFile.exists()) { + await depFile.delete(); + } + if (await kernelFile.exists()) { + await kernelFile.delete(); + } } return success; } @@ -927,3 +917,57 @@ ${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.substring(0, contents.indexOf(': ')); + contents = contents.substring(output.length + ': '.length).trim(); + 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++; + } + return result; +} + +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 2b1c49259..94449bf04 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 033d635db..655688a19 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 hookDependencies) { final mergedMaps = mergeMaps( encodedAssetsForLinking, hookOutput is BuildOutput @@ -61,6 +61,7 @@ final class HookResult implements BuildResult, BuildDryRunResult, LinkResult { dependencies: [ ...dependencies, ...hookOutput.dependencies, + ...hookDependencies, ]..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 fd248d658..5035e2e8d 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 851458b14..179ad5772 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 7b9566828..005affa26 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 825bbe66a..e9ac02e0a 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 000000000..02897123e --- /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', + ], + ); + }); +}