Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[plugin_tools] build-examples .pluginToolsConfig.yaml support #4305

Merged
merged 1 commit into from
Sep 3, 2021
Merged

[plugin_tools] build-examples .pluginToolsConfig.yaml support #4305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions script/tool/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.7.1

- Add support for `.pluginToolsConfig.yaml` in the `build-examples` command.

## 0.7.0

- `native-test` now supports `--linux` for unit tests.
Expand Down
74 changes: 73 additions & 1 deletion script/tool/lib/src/build_examples_command.dart
View file
Open in desktop
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 @@ -16,7 +17,19 @@ import 'common/repository_package.dart';
/// Key for APK.
const String _platformFlagApk = 'apk';

const String _pluginToolsConfigFileName = '.pluginToolsConfig.yaml';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a super-generic name. If we're going to use this, I think we should make the YAML format not just a flat list so that we can add other tool configuration options there in the future (which I think is actually fairly likely) without breaking everything. E.g.:

buildFlags:
  global:
    - foo
    - bar

That would let us add both arbitrary other tool config options, and also other build flags (like platform specific flags) later without a format break.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can know what format to pick to avoid a format break, but ok. (For example, what if we discover that actually what we want is for this to be a list of buildFlags, so that you can try running with several configurations? So the root should be a list rather than a map? Or, maybe the root should be platforms, with buildFlags as the second level. Or...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when your root is a map you can always change things without it being a hard breaking change, because you can just use a different root key for the new format. Then eventually you can remove support for the old key.

(In this case the cost of the breaking change would be pretty low since we only guarantee that the tool works for two repos that we control; maybe I'm over-engineering based on experiences with things where it's much harder when you can't do the incremental change via a map.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think things are a lot less clear-cut in practice than this (e.g. changing from a list to a map is non-breaking too since you can just keep on supporting the old list mode, and sometimes a map's keyspace is complete, e.g. the semantics are that you do something for each key, in which case the old key would conflict with the new keyspace, etc), but it's not a big deal either way.

const String _pluginToolsConfigBuildFlagsKey = 'buildFlags';
const String _pluginToolsConfigGlobalKey = 'global';

const String _pluginToolsConfigExample = '''
$_pluginToolsConfigBuildFlagsKey:
$_pluginToolsConfigGlobalKey:
- "--no-tree-shake-icons"
- "--dart-define=buildmode=testing"
''';

const int _exitNoPlatformFlags = 3;
const int _exitInvalidPluginToolsConfig = 4;

// Flutter build types. These are the values passed to `flutter build <foo>`.
const String _flutterBuildTypeAndroid = 'apk';
Expand Down Expand Up @@ -99,7 +112,13 @@ class BuildExamplesCommand extends PackageLoopingCommand {
@override
final String description =
'Builds all example apps (IPA for iOS and APK for Android).\n\n'
'This command requires "flutter" to be in your path.';
'This command requires "flutter" to be in your path.\n\n'
'A $_pluginToolsConfigFileName file can be placed in an example app '
'directory to specify additional build arguments. It should be a YAML '
'file with a top-level map containing a single key '
'"$_pluginToolsConfigBuildFlagsKey" containing a map containing a '
'single key "$_pluginToolsConfigGlobalKey" containing a list of build '
'arguments.';

@override
Future<void> initializeRun() async {
Expand Down Expand Up @@ -202,6 +221,58 @@ class BuildExamplesCommand extends PackageLoopingCommand {
: PackageResult.fail(errors);
}

Iterable<String> _readExtraBuildFlagsConfiguration(
Directory directory) sync* {
final File pluginToolsConfig =
directory.childFile(_pluginToolsConfigFileName);
if (pluginToolsConfig.existsSync()) {
final Object? configuration =
loadYaml(pluginToolsConfig.readAsStringSync());
if (configuration is! YamlMap) {
printError('The $_pluginToolsConfigFileName file must be a YAML map.');
printError(
'Currently, the key "$_pluginToolsConfigBuildFlagsKey" is the only one that has an effect.');
printError(
'It must itself be a map. Currently, in that map only the key "$_pluginToolsConfigGlobalKey"');
printError(
'has any effect; it must contain a list of arguments to pass to the');
printError('flutter tool.');
printError(_pluginToolsConfigExample);
throw ToolExit(_exitInvalidPluginToolsConfig);
}
if (configuration.containsKey(_pluginToolsConfigBuildFlagsKey)) {
final Object? buildFlagsConfiguration =
configuration[_pluginToolsConfigBuildFlagsKey];
if (buildFlagsConfiguration is! YamlMap) {
printError(
'The $_pluginToolsConfigFileName file\'s "$_pluginToolsConfigBuildFlagsKey" key must be a map.');
printError(
'Currently, in that map only the key "$_pluginToolsConfigGlobalKey" has any effect; it must ');
printError(
'contain a list of arguments to pass to the flutter tool.');
printError(_pluginToolsConfigExample);
throw ToolExit(_exitInvalidPluginToolsConfig);
}
if (buildFlagsConfiguration.containsKey(_pluginToolsConfigGlobalKey)) {
final Object? globalBuildFlagsConfiguration =
buildFlagsConfiguration[_pluginToolsConfigGlobalKey];
if (globalBuildFlagsConfiguration is! YamlList) {
printError(
'The $_pluginToolsConfigFileName file\'s "$_pluginToolsConfigBuildFlagsKey" key must be a map');
printError('whose "$_pluginToolsConfigGlobalKey" key is a list.');
printError(
'That list must contain a list of arguments to pass to the flutter tool.');
printError(
'For example, the $_pluginToolsConfigFileName file could look like:');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional nit: are the slight variations of error messages actually buying us anything here over just having one constant (like the example) for any invalid structure, that describes the correct structure? It doesn't seem like someone is going to have a hard time figuring out which part they've done wrong from a catch-all message and example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my experience, more detail in error messages (if it's accurate) is always more helpful. :-)

printError(_pluginToolsConfigExample);
throw ToolExit(_exitInvalidPluginToolsConfig);
}
yield* globalBuildFlagsConfiguration.cast<String>();
}
}
}
}

Future<bool> _buildExample(
RepositoryPackage example,
String flutterBuildType, {
Expand Down Expand Up @@ -231,6 +302,7 @@ class BuildExamplesCommand extends PackageLoopingCommand {
'build',
flutterBuildType,
...extraBuildFlags,
..._readExtraBuildFlagsConfiguration(example.directory),
if (enableExperiment.isNotEmpty)
'--enable-experiment=$enableExperiment',
],
Expand Down
2 changes: 1 addition & 1 deletion script/tool/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name: flutter_plugin_tools
description: Productivity utils for flutter/plugins and flutter/packages
repository: https://github.com/flutter/plugins/tree/master/script/tool
version: 0.7.0
version: 0.7.1

dependencies:
args: ^2.1.0
Expand Down
48 changes: 45 additions & 3 deletions script/tool/test/build_examples_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,6 @@ void main() {
]),
);

print(processRunner.recordedCalls);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I thought I'd caught all of these during all the refactoring/cleanup work. Apparently it spread to more places than I'd noticed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's still some prints somewhere in the tests but i couldn't track them down in the 10 seconds i spent trying. It says "Determine diff with base sha:" sometimes, and "null" once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Determine diff with base sha:" sometimes

I bet that's from a test of the helper directly, not going through a command runner that captures output. That helper really shouldn't log anyway, I just haven't gotten around to fixing it.

// Output should be empty since running build-examples --macos with no macos
// implementation is a no-op.
expect(processRunner.recordedCalls, orderedEquals(<ProcessCall>[]));
Expand Down Expand Up @@ -407,7 +406,6 @@ void main() {
]),
);

print(processRunner.recordedCalls);
expect(
processRunner.recordedCalls,
containsAll(<ProcessCall>[
Expand Down Expand Up @@ -436,7 +434,6 @@ void main() {
contains('Creating temporary winuwp folder'),
);

print(processRunner.recordedCalls);
expect(
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
Expand Down Expand Up @@ -679,5 +676,50 @@ void main() {
]));
});
});

test('The .pluginToolsConfig.yaml file', () async {
mockPlatform.isLinux = true;
final Directory pluginDirectory = createFakePlugin('plugin', packagesDir,
platformSupport: <String, PlatformDetails>{
kPlatformLinux: const PlatformDetails(PlatformSupport.inline),
kPlatformMacos: const PlatformDetails(PlatformSupport.inline),
});

final Directory pluginExampleDirectory =
pluginDirectory.childDirectory('example');

final File pluginExampleConfigFile =
pluginExampleDirectory.childFile('.pluginToolsConfig.yaml');
pluginExampleConfigFile
.writeAsStringSync('buildFlags:\n global:\n - "test argument"');

final List<String> output = <String>[
...await runCapturingPrint(
runner, <String>['build-examples', '--linux']),
...await runCapturingPrint(
runner, <String>['build-examples', '--macos']),
];

expect(
output,
containsAllInOrder(<String>[
'\nBUILDING plugin/example for Linux',
'\nBUILDING plugin/example for macOS',
]),
);

expect(
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
ProcessCall(
getFlutterCommand(mockPlatform),
const <String>['build', 'linux', 'test argument'],
pluginExampleDirectory.path),
ProcessCall(
getFlutterCommand(mockPlatform),
const <String>['build', 'macos', 'test argument'],
pluginExampleDirectory.path),
]));
});
});
}