Skip to content

Small refactorings in tool_runner.dart #2685

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 18, 2021
Merged

Small refactorings in tool_runner.dart #2685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34911f0
Small refactorings in tool_runner.dart
srawlins Jun 17, 2021
0d925cd
feedback
srawlins Jun 17, 2021
d362687
Merge branch 'master' into tidy-tool-runner
srawlins Jun 17, 2021
16f0f4d
bad merge
srawlins Jun 17, 2021
4a763ae
Merge branch 'master' into tidy-tool-runner
srawlins Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 23 additions & 21 deletions lib/src/model/documentation_comment.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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:
Expand Down
156 changes: 90 additions & 66 deletions lib/src/tool_runner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -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(
Expand Down Expand Up @@ -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 =
Expand All @@ -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,
};
Expand All @@ -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;
Expand All @@ -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;
}
18 changes: 9 additions & 9 deletions test/tool_runner_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>'));
Expand All @@ -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>'));
Expand All @@ -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.'));
Expand All @@ -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>'));
Expand All @@ -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>'));
Expand All @@ -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(
Expand All @@ -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'));
Expand All @@ -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],
Expand Down