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

[url_launcher_windows] Support win32 and UWP hosts #3828

Closed
clarkezone wants to merge 18 commits into from
Closed

[url_launcher_windows] Support win32 and UWP hosts #3828

clarkezone wants to merge 18 commits into from

Conversation

@clarkezone
Copy link

@clarkezone clarkezone commented Apr 27, 2021

This PR adds support for the winuwp target.

flutter/flutter#70207
flutter/flutter#14967

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format. See plugin_tool format)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@clarkezone clarkezone self-assigned this Apr 27, 2021
@google-cla google-cla bot added the cla: yes label Apr 27, 2021
@stuartmorgan-g stuartmorgan-g mentioned this pull request Apr 27, 2021
Closed
@clarkezone
Copy link
Author

clarkezone commented Apr 27, 2021

FWIW I noticed last night that licenses needed updating but didn't have time to locate the licenses file

stuartmorgan-g
stuartmorgan-g suggested changes Apr 27, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment •
edited
Loading

Choose a reason for hiding this comment

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

High level before reviewing the code itself:

  1. This appears to break Win32 builds. I can't see why (flutter/flutter#81297) but hopefully it's reproducible locally.
  2. This needs to actually be built in CI.
  3. Relatedly, this needs to be running the integration tests.

For 2, what's the current status of UWP runners? Does flutter create . work? Is the template still unstable? If Yes and Yes, we can add the code to create it on the fly that we used to use for other desktop builds here.

For 3, is there a LUCI recipe that can run UWP apps yet?

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Apr 27, 2021

FWIW I noticed last night that licenses needed updating but didn't have time to locate the licenses file

The correct license form for this repo is what was in url_launcher_plugin.cpp before you changed it. You just need to revert that change, and use that text in the other files.

@clarkezone
Copy link
Author

clarkezone commented Apr 28, 2021

  1. The win32 build of url_launcher example works fine locally so not sure how to reason over the failure. Not sure if the infra is also trying to run in UWP mode?
  2. Flutter create isn't ready yet but in progress so this one is currently blocked
  3. I don't believe so. This would likely need WinAppDriver support (https://github.com/Microsoft/WinAppDriver) in LUCI. @cbracken for thoughts on this

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Apr 28, 2021

  1. The win32 build of url_launcher example works fine locally so not sure how to reason over the failure. Not sure if the infra is also trying to run in UWP mode?

I'm not clear on what this means. How would flutter build windows be UWP mode?

@clarkezone
Copy link
Author

clarkezone commented Apr 29, 2021 •
edited
Loading

  1. The win32 build of url_launcher example works fine locally so not sure how to reason over the failure. Not sure if the infra is also trying to run in UWP mode?

I'm not clear on what this means. How would flutter build windows be UWP mode?

flutter build windows is not uwp. If that is what CI is doing (only) which I guess it is, then it works fine locally hence I cannot reason over why CI is failing.

@clarkezone
Copy link
Author

clarkezone commented May 4, 2021

Seeing this in the latest run..

Checking Dart SDK version... Downloading Dart SDK from Flutter engine ... Building flutter tool... Running pub upgrade... Got TLS error trying to find package args at https://pub.dartlang.org. Error (69): Unable to 'pub upgrade' flutter tool. Retrying in five seconds... (9 tries left) Running pub upgrade... Got TLS error trying to find package html at https://pub.dartlang.org. Error (69): Unable to 'pub upgrade' flutter tool. Retrying in five seconds... (8 tries left) Running pub upgrade... Got TLS error trying to find package flutter_template_images at https://pub.dartlang.org. Error (69): Unable to 'pub upgrade' flutter tool. Retrying in five seconds... (7 tries left) Running pub upgrade... Terminate batch job (Y/N)? ^CError (-1073741510): Unable to 'pub upgrade' flutter tool. Retrying in five seconds... (6 tries left) Running pub upgrade...

@cbracken
Copy link
Member

cbracken commented May 4, 2021

@clarkezone this is likely the result of flutter/flutter#81837

@clarkezone clarkezone mentioned this pull request May 8, 2021
Closed
18 tasks
@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented May 12, 2021

@clarkezone Could you merge in master and see what happens? I've temporarily replaced the currently-completely-broken Windows LUCI CI with GA-based CI and I'm curious if it will give us different results.

@domesticmouse
Copy link
Contributor

domesticmouse commented May 17, 2021

@stuartmorgan I believe flutter create now works on Flutter master channel, once flutter config --enable-windows-uwp-desktop has been run.

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented May 17, 2021

I tried building locally, and hit compile warnings (treated as errors) in cppwinrt/winrt/base.h (SDK version 18362). I'll try upgrading my SDK version, but if we're not going to require a higher SDK version for UWP (doctor didn't complain) we should adjust the build flags in the CMake file here.

@clarkezone
Copy link
Author

clarkezone commented May 17, 2021 •
edited
Loading

Do u have UWP workload installed along with c++ support for UWP? 18362 is fine.

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented May 17, 2021

I'll have to check. Is that's the issue, then doctor will definitely need an update.

@clarkezone
Copy link
Author

clarkezone commented May 17, 2021

Yea it's on the list @cbracken @jonahwilliams

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented May 17, 2021

Tomorrow I'll file an issue laying out the pieces still left to unblock plugins landing here, for tracking.

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented May 18, 2021

flutter/flutter#82822 tracks the blockers. We can add more there if we think of anything else.

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Sep 14, 2021

There's still no flutter drive support, but this can be rebased against master where we have build support for UWP plugins to ensure that everything is building as expected.

@clarkezone
Copy link
Author

clarkezone commented Sep 14, 2021

I just got unblocked on building engine, i will rebase on push this sometime as soon as i can

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Sep 16, 2021

I realized that my restructure of the plugin to make it testable caused a bunch of non-trivial conflicts, so I went ahead and resolved it. The resolution is hacky (ifdefs in a header, splitting class implementation between several files) so it'll need cleanup, but it's at least a cleaner starting point that the pile of conflicts.

I haven't tested the UWP part since I still don't have that working on my Windows machine, so apologies if I broke that. But we can at least see if CI builds it :)

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Sep 16, 2021

Hm, the UWP build timed out on CI, which is what happened to me locally. I thought that was just something about my setup, but apparently not.

@clarkezone
Copy link
Author

clarkezone commented Sep 24, 2021

Interesting.

@clarkezone
Copy link
Author

clarkezone commented Sep 26, 2021

I'm blocked currently due to inability to build the UWP target on my machine.

@stuartmorgan-g stuartmorgan-g mentioned this pull request Oct 11, 2021
Closed
@chongbo2013

This comment has been minimized.

Sign in to view
@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Dec 19, 2021

@cbracken @clarkezone Do we have a path to getting this unblocked, or should we close this out for now?

@clarkezone
Copy link
Author

clarkezone commented Jan 6, 2022

I got unblocked before the holidays but haven't had a chance to look at this since then, appols.

@cbracken
Copy link
Member

cbracken commented Jan 6, 2022 •
edited
Loading

I have this and a couple other plugins PRs at the top of my review queue now that I’m back from holidays. Looks like there’s some work to do first though?

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Jan 6, 2022

Looks like there’s some work to do first though?

Yes, this PR currently hangs UWP builds (at least for me and CI). It's possible I broke it with my update to the PR, but IIRC I already had that problem.

@godofredoc godofredoc changed the base branch from master to main January 6, 2022 23:33
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jan 6, 2022

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@stuartmorgan-g stuartmorgan-g marked this pull request as draft January 26, 2022 21:11
@cbracken
Copy link
Member

cbracken commented Jan 27, 2022

@clarkezone is this something that you expect you'll pick up any time soon? If not, shall we close it?

@clarkezone
Copy link
Author

clarkezone commented Feb 2, 2022

I aspire to landing this but I haven't done a good job at making any progress. If you'd be happier closing it out I'm OK with that.

@cbracken
Copy link
Member

cbracken commented Mar 1, 2022

SGTM. Closing for now. When you get time and it's ready for review; fire a new PR our way!

@cbracken cbracken closed this Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cbracken cbracken Awaiting requested review from cbracken cbracken is a code owner

1 more reviewer

@stuartmorgan-g stuartmorgan-g stuartmorgan-g requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@clarkezone clarkezone

Labels

cla: yes needs tests p: url_launcher platform-windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@clarkezone @stuartmorgan-g @cbracken @domesticmouse @chongbo2013