-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
Compile the dart_dev
run script for better performance
#426
Changes from 4 commits
924cf9d
a400944
69af6f2
b395a77
4f72e97
b887017
7ee6009
04cef7f
d4c1a59
500199f
2fc5874
3e0312f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is starting to have a lot of responsibilities. I could have seen the new digest-related functions in a separate file. 🤷 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,18 @@ | ||
import 'dart:async'; | ||
import 'dart:convert'; | ||
import 'dart:io'; | ||
|
||
import 'package:analyzer/dart/analysis/utilities.dart'; | ||
import 'package:args/command_runner.dart'; | ||
import 'package:crypto/crypto.dart'; | ||
import 'package:dart_dev/dart_dev.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 'dart_dev_runner.dart'; | ||
import 'tools/over_react_format_tool.dart'; | ||
|
@@ -16,43 +21,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, | ||
], | ||
Comment on lines
+60
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security review: parameterized shelling out is good. |
||
mode: ProcessStartMode.inheritStdio); | ||
ensureProcessExit(process); | ||
exitCode = await process.exitCode; | ||
|
@@ -62,7 +68,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 +100,109 @@ Future<void> handleFastFormat(List<String> args) async { | |
} | ||
} | ||
|
||
void generateRunScript() { | ||
if (shouldWriteRunScript) { | ||
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', () { | ||
var configHasRelativeImports = false; | ||
var configHasSamePackageImports = false; | ||
final configFile = File(_paths.config); | ||
if (configFile.existsSync()) { | ||
final contents = configFile.readAsStringSync(); | ||
configHasRelativeImports = | ||
RegExp(r'''^import ['"][^:]+$''', multiLine: true).hasMatch(contents); | ||
final currentPackageName = | ||
Pubspec.parse(File('pubspec.yaml').readAsStringSync()).name; | ||
configHasSamePackageImports = RegExp( | ||
'''^import ['"]package:$currentPackageName\/''', | ||
multiLine: true) | ||
.hasMatch(contents); | ||
} | ||
|
||
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. | ||
if (runExecutable.existsSync()) runExecutable.deleteSync(); | ||
if (runExecutableDigest.existsSync()) runExecutableDigest.deleteSync(); | ||
return; | ||
} | ||
|
||
final digest = md5.convert([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're allowing imports of other packages, I think we should include something in the digest that causes recompilation when dependencies change. E.g., the contents of Oh, it looks ilke that was present in the previous PR: https://github.com/Workiva/dart_dev/pull/363/files#diff-16bd3bf901e6560bcc074b4d1fdc6750f4c1ab19007f4d6f0d1acfa0989b9f08R160 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, there's this related comment about skipping caching for mutable dependencies: https://github.com/Workiva/dart_dev/pull/363/files#r753702320 Feels like it'd be worth addressing; @evanweible-wf what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah +1 to both of these comments - even if it's rare for mutable deps to be imported and used in ddev config, it'd be a very confusing issue to debug, so it'd be nice to cover this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added the contents of Handling caching for mutable dependencies, based on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @evanweible-wf @greglittlefield-wf I have addressed this comment. It took me an hour or so to implement but it is working well now. Sadly, the main repo where I was interested in using this precompilation system does have a I can think of some workarounds (tree walking + statting for timestamps?), but nothing quick. I think this PR is probably good to go for now and we could try to think of a more sophisticated solution in the future. |
||
if (configFile.existsSync()) ...configFile.readAsBytesSync(), | ||
if (configHasRelativeImports) | ||
for (final file in Glob('tool/**.dart', recursive: true) | ||
.listSync() | ||
alexandercampbell-wk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.whereType<File>() | ||
.where( | ||
(f) => p.canonicalize(f.path) != p.canonicalize(_paths.config))) | ||
...file.readAsBytesSync(), | ||
]); | ||
encodedDigest = base64.encode(digest.bytes); | ||
}, level: Level.FINE); | ||
|
||
if (encodedDigest != null && | ||
(!runExecutableDigest.existsSync() || | ||
runExecutableDigest.readAsStringSync() != encodedDigest)) { | ||
// Digest is missing or outdated, so we (re-)compile. | ||
logTimedSync(log, 'Compiling run script', () { | ||
alexandercampbell-wk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// 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. | ||
if (runExecutable.existsSync()) runExecutable.deleteSync(); | ||
if (runExecutableDigest.existsSync()) runExecutableDigest.deleteSync(); | ||
|
||
final args = [ | ||
'compile', | ||
'exe', | ||
_paths.runScript, | ||
'-o', | ||
_paths.runExecutable | ||
]; | ||
final result = Process.runSync(Platform.executable, args); | ||
Comment on lines
+208
to
+215
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security review: parameterized shelling 👍 |
||
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(" ")}'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security review: afaik, logging out the args given isn't problematic on cli. |
||
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 +211,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 +239,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:') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import 'dart:async'; | ||
import 'dart:io'; | ||
|
||
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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security review: hardcoded to delete .dart-tool/dart_dev |
||
if (cache.existsSync()) { | ||
cache.deleteSync(recursive: true); | ||
} | ||
alexandercampbell-wk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return ExitCode.success.code; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice touch.