Small refactorings in tool_runner.dart (dart-lang#2685)

srawlins · web-flow · commit 3cf7c639766a · 2021-06-18T15:20:56.000-07:00
diff --git 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<String, String> _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 &#123;@example ...&#125; in API comments with the content of named file.
   ///
   /// Syntax:
diff --git a/lib/src/tool_runner.dart b/lib/src/tool_runner.dart
@@ -7,10 +7,11 @@ library dartdoc.tool_runner;
 import 'dart:io' show Process, ProcessException;
 
 import 'package:analyzer/file_system/file_system.dart';
+import 'package:dartdoc/src/dartdoc_options.dart';
 import 'package:dartdoc/src/io_utils.dart';
 import 'package:dartdoc/src/tool_definition.dart';
+import 'package:meta/meta.dart';
 import 'package:path/path.dart' as p;
-import 'dartdoc_options.dart';
 
 typedef ToolErrorCallback = void Function(String message);
 typedef FakeResultCallback = String Function(String tool,
@@ -20,9 +21,6 @@ typedef FakeResultCallback = String Function(String tool,
 /// limiting both parallelization and the number of open temporary files.
 final MultiFutureTracker<void> _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;
@@ -41,7 +39,7 @@ class ToolTempFileTracker {
 
   int _temporaryFileCount = 0;
 
-  Future<File> createTemporaryFile() async {
+  File createTemporaryFile() {
     _temporaryFileCount++;
     // TODO(srawlins): Assume [temporaryDirectory]'s path is always absolute.
     var tempFile = resourceProvider.getFile(resourceProvider.pathContext.join(
@@ -80,22 +78,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<String> _runProcess(
-      String name,
-      String content,
-      String commandPath,
-      List<String> args,
-      Map<String, String> 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<String> _runProcess(String name, String content, String commandPath,
+      List<String> args, Map<String, String> environment,
+      {@required ToolErrorCallback toolErrorCallback}) async {
     String commandString() => ([commandPath] + args).join(' ');
     try {
       var result =
@@ -120,58 +120,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<String> run(List<String> args, ToolErrorCallback toolErrorCallback,
-      {String content, Map<String, String> 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<String> run(List<String> args,
+      {@required String content,
+      @required ToolErrorCallback toolErrorCallback,
+      Map<String, String> environment}) async {
+    assert(args != null);
+    assert(args.isNotEmpty);
     Future<String> 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<String> _run(List<String> args, ToolErrorCallback toolErrorCallback,
-      {String content, Map<String, String> environment}) async {
+  Future<String> _run(List<String> args,
+      {@required ToolErrorCallback toolErrorCallback,
+      String content,
+      Map<String, String> environment}) async {
     assert(args != null);
     assert(args.isNotEmpty);
     content ??= '';
     environment ??= <String, String>{};
-    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,
     };
@@ -183,17 +177,62 @@ 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 argsWithInput = [
+      ...toolArgs,
+      ..._substituteInArgs(args, envWithInput),
+    ];
+
+    if (toolDefinition.setupCommand != null && !toolDefinition.setupComplete) {
+      await _runSetup(
+          toolName, toolDefinition, envWithInput, toolErrorCallback);
+    }
+
+    var toolStateForArgs = await toolDefinition.toolStateForArgs(argsWithInput);
+    var commandPath = toolStateForArgs.commandPath;
+    argsWithInput = toolStateForArgs.args;
+    var callCompleter = toolStateForArgs.onProcessComplete;
+    var stdout = _runProcess(
+        toolName, content, commandPath, argsWithInput, envWithInput,
+        toolErrorCallback: toolErrorCallback);
+
+    if (callCompleter == null) {
+      return stdout;
+    } else {
+      return stdout.whenComplete(callCompleter);
+    }
+  }
+
+  /// 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<String> _substituteInArgs(
+      List<String> args, Map<String, String> envWithInput) {
     var substitutions = envWithInput.map<RegExp, String>((key, value) {
       var escapedKey = RegExp.escape(key);
       return MapEntry(RegExp('\\\$(\\($escapedKey\\)|$escapedKey\\b)'), value);
     });
+
     var argsWithInput = <String>[];
     for (var arg in args) {
       var newArg = arg;
@@ -202,25 +241,10 @@ class ToolRunner {
       argsWithInput.add(newArg);
     }
 
-    if (toolDefinition.setupCommand != null && !toolDefinition.setupComplete) {
-      await _runSetup(tool, toolDefinition, envWithInput, toolErrorCallback);
-    }
-
-    argsWithInput = toolArgs + argsWithInput;
-    var toolStateForArgs = await toolDefinition.toolStateForArgs(argsWithInput);
-    var commandPath = toolStateForArgs.commandPath;
-    argsWithInput = toolStateForArgs.args;
-    var callCompleter = toolStateForArgs.onProcessComplete;
-
-    if (callCompleter != null) {
-      return _runProcess(tool, content, commandPath, argsWithInput,
-              envWithInput, toolErrorCallback)
-          .whenComplete(callCompleter);
-    } else {
-      return _runProcess(tool, content, commandPath, argsWithInput,
-          envWithInput, toolErrorCallback);
-    }
+    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
@@ -113,8 +113,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=<INPUT_FILE>'));
@@ -125,8 +125,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=<INPUT_FILE>'));
@@ -137,8 +137,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.'));
@@ -149,17 +149,17 @@ 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.
     });
     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=<INPUT_FILE>'));
@@ -168,8 +168,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=<INPUT_FILE>'));
@@ -179,8 +179,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(
@@ -190,8 +190,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'));
@@ -200,8 +200,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],
