Skip to content

POC for better metrics gathering from build_runner #371

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

Open
alan-knight wants to merge 11 commits into
base: master
Choose a base branch
from
Open

POC for better metrics gathering from build_runner #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2da370a
Allow args after separator to TestTool
evanweible-wf Dec 9, 2021
6711d9a
Fix tests
evanweible-wf Dec 9, 2021
846edaa
Fix doc comment example
evanweible-wf Dec 13, 2021
73daa7a
Better variable naming
evanweible-wf Dec 15, 2021
65ab421
Fix tests
evanweible-wf Dec 15, 2021
be06a13
Update CHANGELOG.md
evanweible-wf Dec 15, 2021
9d275e5
Merge pull request #365 from Workiva/test_tool_trailing_args_v370back…
rmconsole5-wk Dec 16, 2021
3d8aec6
Bump version to 3.7.1
rmconsole-readonly-wk Dec 16, 2021
4081fdf
cherry-pick 902abaa
alanknight-wk Feb 14, 2022
b613bfb
Trying to make the command result be more than just an exit code
alanknight-wk Feb 14, 2022
a534b8f
Minimally works as POC, log gets written and read
alanknight-wk Feb 16, 2022
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
12 changes: 12 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
# Changelog

## [3.7.1](https://github.com/Workiva/dart_dev/compare/3.7.0...3.7.1)

- Update `TestTool` to allow arguments after a separator (`--`). These arguments
will always be passed to the `dart test` process. The main use case for this is
integration with IDE plugins that enable running tests directly from the IDE.
- Update `FunctionTool` to allow arguments after a separator (`--`). There isn't
a strong reason to disallow this since the function tool could do anything it
wants with those args (and now we have a concrete use case for just that).
- Fix a bug in `takeAllArgs` (the arg mapper util used with `CompoundTool`) so
that it now properly restores the first separator (`--`) if present in the
original arguments list.

## [3.7.0](https://github.com/Workiva/dart_dev/compare/3.7.0...3.6.7)

- Export ArgResults utilities.
Expand Down
51 changes: 51 additions & 0 deletions bin/tweaked_build_runner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

/// Very simplified version of the build_runner main that also does logging of the
/// build summary.
import 'dart:async';
import 'dart:io';

import 'package:build_runner/src/build_script_generate/bootstrap.dart';
import 'package:build_runner/src/build_script_generate/build_script_generate.dart';
import 'package:build_runner/src/logging/std_io_logging.dart';
import 'package:io/io.dart';
import 'package:logging/logging.dart';

String logFile;
const String logFileArgument = '--logFile=';

bool isLogArgument(String arg) => arg.startsWith(logFileArgument);
Future<void> main(List<String> args) async {
logFile = args
.firstWhere(isLogArgument, orElse: () => null)
.substring(logFileArgument.length);
// We're generating the argument directly into the file, but that means that each run will
// rebuild the build script??
args = [
for (var arg in args)
if (!isLogArgument(arg)) arg
];
var logListener = Logger.root.onRecord.listen(stdIOLogListener());

while ((exitCode = await generateAndRun(args,
generateBuildScript: generateMyBuildScript)) ==
ExitCode.tempFail.code) {}
await logListener.cancel();
}

// Tweak the build script to also log to the file named passed in the --logFileName argument.
Future<String> generateMyBuildScript() async {
var basic = await generateBuildScript();
var lines = basic.split('\n');
lines.insert(7, "import 'dart:io';");
lines.insert(7, "import 'package:logging/logging.dart';");
var startOfMain =
lines.indexOf(lines.firstWhere((l) => l.contains('void main')));
lines.insert(startOfMain + 1,
' var log = File("${logFile ?? 'temporaryLogFile.txt'}");');
lines.insert(startOfMain + 2,
' Logger.root.onRecord.listen((r) {log.writeAsString("\$r", mode: FileMode.append);});');
return lines.join('\n');
}
25 changes: 22 additions & 3 deletions lib/src/dart_dev_runner.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'dart:async';
import 'dart:io';

import 'package:args/args.dart';
import 'package:args/command_runner.dart';
Expand Down Expand Up @@ -44,12 +45,30 @@ class DartDevRunner extends CommandRunner<int> {
print(dartDevVersion);
return 0;
}
// About the only mechanism I can find for communicating between the different levels of this
// is that this runner holds an actual list of command instances, so we can associate the log
// file path with that.
print("commands = $commands");
print("we're trying to execute ${argResults.name}");
print("or is that ${argResults.command.name}");
final command = commands[argResults.command.name];
print(
"We are trying to write a log at ${(command as DevToolCommand).logFilePath}");
final stopwatch = Stopwatch()..start();

print("running with ${args}");
final exitCode = (await super.run(args)) ?? 0;
stopwatch.stop();
await events.commandComplete(
events.CommandResult(args, exitCode, stopwatch.elapsed));
return exitCode;
String log;
if (command != null && command is DevToolCommand) {
print("We expect to read a log at ${command.logFilePath}");
log = await File(command.logFilePath).readAsString();
print("Got ${log.length} bytes of log");
}
events.CommandResult result =
events.CommandResult(args, exitCode, stopwatch.elapsed, log: log);
await events.commandComplete(result);
return result.exitCode;
}
}

Expand Down
26 changes: 19 additions & 7 deletions lib/src/dart_dev_tool.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import 'dart:async';
import 'dart:io';
import 'dart:math';

import 'package:args/args.dart';
import 'package:args/command_runner.dart';
import 'package:dart_dev/dart_dev.dart';
import 'package:path/path.dart' as path;

import 'tools/function_tool.dart';
import 'utils/verbose_enabled.dart';
Expand Down Expand Up @@ -83,7 +85,8 @@ class DevToolExecutionContext {
{this.argResults,
this.commandName,
void Function(String message) usageException,
bool verbose})
bool verbose,
this.logFilePath})
: _usageException = usageException,
verbose = verbose ?? false;

Expand All @@ -107,20 +110,23 @@ class DevToolExecutionContext {
/// This will not be null; it defaults to `false`.
final bool verbose;

String logFilePath;

/// Return a copy of this instance with optional updates; any field that does
/// not have an updated value will remain the same.
DevToolExecutionContext update({
ArgResults argResults,
String commandName,
void Function(String message) usageException,
bool verbose,
String logFilePath,
}) =>
DevToolExecutionContext(
argResults: argResults ?? this.argResults,
commandName: commandName ?? this.commandName,
usageException: usageException ?? this.usageException,
verbose: verbose ?? this.verbose,
);
argResults: argResults ?? this.argResults,
commandName: commandName ?? this.commandName,
usageException: usageException ?? this.usageException,
verbose: verbose ?? this.verbose,
logFilePath: logFilePath ?? this.logFilePath);

/// Calling this will throw a [UsageException] with [message] that should be
/// caught by [CommandRunner] and used to set the exit code accordingly and
Expand Down Expand Up @@ -150,10 +156,16 @@ class DevToolCommand extends Command<int> {
@override
final String name;

/// A probably unique file path to write logs to.
// TODO: A better name mechanism.
final String logFilePath = path.join(Directory.systemTemp.path,
'${Random(DateTime.now().microsecondsSinceEpoch).nextInt(1 << 32)}');

@override
Future<int> run() async => devTool.run(DevToolExecutionContext(
argResults: argResults,
commandName: name,
usageException: usageException,
verbose: verboseEnabled(this)));
verbose: verboseEnabled(this),
logFilePath: logFilePath));
}
3 changes: 2 additions & 1 deletion lib/src/events.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ final _commandCompleteListeners =
<FutureOr<dynamic> Function(CommandResult result)>[];

class CommandResult {
CommandResult(this.args, this.exitCode, this.duration);
CommandResult(this.args, this.exitCode, this.duration, {this.log});
final List<String> args;
final Duration duration;
final int exitCode;
String log;
}
1 change: 0 additions & 1 deletion lib/src/executable.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import 'dart:async';
import 'dart:io';
import 'dart:math';

import 'package:analyzer/dart/analysis/utilities.dart';
import 'package:args/command_runner.dart';
Expand Down
3 changes: 2 additions & 1 deletion lib/src/tools/compound_tool.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:dart_dev/dart_dev.dart';
import 'package:logging/logging.dart';

import '../dart_dev_tool.dart';
import '../utils/rest_args_with_separator.dart';

final _log = Logger('CompoundTool');

Expand Down Expand Up @@ -43,7 +44,7 @@ ArgResults takeOptionArgs(ArgParser parser, ArgResults results) =>
/// };
ArgResults takeAllArgs(ArgParser parser, ArgResults results) => parser.parse([
...optionArgsOnly(results, allowedOptions: parser.options.keys),
...results.rest,
...restArgsWithSeparator(results),
]);

class CompoundTool extends DevTool with CompoundToolMixin {}
Expand Down
3 changes: 0 additions & 3 deletions lib/src/tools/function_tool.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ class FunctionTool extends DevTool {
assertNoPositionalArgsNorArgsAfterSeparator(
context.argResults, context.usageException,
commandName: context.commandName);
} else {
assertNoArgsAfterSeparator(context.argResults, context.usageException,
commandName: context.commandName);
}
}
final exitCode = await _function(context);
Expand Down
34 changes: 15 additions & 19 deletions lib/src/tools/test_tool.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import 'package:logging/logging.dart';

import '../dart_dev_tool.dart';
import '../utils/arg_results_utils.dart';
import '../utils/assert_no_args_after_separator.dart';
import '../utils/logging.dart';
import '../utils/package_is_immediate_dependency.dart';
import '../utils/process_declaration.dart';
Expand Down Expand Up @@ -188,6 +187,7 @@ List<String> buildArgs({
List<String> configuredTestArgs,
bool useBuildRunner,
bool verbose,
String logFilePath,
}) {
useBuildRunner ??= false;
verbose ??= false;
Expand Down Expand Up @@ -233,10 +233,14 @@ List<String> buildArgs({
}

return [
// `pub run test` or `pub run build_runner test`
'run',
if (useBuildRunner) 'build_runner',
'test',
// `dart test` or `dart run build_runner test`
if (useBuildRunner) ...[
'run',
'dart_dev:tweaked_build_runner',
'test',
if (useBuildRunner && (logFilePath != null)) '--logFile=$logFilePath',
] else
'test',

// Add the args targeting the build_runner command.
if (useBuildRunner) ...buildArgs,
Expand Down Expand Up @@ -273,16 +277,6 @@ TestExecution buildExecution(
List<String> configuredTestArgs,
String path,
}) {
if (context.argResults != null) {
assertNoArgsAfterSeparator(context.argResults, context.usageException,
commandName: context.commandName,
usageFooter:
'Arguments can be passed to the test runner process via the '
'--test-args option.\n'
'If this project runs tests via build_runner, arguments can be '
'passed to that process via the --build-args option.');
}

final hasBuildRunner =
packageIsImmediateDependency('build_runner', path: path);
final hasBuildTest = packageIsImmediateDependency('build_test', path: path);
Expand Down Expand Up @@ -323,19 +317,21 @@ TestExecution buildExecution(
configuredBuildArgs: configuredBuildArgs,
configuredTestArgs: configuredTestArgs,
useBuildRunner: useBuildRunner,
verbose: context.verbose);
verbose: context.verbose,
logFilePath: context.logFilePath);
logSubprocessHeader(_log, 'pub ${args.join(' ')}'.trim());
return TestExecution.process(
ProcessDeclaration('pub', args, mode: ProcessStartMode.inheritStdio));
ProcessDeclaration('dart', args, mode: ProcessStartMode.inheritStdio));
}

// NOTE: This currently depends on https://github.com/dart-lang/build/pull/2445
// Additionally, consumers need to depend on build_web_compilers AND build_vm_compilers
// We should add some guard-rails (don't use filters if either of those deps are
// missing, and ensure adequate version of build_runner).
Iterable<String> buildFiltersForTestArgs(List<String> testInputs) {
Iterable<String> buildFiltersForTestArgs(List<String> testArgs) {
final testInputs = (testArgs ?? []).where((arg) => arg.startsWith('test'));
final filters = <String>[];
for (final input in testInputs ?? []) {
for (final input in testInputs) {
if (input.endsWith('.dart')) {
filters..add('$input.*_test.dart.js*')..add(dartExtToHtml(input));
} else {
Expand Down
44 changes: 44 additions & 0 deletions lib/src/utils/rest_args_with_separator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import 'package:args/args.dart';

/// Returns the "rest" args from [argResults], but with the arg separator "--"
/// restored to its original position if it was included.
///
/// This is necessary because [ArgResults.rest] will _not_ include the separator
/// unless it stopped parsing before it reached the separator.
///
/// The use case for this is a [CompoundTool] that uses the [takeAllArgs] arg
/// mapper, because the goal there is to forward on the original args minus the
/// consumed options and flags. If the separator has also been removed, you may
/// hit an error when trying to parse those args.
///
/// var parser = ArgParser()..addFlag('verbose', abbr: 'v');
/// var results = parser.parse(['a', '-v', 'b', '--', '--unknown', 'c']);
/// print(results.rest);
/// // ['a', 'b', '--unknown', 'c']
/// print(restArgsWithSeparator(results));
/// // ['a', 'b', '--', '--unknown', 'c']
List<String> restArgsWithSeparator(ArgResults argResults) {
// If no separator was used, return the rest args as is.
if (!argResults.arguments.contains('--')) {
return argResults.rest;
}

final args = argResults.arguments;
final rest = argResults.rest;
var restIndex = 0;
for (var argsIndex = 0; argsIndex < args.length; argsIndex++) {
// Iterate through the original args until we hit the first separator.
if (args[argsIndex] == '--') break;
// While doing so, move a cursor through the rest args list each time we
// match up between the original list and the rest args list. This works
// because the rest args list should be an ordered subset of the original
// args list.
if (args[argsIndex] == rest[restIndex]) {
restIndex++;
}
}

// At this point, [restIndex] should be pointing to the spot where the first
// arg separator should be restored.
return [...rest.sublist(0, restIndex), '--', ...rest.sublist(restIndex)];
}
2 changes: 1 addition & 1 deletion pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: dart_dev
version: 3.7.0
version: 3.7.1
description: Centralized tooling for Dart projects. Consistent interface across projects. Easily configurable.
homepage: https://github.com/Workiva/dart_dev

Expand Down
10 changes: 3 additions & 7 deletions test/tools/function_tool_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,10 @@ void main() {
.run(DevToolExecutionContext(argResults: parser.parse(['--flag'])));
});

test(
'throws UsageException with custom ArgParser and args after a separator',
() {
test('allows a custom ArgParser and args after a separator', () async {
final tool = DevTool.fromFunction((_) => 0, argParser: ArgParser());
expect(
() => tool.run(DevToolExecutionContext(
argResults: ArgParser().parse(['--', 'foo']))),
throwsA(isA<UsageException>()));
await tool.run(DevToolExecutionContext(
argResults: ArgParser().parse(['--', 'foo'])));
});

test('logs a warning if no exit code is returned', () {
Expand Down
Loading