Skip to content
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

CPLAT-15507: ddev should use new dart CLI #362

Merged
merged 20 commits into from
Oct 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 15 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ By default, this provides three core tasks:
Run any of these tools via the `dart_dev` command-line app:

```bash
$ pub run dart_dev analyze
$ dart run dart_dev analyze
[INFO] Running subprocess:
dartanalyzer .
dart analyze .
--------------------------
Analyzing dart_dev...
No issues found!
Expand All @@ -51,7 +51,7 @@ No issues found!
> We recommend adding a `ddev` alias:
>
> ```bash
> alias ddev='pub run dart_dev'
> alias ddev='dart run dart_dev'
> ```

Additional Dart developer tools can be added and every tool can be configured.
Expand Down Expand Up @@ -79,7 +79,7 @@ final config = {
Most Dart projects eventually share a common set of development requirements
(e.g. static analysis, formatting, test running, serving, etc.). The Dart SDK
along with some core packages supply the necessary tooling for these developer
tasks (e.g. `dartanalyzer`, `dartfmt`, or `pub run test`).
tasks (e.g. `dart analyze`, `dart format`, or `dart test`).

While the core tooling gets us far, there are two areas in which we feel it
falls short:
Expand Down Expand Up @@ -110,7 +110,7 @@ memorized or referenced in order to run said task.
Consider formatting as an example. The default approach to formatting files is
to run `dartfmt -w .`. But, some projects may want to exclude certain files that
would otherwise be formatted by this command. Or, some projects may want to use
`pub run dart_style:format` instead of `dartfmt`. Currently, there is no
`pub run dart_style:format` instead of `dart format`. Currently, there is no
project-level configuration supported by the formatter, so these sorts of things
just have to be documented in a `README.md` or `CONTRIBUTING.md`.

Expand All @@ -131,7 +131,7 @@ final config = {
```bash
$ ddev format
[INFO] Running subprocess:
pub run dart_style:format -w <3 paths>
dart run dart_style:format -w <3 paths>
--------------------------------------
Unchanged ./lib/foo.dart
Unchanged ./lib/src/bar.dart
Expand All @@ -144,33 +144,33 @@ Using existing tooling provided by (or conventionalized by) the Dart community
should always be the goal, but the reality is that there are gaps. Certain use
cases can be made more convenient and new use cases may arise.

Consider test running as an example. For simple projects, `pub run test` is
Consider test running as an example. For simple projects, `dart test` is
sufficient. In fact, the test package supports a huge amount of project-level
configuration via `dart_test.yaml`, which means that for projects that are
properly configured, `pub run test` just works.
properly configured, `dart test` just works.

Unfortunately, at this time, projects that rely on builders must run tests via
`pub run build_runner test`. Based on the project, you would need to know which
`dart run build_runner test`. Based on the project, you would need to know which
test command should be run.

With `dart_dev`, the `TestTool` handles this automatically by checking the
project's `pubspec.yaml` for a dependency on `build_test`. If present, tests
will be run via `pub run build_runner test`, otherwise it falls back to the
default of `pub run test`.
will be run via `dart run build_runner test`, otherwise it falls back to the
default of `dart test`.

```bash
# In a project without a `build_test` dependency:
$ ddev test
[INFO] Running subprocess:
pub run test
dart test
----------------------------
00:01 +75: All tests passed!


# In a project with a `build_test` dependency:
$ ddev test
[INFO] Running subprocess:
pub run build_runner test
dart run build_runner test
----------------------------
[INFO] Generating build script completed, took 425ms
[INFO] Creating build script snapshot... completed, took 13.6s
Expand All @@ -187,7 +187,7 @@ Running tests...
```

Additionally, `TestTool` automatically applies [`--build-filter`][build-filter]
options to the `pub run build_runner test` command to help reduce build time and
options to the `dart run build_runner test` command to help reduce build time and
speed up dev iteration when running a subset of the available tests.

Generally speaking, these dart tool abstractions provide a place to address
Expand Down Expand Up @@ -222,7 +222,7 @@ final config = {

// Override a target by including it after `...coreConfig`:
'format': FormatTool()
..formatter = Formatter.dartStyle,
..formatter = Formatter.dartFormat,

// Add a custom target:
'github': ProcessTool(
Expand Down
20 changes: 10 additions & 10 deletions doc/tools/test-tool.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,23 @@ final config = {

## `test` vs `build_runner test`

Historically, `pub run test` has been the canonical way to run Dart tests.
Historically, `dart test` has been the canonical way to run Dart tests.
With the introduction of the [build system][build-system], there is now a second
way to run tests. Projects that rely on builder outputs must run tests via
`pub run build_runner test`.
`dart run build_runner test`.

The `TestTool` will make this choice for you. If the current project has a
dependency on `build_test`, it will run `pub run build_runner test`. Otherwise
it will default to running `pub run test`.
dependency on `build_test`, it will run `dart run build_runner test`. Otherwise
it will default to running `dart test`.

> It [appears][test-future] as though the long term goal is to integrate the
> build system into the test runner so that `pub run test` is once again the
> build system into the test runner so that `dart test` is once again the
> canonical way to run tests.

## Default behavior

By default this tool will run `pub run test`, unless there is a dependency on
`build_test`, in which case it will run `pub run build_runner test`.
By default this tool will run `dart test`, unless there is a dependency on
`build_test`, in which case it will run `dart run build_runner test`.

## Running a subset of tests

Expand Down Expand Up @@ -71,10 +71,10 @@ Usage: dart_dev test [files or directories...]
======== Other Options
--test-stdout Write the test process stdout to this file path.
--test-args Args to pass to the test runner process.
Run "pub run test -h -v" to see all available options.
Run "dart test -h -v" to see all available options.

--build-args Args to pass to the build runner process.
Run "pub run build_runner test -h -v" to see all available options.
Run "dart run build_runner test -h -v" to see all available options.
Note: these args are only applicable if the current project depends on "build_test".

-h, --help Print this usage information.
Expand All @@ -89,7 +89,7 @@ iterating on tests much more efficient.
```bash
$ ddev test test/foo/bar/ test/baz_test.dart
[INFO] Running subprocess:
pub run build_runner test --build-filter=test/foo/bar/** --build-filter=test/baz_test.dart.*_test.dart.js --build-filter=test/baz_test.html -- test/foo/bar/ test/baz_test.dart
dart run build_runner test --build-filter=test/foo/bar/** --build-filter=test/baz_test.dart.*_test.dart.js --build-filter=test/baz_test.html -- test/foo/bar/ test/baz_test.dart
----------------------------------------------------------------------------
```

Expand Down
4 changes: 2 additions & 2 deletions doc/tools/tuneup-check-tool.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ final config = {

## Default behavior

By default this tool will run `pub run tuneup check` which will analyze all dart
By default this tool will run `dart run tuneup check` which will analyze all dart
files in the current project.

## Ignoring info outputs

By default, `pub run tuneup check` will include "info"-level analysis messages
By default, `dart run tuneup check` will include "info"-level analysis messages
in its output and fail if there are any. You can tell tuneup to ignore these:

```dart
Expand Down
2 changes: 1 addition & 1 deletion doc/tools/webdev-serve-tool.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ final config = {

## Default behavior

By default this tool will run `pub global run webdev serve` which will build the
By default this tool will run `dart pub global run webdev serve` which will build the
`web/` directory using the Dart Dev Compiler and serve it on port 8080.

## Configuration
Expand Down
81 changes: 54 additions & 27 deletions lib/src/tools/analyze_tool.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,19 @@ import '../utils/run_process_and_ensure_exit.dart';

final _log = Logger('Analyze');

/// A dart_dev tool that runs the `dartanalyzer` on the current project.
/// A dart_dev tool that runs the `dartanalyzer` or `dart analyze` on the current project.
/// If the `useDartAnalyze` flag is not specified it will default to `dartanalyzer`.
///
/// To use this tool in your project, include it in the dart_dev config in
/// `tool/dart_dev/config.dart`:
/// import 'package:dart_dev/dart_dev.dart';
///
/// final config = {
/// 'analyze': AnalyzeTool(),
/// 'analyze': AnalyzeTool() ..useDartAnalyze = true,
/// };
///
/// This will make it available via the `dart_dev` command-line app like so:
/// pub run dart_dev analyze
/// dart run dart_dev analyze
///
/// This tool can be configured by modifying any of its fields:
/// // tool/dart_dev/config.dart
Expand All @@ -36,14 +37,15 @@ final _log = Logger('Analyze');
/// 'analyze': AnalyzeTool()
/// ..analyzerArgs = ['--fatal-infos']
/// ..include = [Glob('.'), Glob('other/**.dart')],
/// ..useDartAnalyze = true
/// };
///
/// It is also possible to run this tool directly in a dart script:
/// AnalyzeTool().run();
class AnalyzeTool extends DevTool {
/// The args to pass to the `dartanalyzer` process run by this tool.
/// The args to pass to the `dartanalyzer` or `dart analyze` process run by this tool.
///
/// Run `dartanalyzer -h -v` to see all available args.
/// Run `dartanalyzer -h -v` or `dart analyze -h -v` to see all available args.
List<String> analyzerArgs;

/// The globs to include as entry points to run static analysis on.
Expand All @@ -52,28 +54,37 @@ class AnalyzeTool extends DevTool {
/// files in the current working directory.
List<Glob> include;

/// The default tool for analysis will be `dartanalyzer` unless opted in here
/// to utilize `dart analyze`.
annawatson-wk marked this conversation as resolved.
Show resolved Hide resolved
bool useDartAnalyze;

// ---------------------------------------------------------------------------
// DevTool Overrides
// ---------------------------------------------------------------------------

@override
final ArgParser argParser = ArgParser()
..addOption('analyzer-args',
help: 'Args to pass to the "dartanalyzer" process.\n'
'Run "dartanalyzer -h -v" to see all available options.');
help: 'Args to pass to the "dartanalyzer" or "dart analyze" process.\n'
'Run "dartanalyzer -h -v" or `dart analyze -h -v" to see all available options.');
annawatson-wk marked this conversation as resolved.
Show resolved Hide resolved

@override
String description = 'Run static analysis on dart files in this package.';

@override
FutureOr<int> run([DevToolExecutionContext context]) =>
runProcessAndEnsureExit(
buildProcess(context ?? DevToolExecutionContext(),
configuredAnalyzerArgs: analyzerArgs, include: include),
log: _log);
FutureOr<int> run([DevToolExecutionContext context]) {
useDartAnalyze ??= false;
return runProcessAndEnsureExit(
buildProcess(context ?? DevToolExecutionContext(),
configuredAnalyzerArgs: analyzerArgs,
include: include,
useDartAnalyze: useDartAnalyze),
log: _log);
}
}

/// Returns a combined list of args for the `dartanalyzer` process.
/// Returns a combined list of args for the `dartanalyzer`
/// or `dart analyze` process.
///
/// If [configuredAnalyzerArgs] is non-null, they will be included first.
///
Expand All @@ -82,18 +93,21 @@ class AnalyzeTool extends DevTool {
///
/// If [verbose] is true and the verbose flag (`-v`) is not already included, it
/// will be added.
Iterable<String> buildArgs({
ArgResults argResults,
List<String> configuredAnalyzerArgs,
bool verbose,
}) {
Iterable<String> buildArgs(
{ArgResults argResults,
List<String> configuredAnalyzerArgs,
bool useDartAnalyze,
bool verbose}) {
useDartAnalyze ??= false;
verbose ??= false;
final args = <String>[
// Combine all args that should be passed through to the dartanalyzer in
// Combine all args that should be passed through to the analyzer in
// this order:
// 1. Statically configured args from [AnalyzeTool.analyzerArgs]
// 1. The analyze command if using dart analyze
if (useDartAnalyze) 'analyze',
// 2. Statically configured args from [AnalyzeTool.analyzerArgs]
...?configuredAnalyzerArgs,
// 2. Args passed to --analyzer-args
// 3. Args passed to --analyzer-args
...?splitSingleOptionValue(argResults, 'analyzer-args'),
];
if (verbose && !args.contains('-v') && !args.contains('--verbose')) {
Expand Down Expand Up @@ -139,6 +153,9 @@ Iterable<String> buildEntrypoints({List<Glob> include, String root}) {
/// If non-null, [path] will override the current working directory for any
/// operations that require it. This is intended for use by tests.
///
/// If true, [useDartAnalyze] will utilize `dart analyze` for analysis.
/// If null, it will default to utilze `dartanalyzer`.
///
/// The [AnalyzeTool] can be tested almost completely via this function by
/// enumerating all of the possible parameter variations and making assertions
/// on the declarative output.
Expand All @@ -147,37 +164,47 @@ ProcessDeclaration buildProcess(
List<String> configuredAnalyzerArgs,
List<Glob> include,
String path,
bool useDartAnalyze,
annawatson-wk marked this conversation as resolved.
Show resolved Hide resolved
}) {
useDartAnalyze ??= false;
if (context.argResults != null) {
final analyzerUsed = useDartAnalyze ? 'dart analyze' : 'dartanalyzer';
assertNoPositionalArgsNorArgsAfterSeparator(
context.argResults, context.usageException,
commandName: context.commandName,
usageFooter:
'Arguments can be passed to the "dartanalyzer" process via '
'Arguments can be passed to the "${analyzerUsed}" process via '
'the --analyzer-args option.');
}
final executable = useDartAnalyze ? 'dart' : 'dartanalyzer';
final args = buildArgs(
argResults: context.argResults,
configuredAnalyzerArgs: configuredAnalyzerArgs,
verbose: context.verbose);
verbose: context.verbose,
useDartAnalyze: useDartAnalyze);
final entrypoints = buildEntrypoints(include: include, root: path);
logCommand(args, entrypoints, verbose: context.verbose);
return ProcessDeclaration('dartanalyzer', [...args, ...entrypoints],
logCommand(args, entrypoints,
verbose: context.verbose, useDartAnalyzer: useDartAnalyze);
return ProcessDeclaration(executable, [...args, ...entrypoints],
mode: ProcessStartMode.inheritStdio);
}

/// Logs the `dartanalyzer` command that will be run by [AnalyzeTool] so that
/// Logs the `dartanalyzer` or `dart analyze` command that will be run by [AnalyzeTool] so that
/// consumers can run it directly for debugging purposes.
///
/// Unless [verbose] is true, the list of entrypoints will be abbreviated to
/// avoid an unnecessarily long log.
void logCommand(
Iterable<String> args,
Iterable<String> entrypoints, {
bool useDartAnalyzer,
annawatson-wk marked this conversation as resolved.
Show resolved Hide resolved
bool verbose,
}) {
useDartAnalyzer ??= false;
verbose ??= false;
final exeAndArgs = 'dartanalyzer ${args.join(' ')}'.trim();
final exeAndArgs =
'${useDartAnalyzer ? "dart" : "dartanalyzer"} ${args.join(' ')}'.trim();

if (entrypoints.length <= 5 || verbose) {
logSubprocessHeader(_log, '$exeAndArgs ${entrypoints.join(' ')}');
} else {
Expand Down
Loading