- Notifications
You must be signed in to change notification settings - Fork9.7k
[flutter_plugin_tool] Add support for building UWP plugins#4047
[flutter_plugin_tool] Add support for building UWP plugins#4047stuartmorgan-g merged 20 commits intoflutter:masterfrom
Conversation
stuartmorgan-g commentedJun 11, 2021
Sorry this turned into a yak-shave; once I was changing that test utility method it was very hard to stop... |
stuartmorgan-g commentedJun 17, 2021
Hold off on reviewing; I re-implemented most of the yak as a prequel PR. |
stuartmorgan-g commentedJun 21, 2021
Now with 100% less yak. |
cbracken left a comment
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.
lgtm modulo a few nits.
| // needs to be created on the fly with 'flutter create .' | ||
| Directory? temporaryPlatformDirectory; | ||
| if (flutterBuildType == 'winuwp') { | ||
| final Directory uwpFolder = example.childDirectory('winuwp'); |
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.
pedantic nit: consideruwpDirectory for consistent naming with the APIs, unless folder is used consistently throughout the tool already.
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.
Fixed (that's my macOS showing).
| if (!uwpFolder.existsSync()) { | ||
| print('Creating temporary winuwp folder'); | ||
| final int exitCode = await processRunner.runAndStream( | ||
| flutterCommand, <String>['create', '--platforms=winuwp', '.'], |
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.
Is thiswinuwp the same as theplatformVariantWinUwp orkPlatformWinUwp const incore.dart? If so consider using a string interpolation here both to indicate the connection, and because it'll break this code if we ever removed that const.
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.
Is this
winuwpthe same as theplatformVariantWinUwporkPlatformWinUwpconst incore.dart?
Hm. I guess it's probably the same as kPlatformWinUwp now that you mention it ("UWP is a platform for the purposes of flutter commands likebuild andrun"); presumably the fact thatbuild andcreate are using the same string influtter isn't an accident.
The "platform" situation is kind of a disaster. I think the thing to do is to explore doing a follow-up PR that makes a tool-wide platform class that's based on the_PlatformDetails I had to define here, because it's a more general problem (as indicated by the constant names we have globally). That can have accessors for each different usage; mostly that will be the same thing in each case, but not always, and we can pass around that object internally everywhere instead of strings (and only convert to strings at the last minute when interfacing with things outside the tool).
| PlatformSupport? requiredMode, | ||
| String? variant, | ||
| }) { | ||
| assert(platform == kPlatformIos || |
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.
Should this be updated to includekPlatformWinUwp or is that always passed askPlatformWindows with the uwp variant?
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.
The latter; from the standpoint of a plugin, UWP isn't a platform.
(But see my other comment; I think I could avoid this kind of confusion with a generic platform class.)
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.
Agreed; the backward compatibility makes this a bit of a pain. As you say, modelling this with classes might result in better readability than with logic in various bits of the tool. Agree that approach is worth exploring.
| // The UWP template is not yet stable, so the UWP directory | ||
| // needs to be created on the fly with 'flutter create .' | ||
| Directory? temporaryPlatformDirectory; | ||
| if (flutterBuildType == 'winuwp') { |
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.
It looks like these are currently literals for all the platforms; consider extracting them to build type consts.
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.
Done.
| ])); | ||
| }); | ||
| test('driving UWP is a no-op', () async { |
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.
Might be useful to add a TODO here and elsewhere with a link to a bug.
stuartmorgan-g commentedAug 26, 2021
The red is a flaky test that I have a PR out for review to disable, so I'm going to land without wasting the resources of re-running it until it's green. |
)This allows building UWP plugin examples with `build-examples --winuwp`. As with previous pre-stable-template desktop support, this avoids the issue of unstable app templates by running `flutter create` on the fly before trying to build, so a template that will bitrot doesn't need to be checked in.Also adds no-op "support" for `drive-examples --winuwp`, with warnings about it not doing anything. This is to handle the fact that the LUCI recipe is shared between Win32 and UWP, and didn't conditionalize `drive`. Rather than change that, then change it back later, this just adds the no-op support now (since changing the tooling is much easier than changing LUCI recipes currently).This required some supporting tool changes:- Adds the ability to check for the new platform variants in a pubspec- Adds the ability to write test pubspecs that include variants, for testingPart offlutter/flutter#82817
)This allows building UWP plugin examples with `build-examples --winuwp`. As with previous pre-stable-template desktop support, this avoids the issue of unstable app templates by running `flutter create` on the fly before trying to build, so a template that will bitrot doesn't need to be checked in.Also adds no-op "support" for `drive-examples --winuwp`, with warnings about it not doing anything. This is to handle the fact that the LUCI recipe is shared between Win32 and UWP, and didn't conditionalize `drive`. Rather than change that, then change it back later, this just adds the no-op support now (since changing the tooling is much easier than changing LUCI recipes currently).This required some supporting tool changes:- Adds the ability to check for the new platform variants in a pubspec- Adds the ability to write test pubspecs that include variants, for testingPart offlutter/flutter#82817
Uh oh!
There was an error while loading.Please reload this page.
This allows building UWP plugin examples with
build-examples --winuwp. As with previous pre-stable-template desktop support, this avoids the issue of unstable app templates by runningflutter createon the fly before trying to build, so a template that will bitrot doesn't need to be checked in.Also adds no-op "support" for
drive-examples --winuwp, with warnings about it not doing anything. This is to handle the fact that the LUCI recipe is shared between Win32 and UWP, and didn't conditionalizedrive. Rather than change that, then change it back later, this just adds the no-op support now (since changing the tooling is much easier than changing LUCI recipes currently).This required some supporting tool changes:
Part offlutter/flutter#82817
Pre-launch Checklist
dart format.)[shared_preferences]///).