-
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 all 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,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, | ||
], | ||
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 +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(); | ||
}); | ||
Comment on lines
+108
to
+111
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: The files to delete look like they are statically defined, so this couldn't be used to delete something unexpected: https://github.com/Workiva/dart_dev/pull/426/files#diff-6e5f65bcac0e9f8da735ef58d443983df38419788614afe4a860adebb5016f5cR35-R37 |
||
|
||
/// 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() { | ||
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. It would have been nice to split this function up to call a config getter, configImports validator, and digest encoder function so it was easier to digest. |
||
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); | ||
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}'); | ||
} | ||
Comment on lines
+200
to
+227
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. It would have been nice to split this out into a separate function. 🤷 |
||
}); | ||
} | ||
|
||
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:') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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()); | ||
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()) { | ||
log.info('Deleting ${cache.path}'); | ||
cache.deleteSync(recursive: true); | ||
} else { | ||
log.info('Nothing to do: no ${cache.path} found'); | ||
} | ||
alexandercampbell-wk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return ExitCode.success.code; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
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. pfft, not that Dart's AST parsing would be 100% accurate either. 😉 |
||
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:([^/]+)/.+'); |
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.