Skip to content

[native_assets_builder] Use file content hashing #1750

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

Merged
merged 5 commits into from
Nov 28, 2024
Merged

[native_assets_builder] Use file content hashing #1750

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
58b6279
[native_assets_builder] Use XXH3 hashing
dcharkes Nov 26, 2024
b6466a6
address comment
dcharkes Nov 27, 2024
a7b05fb
use md5 hashing
dcharkes Nov 27, 2024
c54b262
fix var name
dcharkes Nov 27, 2024
0f89668
remove dep
dcharkes Nov 28, 2024
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
109 changes: 68 additions & 41 deletions pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ import 'package:logging/logging.dart';
import 'package:native_assets_cli/native_assets_cli_internal.dart';
import 'package:package_config/package_config.dart';

import '../dependencies_hash_file/dependencies_hash_file.dart';
import '../locking/locking.dart';
import '../model/build_dry_run_result.dart';
import '../model/build_result.dart';
import '../model/hook_result.dart';
import '../model/link_result.dart';
import '../package_layout/package_layout.dart';
import '../utils/file.dart';
import '../utils/run_process.dart';
import '../utils/uri.dart';
import 'build_planner.dart';

typedef DependencyMetadata = Map<String, Metadata>;
Expand Down Expand Up @@ -451,6 +450,8 @@ class NativeAssetsBuildRunner {
return hookResult;
}

// TODO(https://github.com/dart-lang/native/issues/32): Rerun hook if
// environment variables change.
Future<HookOutput?> _runHookForPackageCached(
Hook hook,
HookConfig config,
Expand All @@ -473,7 +474,7 @@ class NativeAssetsBuildRunner {
final (
compileSuccess,
hookKernelFile,
hookLastSourceChange,
hookHashesFile,
) = await _compileHookForPackageCached(
config.packageName,
config.outputDirectory,
Expand All @@ -488,7 +489,14 @@ class NativeAssetsBuildRunner {

final buildOutputFile =
File.fromUri(config.outputDirectory.resolve(hook.outputName));
if (buildOutputFile.existsSync()) {
final dependenciesHashFile = File.fromUri(
config.outputDirectory
.resolve('../dependencies.dependencies_hash_file.json'),
);
final dependenciesHashes =
DependenciesHashFile(file: dependenciesHashFile);
final lastModifiedCutoffTime = DateTime.now();
if (buildOutputFile.existsSync() && dependenciesHashFile.existsSync()) {
late final HookOutput output;
try {
output = _readHookOutputFromUri(hook, buildOutputFile);
Expand All @@ -503,17 +511,13 @@ ${e.message}
return null;
}

final lastBuilt = output.timestamp.roundDownToSeconds();
final dependenciesLastChange =
await Dependencies(output.dependencies).lastModified();
if (lastBuilt.isAfter(dependenciesLastChange) &&
lastBuilt.isAfter(hookLastSourceChange)) {
final outdated =
(await dependenciesHashes.findOutdatedFileSystemEntity()) != null;
if (!outdated) {
logger.info(
[
'Skipping ${hook.name} for ${config.packageName} in $outDir.',
'Last build on $lastBuilt.',
'Last dependencies change on $dependenciesLastChange.',
'Last hook change on $hookLastSourceChange.',
'Last build on ${output.timestamp}.',
].join(' '),
);
// All build flags go into [outDir]. Therefore we do not have to
Expand All @@ -522,7 +526,7 @@ ${e.message}
}
}

return await _runHookForPackage(
final result = await _runHookForPackage(
hook,
config,
validator,
Expand All @@ -533,6 +537,24 @@ ${e.message}
hookKernelFile,
packageLayout,
);
if (result == null) {
if (await dependenciesHashFile.exists()) {
await dependenciesHashFile.delete();
}
} else {
final modifiedDuringBuild = await dependenciesHashes.hashFiles(
[
...result.dependencies,
// Also depend on the hook source code.
hookHashesFile.uri,
],
validBeforeLastModified: lastModifiedCutoffTime,
);
if (modifiedDuringBuild != null) {
logger.severe('File modified during build. Build must be rerun.');
}
}
return result;
},
);
}
Expand Down Expand Up @@ -644,7 +666,10 @@ ${e.message}
/// It does not reuse the cached kernel for different configs due to
/// reentrancy requirements. For more info see:
/// https://github.com/dart-lang/native/issues/1319
Future<(bool success, File kernelFile, DateTime lastSourceChange)>
///
/// 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)>
_compileHookForPackageCached(
String packageName,
Uri outputDirectory,
Expand All @@ -659,29 +684,17 @@ ${e.message}
final depFile = File.fromUri(
outputDirectory.resolve('../hook.dill.d'),
);
final dependenciesHashFile = File.fromUri(
outputDirectory.resolve('../hook.dependencies_hash_file.json'),
Copy link
Member

Choose a reason for hiding this comment

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

Will we re-build the kernel file if the Dart SDK has changed? (otherwise invoking the old kernel on newer SDK may just crash)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is why you review the code! 🤓 🙏

);
final dependenciesHashes = DependenciesHashFile(file: dependenciesHashFile);
final lastModifiedCutoffTime = DateTime.now();
final bool mustCompile;
final DateTime sourceLastChange;
if (!await depFile.exists()) {
if (!await dependenciesHashFile.exists()) {
mustCompile = true;
sourceLastChange = DateTime.now();
} else {
// Format: `path/to/my.dill: path/to/my.dart, path/to/more.dart`
final depFileContents = await depFile.readAsString();
final dartSourceFiles = depFileContents
.trim()
.split(' ')
.skip(1) // '<kernel file>:'
.map((u) => Uri.file(u).fileSystemEntity)
.toList();
final dartFilesLastChange = await dartSourceFiles.lastModified();
final packageConfigLastChange =
await packageConfigUri.fileSystemEntity.lastModified();
sourceLastChange = packageConfigLastChange.isAfter(dartFilesLastChange)
? packageConfigLastChange
: dartFilesLastChange;
final kernelLastChange = await kernelFile.lastModified();
mustCompile = sourceLastChange == kernelLastChange ||
sourceLastChange.isAfter(kernelLastChange);
mustCompile =
(await dependenciesHashes.findOutdatedFileSystemEntity()) != null;
Copy link
Member

Choose a reason for hiding this comment

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

Consider logging the file name that caused to re-compile it.

}
final bool success;
if (!mustCompile) {
Expand All @@ -696,8 +709,28 @@ ${e.message}
kernelFile,
depFile,
);

if (success) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Consider using early returns and less nesting (on the lines above) - it makes it easier to read this code:

if (!mustCompile) {
  return (true, kernelFile, dependenciesHashFile);
}

final success = await _compileHookForPackage(...):
if (!success) {
  return (false, ...);
}

...
return (true, ...);

// Format: `path/to/my.dill: path/to/my.dart, path/to/more.dart`
final depFileContents = await depFile.readAsString();
Copy link
Member

Choose a reason for hiding this comment

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

nit: Consider moving this code (that knows about the format of .d files - into it's own function - e.g. parseDepFile(<file-content>))

final dartSources = depFileContents
.trim()
.split(' ')
.skip(1) // '<kernel file>:'
.map(Uri.file)
.toList();
final modifiedDuringBuild = await dependenciesHashes.hashFiles(
dartSources,
validBeforeLastModified: lastModifiedCutoffTime,
);
if (modifiedDuringBuild != null) {
logger.severe('File modified during build. Build must be rerun.');
}
} else {
await dependenciesHashFile.delete();
}
}
return (success, kernelFile, sourceLastChange);
return (success, kernelFile, dependenciesHashFile);
}

Future<bool> _compileHookForPackage(
Expand Down Expand Up @@ -859,12 +892,6 @@ ${compileResult.stdout}
}
}

extension on DateTime {
DateTime roundDownToSeconds() =>
DateTime.fromMillisecondsSinceEpoch(millisecondsSinceEpoch -
millisecondsSinceEpoch % const Duration(seconds: 1).inMilliseconds);
}

extension on Uri {
Uri get parent => File(toFilePath()).parent.uri;
}
Loading