-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[flutter_plugin_tools] Fix build-examples for packages #4248
Changes from 3 commits
43af78d
4544b2e
8aa5e54
4b989c7
0c3e993
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,38 +99,60 @@ class BuildExamplesCommand extends PackageLoopingCommand { | |
| Future<PackageResult> runForPackage(Directory package) async { | ||
| final List<String> errors = <String>[]; | ||
|
|
||
| final bool isPlugin = isFlutterPlugin(package); | ||
| final Iterable<_PlatformDetails> requestedPlatforms = _platforms.entries | ||
| .where( | ||
| (MapEntry<String, _PlatformDetails> entry) => getBoolArg(entry.key)) | ||
| .map((MapEntry<String, _PlatformDetails> entry) => entry.value); | ||
| final Set<_PlatformDetails> buildPlatforms = <_PlatformDetails>{}; | ||
| final Set<_PlatformDetails> unsupportedPlatforms = <_PlatformDetails>{}; | ||
| for (final _PlatformDetails platform in requestedPlatforms) { | ||
| if (pluginSupportsPlatform(platform.pluginPlatform, package)) { | ||
| buildPlatforms.add(platform); | ||
| } else { | ||
| unsupportedPlatforms.add(platform); | ||
| } | ||
| } | ||
|
|
||
| // Platform support is checked at the package level for plugins; there is | ||
| // no package-level platform information for non-plugin packages. | ||
| final Set<_PlatformDetails> buildPlatforms = isPlugin | ||
| ? requestedPlatforms | ||
| .where((_PlatformDetails platform) => | ||
| pluginSupportsPlatform(platform.pluginPlatform, package)) | ||
| .toSet() | ||
| : requestedPlatforms.toSet(); | ||
| if (buildPlatforms.isEmpty) { | ||
| final String unsupported = requestedPlatforms.length == 1 | ||
| ? '${requestedPlatforms.first.label} is not supported' | ||
| : 'None of [${requestedPlatforms.map((_PlatformDetails p) => p.label).join(',')}] are supported'; | ||
| : 'None of [${requestedPlatforms.map((_PlatformDetails p) => p.label).join(', ')}] are supported'; | ||
| return PackageResult.skip('$unsupported by this plugin'); | ||
| } | ||
| print('Building for: ' | ||
| '${buildPlatforms.map((_PlatformDetails platform) => platform.label).join(',')}'); | ||
| '${buildPlatforms.map((_PlatformDetails platform) => platform.label).join(', ')}'); | ||
|
|
||
| final Set<_PlatformDetails> unsupportedPlatforms = | ||
| requestedPlatforms.toSet().difference(buildPlatforms); | ||
| if (unsupportedPlatforms.isNotEmpty) { | ||
| final List<String> skippedPlatforms = unsupportedPlatforms | ||
| .map((_PlatformDetails platform) => platform.label) | ||
| .toList(); | ||
| skippedPlatforms.sort(); | ||
| print('Skipping unsupported platform(s): ' | ||
| '${unsupportedPlatforms.map((_PlatformDetails platform) => platform.label).join(',')}'); | ||
| '${skippedPlatforms.join(', ')}'); | ||
| } | ||
| print(''); | ||
|
|
||
| for (final Directory example in getExamplesForPlugin(package)) { | ||
| bool builtSomething = false; | ||
| for (final Directory example in getExamplesForPackage(package)) { | ||
| final String packageName = | ||
| getRelativePosixPath(example, from: packagesDir); | ||
|
|
||
| for (final _PlatformDetails platform in buildPlatforms) { | ||
| // Repo policy is that a plugin must have examples configured for all | ||
| // supported platforms. For packages, just log and skip any requested | ||
| // platform that a package doesn't have set up. | ||
| if (!isPlugin && | ||
| !example | ||
| .childDirectory(platform.flutterPlatformDirectory) | ||
| .existsSync()) { | ||
| print('Skipping ${platform.label} for $packageName; not supported.'); | ||
| continue; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Instead of using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I generally don't like to nest non-trivial primary logic in an else clause.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM, not my preference but not in violation of our style. Academics have argued about it for decades: https://en.wikipedia.org/wiki/Structured_programming#Early_exit |
||
| } | ||
|
|
||
| builtSomething = true; | ||
|
|
||
| String buildPlatform = platform.label; | ||
| if (platform.label.toLowerCase() != platform.flutterBuildType) { | ||
| buildPlatform += ' (${platform.flutterBuildType})'; | ||
|
|
@@ -143,6 +165,15 @@ class BuildExamplesCommand extends PackageLoopingCommand { | |
| } | ||
| } | ||
|
|
||
| if (!builtSomething) { | ||
| if (isPlugin) { | ||
| errors.add('No examples found'); | ||
| } else { | ||
| return PackageResult.skip( | ||
| 'No examples found supporting requested platform(s).'); | ||
| } | ||
| } | ||
|
|
||
| return errors.isEmpty | ||
| ? PackageResult.success() | ||
| : PackageResult.fail(errors); | ||
|
|
@@ -188,6 +219,11 @@ class _PlatformDetails { | |
| /// The `flutter build` build type. | ||
| final String flutterBuildType; | ||
|
|
||
| /// The Flutter platform directory name. | ||
| // In practice, this is the same as the plugin platform key for all platforms. | ||
| // If that changes, this can be adjusted. | ||
| String get flutterPlatformDirectory => pluginPlatform; | ||
|
|
||
| /// Any extra flags to pass to `flutter build`. | ||
| final List<String> extraBuildFlags; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,25 @@ enum PlatformSupport { | |
| federated, | ||
| } | ||
|
|
||
| /// Returns true if [package] is a Flutter plugin. | ||
| bool isFlutterPlugin(Directory package) { | ||
| try { | ||
| final File pubspecFile = package.childFile('pubspec.yaml'); | ||
| final YamlMap pubspecYaml = | ||
| loadYaml(pubspecFile.readAsStringSync()) as YamlMap; | ||
| final YamlMap? flutterSection = pubspecYaml['flutter'] as YamlMap?; | ||
| if (flutterSection == null) { | ||
| return false; | ||
| } | ||
| final YamlMap? pluginSection = flutterSection['plugin'] as YamlMap?; | ||
| return pluginSection != null; | ||
| } on FileSystemException { | ||
| return false; | ||
| } on YamlException { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be better to use explicit checks instead of relying on exceptions? It seems like a malformed yaml file would result in a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Removed the
In practice that wouldn't actually be a problem since we have a different CI test that validates the pubspecs, but I agree we shouldn't rely on it (and in fact, that's much more recent than this pubspec parsing code). I removed the exception handler for |
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /// Returns whether the given directory contains a Flutter [platform] plugin. | ||
| /// | ||
| /// It checks this by looking for the following pattern in the pubspec: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I ran into this too, it probably would be better to pull the join argument to a const or wrap that behavior in a function so it could be updated in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled out a local function for the two lines that were doing the same display list transform.