Skip to content

Commit

Permalink
[native_assets_builder] Don't cache if env vars change
Browse files Browse the repository at this point in the history
  • Loading branch information
dcharkes committed Nov 27, 2024
1 parent c54b262 commit 719460b
Show file tree
Hide file tree
Showing 10 changed files with 82 additions and 148 deletions.
40 changes: 17 additions & 23 deletions pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:async';
import 'dart:convert';
import 'dart:io';

import 'package:collection/collection.dart';
import 'package:logging/logging.dart';
import 'package:native_assets_cli/native_assets_cli_internal.dart';
import 'package:package_config/package_config.dart';
Expand Down Expand Up @@ -96,7 +97,6 @@ class NativeAssetsBuildRunner {
required OS targetOS,
required BuildMode buildMode,
required Uri workingDirectory,
required bool includeParentEnvironment,
PackageLayout? packageLayout,
String? runPackageName,
required List<String> supportedAssetTypes,
Expand Down Expand Up @@ -165,7 +165,6 @@ class NativeAssetsBuildRunner {
buildValidator(config as BuildConfig, output as BuildOutput),
packageLayout.packageConfigUri,
workingDirectory,
includeParentEnvironment,
null,
packageLayout,
);
Expand Down Expand Up @@ -205,7 +204,6 @@ class NativeAssetsBuildRunner {
required BuildMode buildMode,
required Uri workingDirectory,
required ApplicationAssetValidator applicationAssetValidator,
required bool includeParentEnvironment,
PackageLayout? packageLayout,
Uri? resourceIdentifiers,
String? runPackageName,
Expand Down Expand Up @@ -269,7 +267,6 @@ class NativeAssetsBuildRunner {
linkValidator(config as LinkConfig, output as LinkOutput),
packageLayout.packageConfigUri,
workingDirectory,
includeParentEnvironment,
resourceIdentifiers,
packageLayout,
);
Expand Down Expand Up @@ -330,7 +327,6 @@ class NativeAssetsBuildRunner {
required OS targetOS,
required Uri workingDirectory,
required bool linkingEnabled,
required bool includeParentEnvironment,
PackageLayout? packageLayout,
String? runPackageName,
required List<String> supportedAssetTypes,
Expand All @@ -341,7 +337,6 @@ class NativeAssetsBuildRunner {
validator: (HookConfig config, HookOutput output) =>
buildValidator(config as BuildConfig, output as BuildOutput),
workingDirectory: workingDirectory,
includeParentEnvironment: includeParentEnvironment,
packageLayout: packageLayout,
runPackageName: runPackageName,
supportedAssetTypes: supportedAssetTypes,
Expand All @@ -353,7 +348,6 @@ class NativeAssetsBuildRunner {
required _HookValidator validator,
required OS targetOS,
required Uri workingDirectory,
required bool includeParentEnvironment,
PackageLayout? packageLayout,
String? runPackageName,
required bool linkingEnabled,
Expand Down Expand Up @@ -420,7 +414,6 @@ class NativeAssetsBuildRunner {
config.packageRoot.resolve('hook/${hook.scriptName}'),
packageConfigUri,
workingDirectory,
includeParentEnvironment,
);
if (!compileSuccess) return null;

Expand All @@ -438,7 +431,6 @@ class NativeAssetsBuildRunner {
validator,
packageConfigUri,
workingDirectory,
includeParentEnvironment,
null,
hookKernelFile,
packageLayout!,
Expand All @@ -450,15 +442,12 @@ 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,
_HookValidator validator,
Uri packageConfigUri,
Uri workingDirectory,
bool includeParentEnvironment,
Uri? resources,
PackageLayout packageLayout,
) async {
Expand All @@ -481,7 +470,6 @@ class NativeAssetsBuildRunner {
config.packageRoot.resolve('hook/${hook.scriptName}'),
packageConfigUri,
workingDirectory,
includeParentEnvironment,
);
if (!compileSuccess) {
return null;
Expand All @@ -496,7 +484,12 @@ class NativeAssetsBuildRunner {
final dependenciesHashes =
DependenciesHashFile(file: dependenciesHashFile);
final lastModifiedCutoffTime = DateTime.now();
if (buildOutputFile.existsSync() && dependenciesHashFile.existsSync()) {
final environmentFile = File.fromUri(
config.outputDirectory.resolve('../environment.json'),
);
if (buildOutputFile.existsSync() &&
dependenciesHashFile.existsSync() &&
environmentFile.existsSync()) {
late final HookOutput output;
try {
output = _readHookOutputFromUri(hook, buildOutputFile);
Expand All @@ -511,9 +504,14 @@ ${e.message}
return null;
}

final outdated =
final dependenciesOutdated =
(await dependenciesHashes.findOutdatedFileSystemEntity()) != null;
if (!outdated) {
final environmentChanged = (!await environmentFile.exists()) ||
!const MapEquality<String, String>().equals(
(json.decode(await environmentFile.readAsString()) as Map)
.cast<String, String>(),
Platform.environment);
if (!dependenciesOutdated && !environmentChanged) {
logger.info(
[
'Skipping ${hook.name} for ${config.packageName} in $outDir.',
Expand All @@ -532,7 +530,6 @@ ${e.message}
validator,
packageConfigUri,
workingDirectory,
includeParentEnvironment,
resources,
hookKernelFile,
packageLayout,
Expand All @@ -542,6 +539,9 @@ ${e.message}
await dependenciesHashFile.delete();
}
} else {
await environmentFile.writeAsString(
json.encode(Platform.environment),
);
final modifiedDuringBuild = await dependenciesHashes.hashFiles(
[
...result.dependencies,
Expand All @@ -565,7 +565,6 @@ ${e.message}
_HookValidator validator,
Uri packageConfigUri,
Uri workingDirectory,
bool includeParentEnvironment,
Uri? resources,
File hookKernelFile,
PackageLayout packageLayout,
Expand Down Expand Up @@ -593,7 +592,6 @@ ${e.message}
executable: dartExecutable,
arguments: arguments,
logger: logger,
includeParentEnvironment: includeParentEnvironment,
);

var deleteOutputIfExists = false;
Expand Down Expand Up @@ -676,7 +674,6 @@ ${e.message}
Uri scriptUri,
Uri packageConfigUri,
Uri workingDirectory,
bool includeParentEnvironment,
) async {
final kernelFile = File.fromUri(
outputDirectory.resolve('../hook.dill'),
Expand Down Expand Up @@ -705,7 +702,6 @@ ${e.message}
scriptUri,
packageConfigUri,
workingDirectory,
includeParentEnvironment,
kernelFile,
depFile,
);
Expand Down Expand Up @@ -738,7 +734,6 @@ ${e.message}
Uri scriptUri,
Uri packageConfigUri,
Uri workingDirectory,
bool includeParentEnvironment,
File kernelFile,
File depFile,
) async {
Expand All @@ -755,7 +750,6 @@ ${e.message}
executable: dartExecutable,
arguments: compileArguments,
logger: logger,
includeParentEnvironment: includeParentEnvironment,
);
var success = true;
if (compileResult.exitCode != 0) {
Expand Down
18 changes: 1 addition & 17 deletions pkgs/native_assets_builder/lib/src/utils/run_process.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,11 @@ Future<RunProcessResult> runProcess({
List<String> arguments = const [],
Uri? workingDirectory,
Map<String, String>? environment,
bool includeParentEnvironment = true,
required Logger? logger,
bool captureOutput = true,
int expectedExitCode = 0,
bool throwOnUnexpectedExitCode = false,
}) async {
if (Platform.isWindows && !includeParentEnvironment) {
const winEnvKeys = [
'SYSTEMROOT',
'TEMP',
'TMP',
];
environment = {
for (final winEnvKey in winEnvKeys)
winEnvKey: Platform.environment[winEnvKey]!,
...?environment,
};
}

final printWorkingDir =
workingDirectory != null && workingDirectory != Directory.current.uri;
final commandString = [
Expand All @@ -56,9 +42,7 @@ Future<RunProcessResult> runProcess({
arguments,
workingDirectory: workingDirectory?.toFilePath(),
environment: environment,
includeParentEnvironment: includeParentEnvironment,
runInShell: Platform.isWindows &&
(!includeParentEnvironment || workingDirectory != null),
runInShell: Platform.isWindows && workingDirectory != null,
);

final stdoutSub = process.stdout
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// 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:convert';
import 'dart:io';

import 'package:test/test.dart';
Expand Down Expand Up @@ -198,4 +199,66 @@ void main() async {
});
},
);

test(
'change environment',
timeout: longTimeout,
() async {
await inTempDir((tempUri) async {
await copyTestProjects(targetUri: tempUri);
final packageUri = tempUri.resolve('native_add/');

final logMessages = <String>[];
final logger = createCapturingLogger(logMessages);

await runPubGet(workingDirectory: packageUri, logger: logger);
logMessages.clear();

final result = (await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
configValidator: validateCodeAssetBuildConfig,
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetInApplication,
))!;
logMessages.clear();

// Simulate that the environment variables changed by augmenting the
// persisted environment from the last invocation.
final environmentFile = File.fromUri(
CodeAsset.fromEncoded(result.encodedAssets.single)
.file!
.parent
.parent
.resolve('environment.json'),
);
expect(await environmentFile.exists(), true);
await environmentFile.writeAsString(jsonEncode({
...Platform.environment,
'SOME_KEY_THAT_IS_NOT_ALREADY_IN_THE_ENVIRONMENT': 'some_value',
}));

(await build(
packageUri,
logger,
dartExecutable,
supportedAssetTypes: [CodeAsset.type],
configValidator: validateCodeAssetBuildConfig,
buildValidator: validateCodeAssetBuildOutput,
applicationAssetValidator: validateCodeAssetInApplication,
))!;
expect(
logMessages.join('\n'),
contains('hook.dill'),
);
expect(
logMessages.join('\n'),
isNot(contains('Skipping build for native_add')),
);
logMessages.clear();
});
},
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ void main() async {
configCreator: configCreator,
targetOS: Target.current.os,
workingDirectory: packageUri,
includeParentEnvironment: true,
linkingEnabled: false,
supportedAssetTypes: [],
buildValidator: (config, output) async => [],
Expand All @@ -46,7 +45,6 @@ void main() async {
configCreator: configCreator,
targetOS: Target.current.os,
workingDirectory: packageUri,
includeParentEnvironment: true,
linkingEnabled: false,
supportedAssetTypes: [],
buildValidator: (config, output) async => [],
Expand All @@ -56,7 +54,6 @@ void main() async {
targetOS: OS.current,
buildMode: BuildMode.release,
workingDirectory: packageUri,
includeParentEnvironment: true,
linkingEnabled: false,
supportedAssetTypes: [],
configValidator: (config) async => [],
Expand All @@ -68,7 +65,6 @@ void main() async {
buildMode: BuildMode.release,
targetOS: OS.current,
workingDirectory: packageUri,
includeParentEnvironment: true,
linkingEnabled: false,
supportedAssetTypes: [],
configValidator: (config) async => [],
Expand Down
Loading

0 comments on commit 719460b

Please sign in to comment.