Merge pull request #426 from Workiva/compile_run_script_for_better_pe…

…rf_followup

Compile the `dart_dev` run script for better performance

CHANGELOG.md

@@ -1,5 +1,18 @@
 # Changelog
 
+## 4.2.0
+
+- Add a `clean` command by default that removes temporary files used by
+dart_dev, like the compiled version of the run script.
+- Use `dart compile exe` to compile the `.dart_tool/dart_dev/run.dart` script
+for better startup performance on subsequent runs. This compilation step will be
+cached until `tool/dart_dev/config.dart`, the installed packages, or the current
+Dart SDK is changed.
+
+## 4.1.1
+
+- Optimization pass to reduce filesystem iteration.
+
 ## 4.1.0
 
 - Raise maximum SDK to include Dart 3.

lib/src/dart_dev_runner.dart

@@ -2,21 +2,35 @@ import 'dart:async';
 
 import 'package:args/args.dart';
 import 'package:args/command_runner.dart';
+import 'package:dart_dev/dart_dev.dart';
 
 import 'dart_dev_tool.dart';
 import 'events.dart' as events;
+import 'tools/clean_tool.dart';
 import 'utils/version.dart';
 
 class DartDevRunner extends CommandRunner<int> {
   DartDevRunner(Map<String, DevTool> commands)
       : super('dart_dev', 'Dart tool runner.') {
+    // For backwards-compatibility, only add the `clean` command if it doesn't
+    // conflict with any configured command.
+    if (!commands.containsKey('clean')) {
+      // Construct a new commands map here, to work around a runtime typecheck
+      // failure:
+      // `type 'CleanTool' is not a subtype of type 'FormatTool' of 'value'`
+      // As seen in this CI run:
+      // https://github.com/Workiva/dart_dev/actions/runs/8161855516/job/22311519665?pr=426#step:8:295
+      commands = <String, DevTool>{...commands, 'clean': CleanTool()};
+    }
+
     commands.forEach((name, builder) {
       final command = builder.toCommand(name);
       if (command.name != name) {
         throw CommandNameMismatch(command.name, name);
       }
       addCommand(command);
     });
+
     argParser
       ..addFlag('verbose',
           abbr: 'v', negatable: false, help: 'Enables verbose logging.')

lib/src/executable.dart

@@ -1,13 +1,22 @@
 import 'dart:async';
+import 'dart:convert';
 import 'dart:io';
 
 import 'package:analyzer/dart/analysis/utilities.dart';
 import 'package:args/command_runner.dart';
+import 'package:collection/collection.dart';
+import 'package:crypto/crypto.dart';
 import 'package:dart_dev/dart_dev.dart';
+import 'package:dart_dev/src/utils/parse_imports.dart';
+import 'package:dart_dev/src/utils/pubspec_lock.dart';
+import 'package:glob/glob.dart';
+import 'package:glob/list_local_fs.dart';
 import 'package:io/ansi.dart';
 import 'package:io/io.dart' show ExitCode;
 import 'package:logging/logging.dart';
 import 'package:path/path.dart' as p;
+import 'package:pubspec_parse/pubspec_parse.dart';
+import 'package:yaml/yaml.dart';
 
 import 'dart_dev_runner.dart';
 import 'tools/over_react_format_tool.dart';
@@ -16,43 +25,44 @@ import 'utils/cached_pubspec.dart';
 import 'utils/dart_dev_paths.dart';
 import 'utils/dart_tool_cache.dart';
 import 'utils/ensure_process_exit.dart';
-import 'utils/executables.dart' as exe;
 import 'utils/format_tool_builder.dart';
 import 'utils/get_dart_version_comment.dart';
 import 'utils/logging.dart';
 import 'utils/parse_flag_from_args.dart';
 
 typedef _ConfigGetter = Map<String, DevTool> Function();
 
-final paths = DartDevPaths();
-
-final _runScript = File(paths.runScript);
+final _paths = DartDevPaths();
 
 Future<void> run(List<String> args) async {
   attachLoggerToStdio(args);
 
-  final configExists = File(paths.config).existsSync();
-  final oldDevDartExists = File(paths.legacyConfig).existsSync();
+  final configExists = File(_paths.config).existsSync();
+  final oldDevDartExists = File(_paths.legacyConfig).existsSync();
 
   if (!configExists) {
-    log.fine('No custom `${paths.config}` file found; '
+    log.fine('No custom `${_paths.config}` file found; '
         'using default config.');
   }
   if (oldDevDartExists) {
     log.warning(yellow.wrap(
-        'dart_dev v3 now expects configuration to be at `${paths.config}`,\n'
-        'but `${paths.legacyConfig}` still exists. View the guide to see how to upgrade:\n'
+        'dart_dev v3 now expects configuration to be at `${_paths.config}`,\n'
+        'but `${_paths.legacyConfig}` still exists. View the guide to see how to upgrade:\n'
         'https://github.com/Workiva/dart_dev/blob/master/doc/v3-upgrade-guide.md'));
   }
 
   if (args.contains('hackFastFormat') && !oldDevDartExists) {
     await handleFastFormat(args);
-
     return;
   }
 
-  generateRunScript();
-  final process = await Process.start(exe.dart, [paths.runScript, ...args],
+  final processArgs = generateRunScript();
+  final process = await Process.start(
+      processArgs.first,
+      [
+        if (processArgs.length > 1) ...processArgs.sublist(1),
+        ...args,
+      ],
       mode: ProcessStartMode.inheritStdio);
   ensureProcessExit(process);
   exitCode = await process.exitCode;
@@ -62,7 +72,7 @@ Future<void> handleFastFormat(List<String> args) async {
   assertDirIsDartPackage();
 
   DevTool? formatTool;
-  final configFile = File(paths.config);
+  final configFile = File(_paths.config);
   if (configFile.existsSync()) {
     final toolBuilder = FormatToolBuilder();
     parseString(content: configFile.readAsStringSync())
@@ -94,25 +104,142 @@ Future<void> handleFastFormat(List<String> args) async {
   }
 }
 
-void generateRunScript() {
-  if (shouldWriteRunScript) {
+void _deleteRunExecutableAndDigest() =>
+    [_paths.runExecutable, _paths.runExecutableDigest].forEach((p) {
+      final f = File(p);
+      if (f.existsSync()) f.deleteSync();
+    });
+
+/// Return true iff all the provided package names can be traced to 'hosted'
+/// entries in pubspec.lock.
+bool _allPackagesAreImportedImmutably(Iterable<String> packageNames) {
+  File pubspecLockFile = File('pubspec.lock');
+  if (!pubspecLockFile.existsSync()) return false;
+  final pubSpecLock = loadYamlDocument(pubspecLockFile.readAsStringSync());
+  return getDependencySources(pubSpecLock, packageNames)
+      .values
+      .every((source) => source == 'hosted' || source == 'git');
+}
+
+/// Return null iff it is not possible to account for all
+/// recompilation-necessitating factors in the digest.
+String? _computeRunScriptDigest() {
+  final currentPackageName =
+      Pubspec.parse(File('pubspec.yaml').readAsStringSync()).name;
+  final configFile = File(_paths.config);
+  var configHasRelativeImports = false;
+  if (configFile.existsSync()) {
+    final configImports = parseImports(configFile.readAsStringSync());
+    configHasRelativeImports = configImports.any((i) => !i.contains(':'));
+    final configHasSamePackageImports =
+        configImports.any((i) => i.startsWith('package:$currentPackageName'));
+
+    if (configHasSamePackageImports) {
+      log.fine(
+          'Skipping compilation because ${_paths.config} imports from its own package.');
+
+      // If the config imports from its own source files, we don't have a way of
+      // efficiently tracking changes that would require recompilation of this
+      // executable, so we skip the compilation altogether.
+      _deleteRunExecutableAndDigest();
+      return null;
+    }
+
+    final packageNames = computePackageNamesFromImports(configImports);
+    if (!_allPackagesAreImportedImmutably(packageNames)) {
+      log.fine(
+          'Skipping compilation because ${_paths.config} imports non-hosted packages.');
+      _deleteRunExecutableAndDigest();
+      return null;
+    }
+  }
+
+  // Include the packageConfig in the digest so that when dependencies change,
+  // we recompile.
+  final packageConfig = File(_paths.packageConfig);
+
+  final digest = md5.convert([
+    if (packageConfig.existsSync()) ...packageConfig.readAsBytesSync(),
+    if (configFile.existsSync()) ...configFile.readAsBytesSync(),
+    if (configHasRelativeImports)
+      for (final file in Glob('tool/**.dart', recursive: true)
+          .listSync()
+          .whereType<File>()
+          .where((f) => p.canonicalize(f.path) != p.canonicalize(_paths.config))
+          .sortedBy((f) => f.path))
+        ...file.readAsBytesSync(),
+  ]);
+  return base64.encode(digest.bytes);
+}
+
+List<String> generateRunScript() {
+  // Generate the run script if it doesn't yet exist or regenerate it if the
+  // existing script is outdated.
+  final runScriptContents = buildDartDevRunScriptContents();
+  final runScript = File(_paths.runScript);
+  final runExecutable = File(_paths.runExecutable);
+  final runExecutableDigest = File(_paths.runExecutableDigest);
+  if (!runScript.existsSync() ||
+      runScript.readAsStringSync() != runScriptContents) {
     logTimedSync(log, 'Generating run script', () {
       createCacheDir();
-      _runScript.writeAsStringSync(buildDartDevRunScriptContents());
+      runScript.writeAsStringSync(runScriptContents);
     }, level: Level.INFO);
   }
-}
 
-bool get shouldWriteRunScript =>
-    !_runScript.existsSync() ||
-    _runScript.readAsStringSync() != buildDartDevRunScriptContents();
+  // Generate a digest of inputs to the run script. We use this to determine
+  // whether we need to recompile the executable.
+  String? encodedDigest;
+  logTimedSync(log, 'Computing run script digest',
+      () => encodedDigest = _computeRunScriptDigest(),
+      level: Level.FINE);
+
+  if (encodedDigest != null &&
+      (!runExecutableDigest.existsSync() ||
+          runExecutableDigest.readAsStringSync() != encodedDigest)) {
+    // Digest is missing or outdated, so we (re-)compile.
+    final logMessage = runExecutable.existsSync()
+        ? 'Recompiling run script (digest changed)'
+        : 'Compiling run script';
+    logTimedSync(log, logMessage, () {
+      // Delete the previous executable and digest so that if we hit a failure
+      // trying to compile, we don't leave the outdated one in place.
+      _deleteRunExecutableAndDigest();
+      final args = [
+        'compile',
+        'exe',
+        _paths.runScript,
+        '-o',
+        _paths.runExecutable
+      ];
+      final result = Process.runSync(Platform.executable, args);
+      if (result.exitCode == 0) {
+        // Compilation succeeded. Write the new digest, as well.
+        runExecutableDigest.writeAsStringSync(encodedDigest!);
+      } else {
+        // Compilation failed. Dump some logs for debugging, but note to the
+        // user that dart_dev should still work.
+        log.warning(
+            'Could not compile run script; dart_dev will continue without precompilation.');
+        log.fine('CMD: ${Platform.executable} ${args.join(" ")}');
+        log.fine('STDOUT:\n${result.stdout}');
+        log.fine('STDERR:\n${result.stderr}');
+      }
+    });
+  }
+
+  return runExecutable.existsSync()
+      // Using the absolute path is necessary for Windows to find the executable.
+      ? [runExecutable.absolute.path]
+      : [Platform.executable, 'run', _paths.runScript];
+}
 
 String buildDartDevRunScriptContents() {
-  final hasCustomToolDevDart = File(paths.config).existsSync();
+  final hasCustomToolDevDart = File(_paths.config).existsSync();
   // If the config has a dart version comment (e.g., if it opts out of null safety),
   // copy it over to the entrypoint so the program is run in that language version.
   var dartVersionComment = hasCustomToolDevDart
-      ? getDartVersionComment(File(paths.config).readAsStringSync())
+      ? getDartVersionComment(File(_paths.config).readAsStringSync())
       : null;
 
   return '''
@@ -121,7 +248,7 @@ import 'dart:io';
 
 import 'package:dart_dev/src/core_config.dart';
 import 'package:dart_dev/src/executable.dart' as executable;
-${hasCustomToolDevDart ? "import '${paths.configFromRunScriptForDart}' as custom_dev;" : ""}
+${hasCustomToolDevDart ? "import '${_paths.configFromRunScriptForDart}' as custom_dev;" : ""}
 
 void main(List<String> args) async {
   await executable.runWithConfig(args,
@@ -149,7 +276,7 @@ Future<void> runWithConfig(
     config = configGetter();
   } catch (error) {
     stderr
-      ..writeln('Invalid "${paths.config}" in ${p.absolute(p.current)}')
+      ..writeln('Invalid "${_paths.config}" in ${p.absolute(p.current)}')
       ..writeln()
       ..writeln('It should provide a `Map<String, DevTool> config;` getter,'
           ' but it either does not exist or threw unexpectedly:')

lib/src/tools/clean_tool.dart

@@ -0,0 +1,25 @@
+import 'dart:async';
+import 'dart:io';
+
+import 'package:dart_dev/src/utils/logging.dart';
+import 'package:io/io.dart';
+
+import '../dart_dev_tool.dart';
+import '../utils/dart_dev_paths.dart' show DartDevPaths;
+
+class CleanTool extends DevTool {
+  @override
+  final String? description = 'Cleans up temporary files used by dart_dev.';
+
+  @override
+  FutureOr<int?> run([DevToolExecutionContext? context]) {
+    final cache = Directory(DartDevPaths().cache());
+    if (cache.existsSync()) {
+      log.info('Deleting ${cache.path}');
+      cache.deleteSync(recursive: true);
+    } else {
+      log.info('Nothing to do: no ${cache.path} found');
+    }
+    return ExitCode.success.code;
+  }
+}

lib/src/utils/dart_dev_paths.dart

@@ -25,7 +25,14 @@ class DartDevPaths {
         from: p.url.absolute(_cacheForDart),
       );
 
+  String get packageConfig =>
+      _context.join('.dart_tool', 'package_config.json');
+
   String get legacyConfig => _context.join('tool', 'dev.dart');
 
   String get runScript => cache('run.dart');
+
+  String get runExecutable => cache('run');
+
+  String get runExecutableDigest => cache('run.digest');
 }

lib/src/utils/parse_imports.dart

@@ -0,0 +1,18 @@
+import 'package:collection/collection.dart';
+
+/// Return the contents of the enquoted portion of the import statements in the
+/// file. Not 100% accurate, since we use regular expressions instead of the
+/// Dart AST to extract the imports.
+Iterable<String> parseImports(String fileContents) =>
+    _importRegex.allMatches(fileContents).map((m) => m.group(1)).whereNotNull();
+
+final _importRegex =
+    RegExp(r'''^import ['"]([^'"]+)['"];?$''', multiLine: true);
+
+/// Return a set of package names given a list of imports.
+Set<String> computePackageNamesFromImports(Iterable<String> imports) => imports
+    .map((i) => _packageNameFromImportRegex.matchAsPrefix(i)?.group(1))
+    .whereNotNull()
+    .toSet();
+
+final _packageNameFromImportRegex = RegExp(r'package:([^/]+)/.+');