From 34911f020d1ebf7b4b335100e69c5a36d97130b1 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Thu, 17 Jun 2021 00:57:32 -0700 Subject: [PATCH 1/3] Small refactorings in tool_runner.dart --- lib/src/model/documentation_comment.dart | 44 ++++--- lib/src/tool_runner.dart | 156 +++++++++++++---------- test/tool_runner_test.dart | 18 +-- 3 files changed, 122 insertions(+), 96 deletions(-) diff --git a/lib/src/model/documentation_comment.dart b/lib/src/model/documentation_comment.dart index a8d63748f6..1ab0d84498 100644 --- a/lib/src/model/documentation_comment.dart +++ b/lib/src/model/documentation_comment.dart @@ -4,6 +4,7 @@ import 'package:dartdoc/src/model/model.dart'; import 'package:dartdoc/src/render/model_element_renderer.dart'; import 'package:dartdoc/src/utils.dart'; import 'package:dartdoc/src/warnings.dart'; +import 'package:meta/meta.dart'; import 'package:path/path.dart' as path show Context; final _templatePattern = RegExp( @@ -232,30 +233,31 @@ mixin DocumentationComment // Count the number of invocations of tools in this dartdoc block, // so that tools can differentiate different blocks from each other. invocationIndex++; - return await config.tools.runner.run( - args, - (String message) async => - warn(PackageWarning.toolError, message: message), - content: basicMatch[2], - environment: { - 'SOURCE_LINE': characterLocation?.lineNumber.toString(), - 'SOURCE_COLUMN': characterLocation?.columnNumber.toString(), - 'SOURCE_PATH': - (sourceFileName == null || package?.packagePath == null) - ? null - : pathContext.relative(sourceFileName, - from: package.packagePath), - 'PACKAGE_PATH': package?.packagePath, - 'PACKAGE_NAME': package?.name, - 'LIBRARY_NAME': library?.fullyQualifiedName, - 'ELEMENT_NAME': fullyQualifiedNameWithoutLibrary, - 'INVOCATION_INDEX': invocationIndex.toString(), - 'PACKAGE_INVOCATION_INDEX': - (package.toolInvocationIndex++).toString(), - }..removeWhere((key, value) => value == null)); + return await config.tools.runner.run(args, content: basicMatch[2], + toolErrorCallback: (String message) async { + print('toolerrocallback for $args'); + warn(PackageWarning.toolError, message: message); + }, environment: _toolsEnvironment(invocationIndex: invocationIndex)); }); } + /// The environment variables to use when running a tool. + Map _toolsEnvironment({@required int invocationIndex}) { + return { + 'SOURCE_LINE': characterLocation?.lineNumber.toString(), + 'SOURCE_COLUMN': characterLocation?.columnNumber.toString(), + if (sourceFileName != null && package?.packagePath != null) + 'SOURCE_PATH': + pathContext.relative(sourceFileName, from: package.packagePath), + 'PACKAGE_PATH': package?.packagePath, + 'PACKAGE_NAME': package?.name, + 'LIBRARY_NAME': library?.fullyQualifiedName, + 'ELEMENT_NAME': fullyQualifiedNameWithoutLibrary, + 'INVOCATION_INDEX': invocationIndex.toString(), + 'PACKAGE_INVOCATION_INDEX': (package.toolInvocationIndex++).toString(), + }..removeWhere((key, value) => value == null); + } + /// Replace {@example ...} in API comments with the content of named file. /// /// Syntax: diff --git a/lib/src/tool_runner.dart b/lib/src/tool_runner.dart index b28be74c43..7cd355a806 100644 --- a/lib/src/tool_runner.dart +++ b/lib/src/tool_runner.dart @@ -8,6 +8,7 @@ import 'dart:io' show Process, ProcessException; import 'package:analyzer/file_system/file_system.dart'; import 'package:dartdoc/src/io_utils.dart'; +import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; import 'dartdoc_options.dart'; @@ -19,9 +20,6 @@ typedef FakeResultCallback = String Function(String tool, /// limiting both parallelization and the number of open temporary files. final MultiFutureTracker _toolTracker = MultiFutureTracker(4); -/// Can be called when the ToolRunner is no longer needed. -/// -/// This will remove any temporary files created by the tool runner. class ToolTempFileTracker { final ResourceProvider resourceProvider; final Folder temporaryDirectory; @@ -40,7 +38,7 @@ class ToolTempFileTracker { int _temporaryFileCount = 0; - Future createTemporaryFile() async { + File createTemporaryFile() { _temporaryFileCount++; // TODO(srawlins): Assume [temporaryDirectory]'s path is always absolute. var tempFile = resourceProvider.getFile(resourceProvider.pathContext.join( @@ -79,22 +77,24 @@ class ToolRunner { String commandPath; if (isDartSetup) { - commandPath = toolConfiguration.resourceProvider.resolvedExecutable; + commandPath = resourceProvider.resolvedExecutable; } else { commandPath = args.removeAt(0); } - await _runProcess( - name, '', commandPath, args, environment, toolErrorCallback); + // We do not use the stdout of the setup process. + await _runProcess(name, '', commandPath, args, environment, + toolErrorCallback: toolErrorCallback); tool.setupComplete = true; } - Future _runProcess( - String name, - String content, - String commandPath, - List args, - Map environment, - ToolErrorCallback toolErrorCallback) async { + /// Runs the tool with [Process.run], awaiting the exit code, and returning + /// the stdout. + /// + /// If the process's exit code is not 0, or if a [ProcessException] is thrown, + /// calls [toolErrorCallback] with a detailed error message, and returns `''`. + Future _runProcess(String name, String content, String commandPath, + List args, Map environment, + {@required ToolErrorCallback toolErrorCallback}) async { String commandString() => ([commandPath] + args).join(' '); try { var result = @@ -119,58 +119,52 @@ class ToolRunner { } } - /// Run a tool. The name of the tool is the first argument in the [args]. - /// The content to be sent to to the tool is given in the optional [content], - /// and the stdout of the tool is returned. + /// Run a tool. /// - /// The [args] must not be null, and it must have at least one member (the name - /// of the tool). - Future run(List args, ToolErrorCallback toolErrorCallback, - {String content, Map environment}) async { + /// The name of the tool is the first argument in the [args]. The content to + /// be sent to to the tool is given in the optional [content]. The stdout of + /// the tool is returned. + Future run(List args, + {@required String content, + @required ToolErrorCallback toolErrorCallback, + Map environment}) async { + assert(args != null); + assert(args.isNotEmpty); Future runner; // Prevent too many tools from running simultaneously. await _toolTracker.addFutureFromClosure(() { - runner = _run(args, toolErrorCallback, - content: content, environment: environment); + runner = _run(args, + toolErrorCallback: toolErrorCallback, + content: content, + environment: environment); return runner; }); return runner; } - Future _run(List args, ToolErrorCallback toolErrorCallback, - {String content, Map environment}) async { + Future _run(List args, + {@required ToolErrorCallback toolErrorCallback, + String content, + Map environment}) async { assert(args != null); assert(args.isNotEmpty); content ??= ''; environment ??= {}; - var tool = args.removeAt(0); - if (!toolConfiguration.tools.containsKey(tool)) { + var toolName = args.removeAt(0); + if (!toolConfiguration.tools.containsKey(toolName)) { toolErrorCallback( - 'Unable to find definition for tool "$tool" in tool map. ' + 'Unable to find definition for tool "$toolName" in tool map. ' 'Did you add it to dartdoc_options.yaml?'); return ''; } - var toolDefinition = toolConfiguration.tools[tool]; + var toolDefinition = toolConfiguration.tools[toolName]; var toolArgs = toolDefinition.command; - // Ideally, we would just be able to send the input text into stdin, but - // there's no way to do that synchronously, and converting dartdoc to an - // async model of execution is a huge amount of work. Using dart:cli's - // waitFor feels like a hack (and requires a similar amount of work anyhow - // to fix order of execution issues). So, instead, we have the tool take a - // filename as part of its arguments, and write the input to a temporary - // file before running the tool synchronously. - - // Write the content to a temp file. - var tmpFile = await ToolTempFileTracker.createInstance( - toolConfiguration.resourceProvider) - .createTemporaryFile(); - tmpFile.writeAsStringSync(content); // Substitute the temp filename for the "$INPUT" token, and all of the other // environment variables. Variables are allowed to either be in $(VAR) form, // or $VAR form. var envWithInput = { - 'INPUT': pathContext.absolute(tmpFile.path), + 'INPUT': _tmpFileWithContent(content), 'TOOL_COMMAND': toolDefinition.command[0], ...environment, }; @@ -182,30 +176,22 @@ class ToolRunner { // find out where their script was coming from as an absolute path on the // filesystem. envWithInput['DART_SNAPSHOT_CACHE'] = pathContext.absolute( - SnapshotCache.createInstance(toolConfiguration.resourceProvider) - .snapshotCache - .path); + SnapshotCache.createInstance(resourceProvider).snapshotCache.path); if (toolDefinition.setupCommand != null) { envWithInput['DART_SETUP_COMMAND'] = toolDefinition.setupCommand[0]; } } - var substitutions = envWithInput.map((key, value) { - var escapedKey = RegExp.escape(key); - return MapEntry(RegExp('\\\$(\\($escapedKey\\)|$escapedKey\\b)'), value); - }); - var argsWithInput = []; - for (var arg in args) { - var newArg = arg; - substitutions - .forEach((regex, value) => newArg = newArg.replaceAll(regex, value)); - argsWithInput.add(newArg); - } + + var argsWithInput = [ + ...toolArgs, + ..._substituteInArgs(args, envWithInput), + ]; if (toolDefinition.setupCommand != null && !toolDefinition.setupComplete) { - await _runSetup(tool, toolDefinition, envWithInput, toolErrorCallback); + await _runSetup( + toolName, toolDefinition, envWithInput, toolErrorCallback); } - argsWithInput = toolArgs + argsWithInput; String commandPath; void Function() callCompleter; if (toolDefinition is DartToolDefinition) { @@ -216,16 +202,54 @@ class ToolRunner { } else { commandPath = argsWithInput.removeAt(0); } + var stdout = _runProcess( + toolName, content, commandPath, argsWithInput, envWithInput, + toolErrorCallback: toolErrorCallback); - if (callCompleter != null) { - return _runProcess(tool, content, commandPath, argsWithInput, - envWithInput, toolErrorCallback) - .whenComplete(callCompleter); + if (callCompleter == null) { + return stdout; } else { - return _runProcess(tool, content, commandPath, argsWithInput, - envWithInput, toolErrorCallback); + return stdout.whenComplete(callCompleter); // ignore: unawaited_futures + } + } + + /// Returns the path to the temp file after [content] is written to it. + String _tmpFileWithContent(String content) { + // Ideally, we would just be able to send the input text into stdin, but + // there's no way to do that synchronously, and converting dartdoc to an + // async model of execution is a huge amount of work. Using dart:cli's + // waitFor feels like a hack (and requires a similar amount of work anyhow + // to fix order of execution issues). So, instead, we have the tool take a + // filename as part of its arguments, and write the input to a temporary + // file before running the tool synchronously. + + // Write the content to a temp file. + var tmpFile = ToolTempFileTracker.createInstance(resourceProvider) + .createTemporaryFile(); + tmpFile.writeAsStringSync(content); + return pathContext.absolute(tmpFile.path); + } + + // TODO(srawlins): Unit tests. + List _substituteInArgs( + List args, Map envWithInput) { + var substitutions = envWithInput.map((key, value) { + var escapedKey = RegExp.escape(key); + return MapEntry(RegExp('\\\$(\\($escapedKey\\)|$escapedKey\\b)'), value); + }); + + var argsWithInput = []; + for (var arg in args) { + var newArg = arg; + substitutions + .forEach((regex, value) => newArg = newArg.replaceAll(regex, value)); + argsWithInput.add(newArg); } + + return argsWithInput; } - p.Context get pathContext => toolConfiguration.resourceProvider.pathContext; + ResourceProvider get resourceProvider => toolConfiguration.resourceProvider; + + p.Context get pathContext => resourceProvider.pathContext; } diff --git a/test/tool_runner_test.dart b/test/tool_runner_test.dart index f49af7840e..6504e4c2ad 100644 --- a/test/tool_runner_test.dart +++ b/test/tool_runner_test.dart @@ -112,8 +112,8 @@ echo: test('can invoke a Dart tool, and second run is a snapshot.', () async { var result = await runner.run( ['drill', r'--file=$INPUT'], - errorCallback, content: 'TEST INPUT', + toolErrorCallback: errorCallback, ); expect(errors, isEmpty); expect(result, contains('--file=')); @@ -124,8 +124,8 @@ echo: expect(setupFile.existsSync(), isFalse); result = await runner.run( ['drill', r'--file=$INPUT'], - errorCallback, content: 'TEST INPUT 2', + toolErrorCallback: errorCallback, ); expect(errors, isEmpty); expect(result, contains('--file=')); @@ -136,8 +136,8 @@ echo: test('can invoke a Dart tool', () async { var result = await runner.run( ['drill', r'--file=$INPUT'], - errorCallback, content: 'TEST INPUT', + toolErrorCallback: errorCallback, ); expect(errors, isEmpty); expect(result, contains('Script location is in snapshot cache.')); @@ -148,8 +148,8 @@ echo: test('can invoke a non-Dart tool', () async { var result = await runner.run( ['non_dart', '--version'], - errorCallback, content: 'TEST INPUT', + toolErrorCallback: errorCallback, ); expect(errors, isEmpty); expect(result, isEmpty); // Output is on stderr. @@ -157,8 +157,8 @@ echo: test('can invoke a pre-snapshotted tool', () async { var result = await runner.run( ['snapshot_drill', r'--file=$INPUT'], - errorCallback, content: 'TEST INPUT', + toolErrorCallback: errorCallback, ); expect(errors, isEmpty); expect(result, contains('--file=')); @@ -167,8 +167,8 @@ echo: test('can invoke a tool with a setup action', () async { var result = await runner.run( ['setup_drill', r'--file=$INPUT'], - errorCallback, content: 'TEST INPUT', + toolErrorCallback: errorCallback, ); expect(errors, isEmpty); expect(result, contains('--file=')); @@ -178,8 +178,8 @@ echo: test('fails if tool not in tool map', () async { var result = await runner.run( ['hammer', r'--file=$INPUT'], - errorCallback, content: 'TEST INPUT', + toolErrorCallback: errorCallback, ); expect(errors, isNotEmpty); expect( @@ -189,8 +189,8 @@ echo: test('fails if tool returns non-zero status', () async { var result = await runner.run( ['drill', r'--file=/a/missing/file'], - errorCallback, content: 'TEST INPUT', + toolErrorCallback: errorCallback, ); expect(errors, isNotEmpty); expect(errors[0], contains('Tool "drill" returned non-zero exit code')); @@ -199,8 +199,8 @@ echo: test("fails if tool in tool map doesn't exist", () async { var result = await runner.run( ['missing'], - errorCallback, content: 'TEST INPUT', + toolErrorCallback: errorCallback, ); expect(errors, isNotEmpty); expect(errors[0], From 0d925cd9ae52c75d076f787f524b0ba1882b984c Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Thu, 17 Jun 2021 15:05:04 -0700 Subject: [PATCH 2/3] feedback --- lib/src/tool_runner.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/tool_runner.dart b/lib/src/tool_runner.dart index 7cd355a806..45ddeb8c6e 100644 --- a/lib/src/tool_runner.dart +++ b/lib/src/tool_runner.dart @@ -209,7 +209,7 @@ class ToolRunner { if (callCompleter == null) { return stdout; } else { - return stdout.whenComplete(callCompleter); // ignore: unawaited_futures + return stdout.whenComplete(callCompleter); } } From 16f0f4d8f7eac840eddd7dd29343b4c96dbd60e4 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Thu, 17 Jun 2021 15:49:27 -0700 Subject: [PATCH 3/3] bad merge --- lib/src/tool_runner.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/src/tool_runner.dart b/lib/src/tool_runner.dart index 2e45af74ec..23e8737fc8 100644 --- a/lib/src/tool_runner.dart +++ b/lib/src/tool_runner.dart @@ -193,7 +193,6 @@ class ToolRunner { toolName, toolDefinition, envWithInput, toolErrorCallback); } - argsWithInput = toolArgs + argsWithInput; var toolStateForArgs = await toolDefinition.toolStateForArgs(argsWithInput); var commandPath = toolStateForArgs.commandPath; argsWithInput = toolStateForArgs.args;