Skip to content

Commit

Permalink
[flutter_plugin_tools] Support YAML exception lists (#4183)
Browse files Browse the repository at this point in the history
Currently the tool accepts `--custom-analysis` to allow a list of packages for which custom `analysis_options.yaml` are allowed, and `--exclude` to exclude a set of packages when running a command against all, or all changed, packages. This results in these exception lists being embedded into CI configuration files (e.g., .cirrus.yaml) or scripts, which makes them harder to maintain, and harder to re-use in other contexts (local runs, new CI systems).

This adds support for both flags to accept paths to YAML files that contain the lists, so that they can be maintained separately, and with inline comments about the reasons things are on the lists.

Also updates the CI to use this new support, eliminating those lists from `.cirrus.yaml` and `tool_runner.sh`

Fixes flutter/flutter#86799
  • Loading branch information
stuartmorgan authored Jul 22, 2021
1 parent 97178af commit d17489c
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 4 deletions.
4 changes: 4 additions & 0 deletions script/tool/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
## NEXT

- `--exclude` and `--custom-analysis` now accept paths to YAML files that
contain lists of packages to exclude, in addition to just package names,
so that exclude lists can be maintained separately from scripts and CI
configuration.
- Added an `xctest` flag to select specific test targets, to allow running only
unit tests or integration tests.
- **Breaking change**: Split Xcode analysis out of `xctest` and into a new
Expand Down
21 changes: 19 additions & 2 deletions script/tool/lib/src/analyze_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'dart:async';

import 'package:file/file.dart';
import 'package:platform/platform.dart';
import 'package:yaml/yaml.dart';

import 'common/core.dart';
import 'common/package_looping_command.dart';
Expand All @@ -23,7 +24,10 @@ class AnalyzeCommand extends PackageLoopingCommand {
}) : super(packagesDir, processRunner: processRunner, platform: platform) {
argParser.addMultiOption(_customAnalysisFlag,
help:
'Directories (comma separated) that are allowed to have their own analysis options.',
'Directories (comma separated) that are allowed to have their own '
'analysis options.\n\n'
'Alternately, a list of one or more YAML files that contain a list '
'of allowed directories.',
defaultsTo: <String>[]);
argParser.addOption(_analysisSdk,
valueHelp: 'dart-sdk',
Expand All @@ -37,6 +41,8 @@ class AnalyzeCommand extends PackageLoopingCommand {

late String _dartBinaryPath;

Set<String> _allowedCustomAnalysisDirectories = const <String>{};

@override
final String name = 'analyze';

Expand All @@ -56,7 +62,7 @@ class AnalyzeCommand extends PackageLoopingCommand {
continue;
}

final bool allowed = (getStringListArg(_customAnalysisFlag)).any(
final bool allowed = _allowedCustomAnalysisDirectories.any(
(String directory) =>
directory.isNotEmpty &&
path.isWithin(
Expand Down Expand Up @@ -107,6 +113,17 @@ class AnalyzeCommand extends PackageLoopingCommand {
throw ToolExit(_exitPackagesGetFailed);
}

_allowedCustomAnalysisDirectories =
getStringListArg(_customAnalysisFlag).expand<String>((String item) {
if (item.endsWith('.yaml')) {
final File file = packagesDir.fileSystem.file(item);
return (loadYaml(file.readAsStringSync()) as YamlList)
.toList()
.cast<String>();
}
return <String>[item];
}).toSet();

// Use the Dart SDK override if one was passed in.
final String? dartSdk = argResults![_analysisSdk] as String?;
_dartBinaryPath =
Expand Down
17 changes: 15 additions & 2 deletions script/tool/lib/src/common/plugin_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:file/file.dart';
import 'package:git/git.dart';
import 'package:path/path.dart' as p;
import 'package:platform/platform.dart';
import 'package:yaml/yaml.dart';

import 'core.dart';
import 'git_version_finder.dart';
Expand Down Expand Up @@ -48,7 +49,9 @@ abstract class PluginCommand extends Command<void> {
argParser.addMultiOption(
_excludeArg,
abbr: 'e',
help: 'Exclude packages from this command.',
help: 'A list of packages to exclude from from this command.\n\n'
'Alternately, a list of one or more YAML files that contain a list '
'of packages to exclude.',
defaultsTo: <String>[],
);
argParser.addFlag(_runOnChangedPackagesArg,
Expand Down Expand Up @@ -214,8 +217,18 @@ abstract class PluginCommand extends Command<void> {
/// of packages in the flutter/packages repository.
Stream<Directory> _getAllPlugins() async* {
Set<String> plugins = Set<String>.from(getStringListArg(_packagesArg));

final Set<String> excludedPlugins =
Set<String>.from(getStringListArg(_excludeArg));
getStringListArg(_excludeArg).expand<String>((String item) {
if (item.endsWith('.yaml')) {
final File file = packagesDir.fileSystem.file(item);
return (loadYaml(file.readAsStringSync()) as YamlList)
.toList()
.cast<String>();
}
return <String>[item];
}).toSet();

final bool runOnChangedPackages = getBoolArg(_runOnChangedPackagesArg);
if (plugins.isEmpty &&
runOnChangedPackages &&
Expand Down
19 changes: 19 additions & 0 deletions script/tool/test/analyze_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,25 @@ void main() {
]));
});

test('takes an allow config file', () async {
final Directory pluginDir = createFakePlugin('foo', packagesDir,
extraFiles: <String>['analysis_options.yaml']);
final File allowFile = packagesDir.childFile('custom.yaml');
allowFile.writeAsStringSync('- foo');

await runCapturingPrint(
runner, <String>['analyze', '--custom-analysis', allowFile.path]);

expect(
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
ProcessCall(
'flutter', const <String>['packages', 'get'], pluginDir.path),
ProcessCall('dart', const <String>['analyze', '--fatal-infos'],
pluginDir.path),
]));
});

// See: https://github.com/flutter/flutter/issues/78994
test('takes an empty allow list', () async {
createFakePlugin('foo', packagesDir,
Expand Down
13 changes: 13 additions & 0 deletions script/tool/test/common/plugin_command_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,19 @@ void main() {
expect(plugins, unorderedEquals(<String>[plugin2.path]));
});

test('exclude accepts config files', () async {
createFakePlugin('plugin1', packagesDir);
final File configFile = packagesDir.childFile('exclude.yaml');
configFile.writeAsStringSync('- plugin1');

await runCapturingPrint(runner, <String>[
'sample',
'--packages=plugin1',
'--exclude=${configFile.path}'
]);
expect(plugins, unorderedEquals(<String>[]));
});

group('test run-on-changed-packages', () {
test('all plugins should be tested if there are no changes.', () async {
final Directory plugin1 = createFakePlugin('plugin1', packagesDir);
Expand Down

0 comments on commit d17489c

Please sign in to comment.