[google_sign_in] Add macOS support #4962

stuartmorgan · 2023-09-21T14:50:55Z

Adds macOS support to google_sign_in_ios by moving the existing source to darwin/, adjusting it somewhat to be macOS-compatible, and enabling sharedDarwinSource in the podspec.

Unlike previous cases of code sharing, this doesn't rename the package to be something more generic, since when we do that the name is based on the underlying SDK, and the SDK is still mostly called "Google Sign-In for iOS", so I don't think there's a better name to give it at the moment.

Most of flutter/flutter#46157
(Once this lands, there will be a follow-up PR to update the app-facing package to endorse it and list it in the README.)

Pre-launch Checklist

…assing

flutter-dashboard · 2023-09-21T14:50:58Z

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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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 · 2023-09-21T14:52:17Z

test-exempt: Existing tests are being enabled for macOS in this PR (the bot just doesn't recognize the path).

stuartmorgan · 2023-09-21T14:53:36Z

This isn't landable yet since it won't compile for macOS on the current stable, but I wanted to post it to ensure CI doesn't find any other issues. It can be reviewed now though, since the only change to actually land it (modulo fixing anything else CI turns up) will be bumping the min SDK version again after the next stable release.

stuartmorgan · 2023-09-21T14:55:40Z

packages/google_sign_in/google_sign_in_ios/darwin/Classes/FLTGoogleSignInPlugin.m

-                          }
-                          completion(error == nil ? @(granted) : nil, error);
-                        }];
+    [self addScopes:requestedScopes.allObjects


The reason it looks like there are a lot of changes here (and in the previous block) is that switching to the new helper function without presentingViewController changed the indentation of the whole block. The code in the block doesn't have any non-whitespace changes.

stuartmorgan · 2023-09-21T14:57:08Z

packages/google_sign_in/google_sign_in_ios/darwin/Tests/GoogleSignInTests.m

-                                       code:kGIDSignInErrorCodeHasNoAuthInKeychain
-                                   userInfo:nil];
-  [[self.mockSignIn stub] disconnectWithCallback:[OCMArg invokeBlockWithArgs:error, nil]];
+  NSError *sdkError = [NSError errorWithDomain:kGIDSignInErrorDomain


All the error renaming is because the macOS build settings catch variable shadowing, which apparently our iOS build doesn't have enabled, and the blocks in all of these methods have an error parameter.

Is there an issue filed to get the iOS lints consistent with the macOS lints? (also raises the question of whether macOS is missing any iOS lints)

stuartmorgan · 2023-09-21T14:57:59Z

packages/google_sign_in/google_sign_in_ios/darwin/Tests/GoogleSignInTests.m

-                          hint:nil
-              additionalScopes:OCMOCK_ANY
-                      callback:OCMOCK_ANY]);
+  OCMVerify([self configureMock:self.mockSignIn


As with the changes in the implementation file, all of these changes are whitespace changes from the switch to the new configureMock helper.

stuartmorgan · 2023-09-22T22:42:18Z

since the only change to actually land it (modulo fixing anything else CI turns up) will be bumping the min SDK version again after the next stable release

And I'll have to publish a new Flutter Pod so that the pod linter passes.

vashworth

The iOS side LGTM, but looks like the macOS side is failing to build with

no visible @interface for 'NSObject<FlutterPluginRegistrar>'

stuartmorgan · 2023-09-25T17:26:54Z

That's:

This isn't landable yet since it won't compile for macOS on the current stable, but I wanted to post it to ensure CI doesn't find any other issues.

The PR uses API that I added to the macOS engine recently, so doesn't exist on stable yet.

vashworth

LGTM

cbracken

lgtm!

behrangnm · 2023-10-16T22:15:50Z

That's:

This isn't landable yet since it won't compile for macOS on the current stable, but I wanted to post it to ensure CI doesn't find any other issues.

The PR uses API that I added to the macOS engine recently, so doesn't exist on stable yet.

How would I test this on my flutter project for building a macos app that uses google_sing_io with your changes?
Thanks for the PR!

stuartmorgan · 2023-10-16T22:31:19Z

How would I test this

See flutter/flutter#46157 (comment)

FelixMethe · 2023-10-17T20:46:55Z

Thank you for the PR!
I'm having troubles getting it to work for me.

After completing the authentication in the Google Popup, the Future and the currentUser getter returns null.
(The same code works on iOS)

I can tell that the authentication never completes, as I am missing the email notification from Google.

The only error/warning I get is this:
2023-10-17 16:29:14.739 flutter_google_signin_test[17035:149636] WARNING: Secure coding is automatically enabled for restorable state! However, not on all supported macOS versions of this application. Opt-in to secure coding explicitly by implementing NSApplicationDelegate.applicationSupportsSecureRestorableState:.
This prints when the popup opens.
It appears to be a macOS Sonoma thing. I don't think this is the problem.

chenenyu · 2023-11-20T12:38:59Z

Hi, when will this PR be merged? 😆

stuartmorgan · 2023-11-27T15:20:23Z

Hi, when will this PR be merged? 😆

After I have time to manually publish a new FlutterMacOS pod so that CI analysis can pass.

flutter/packages@ca16173...15584a3 2023-12-07 ditman@gmail.com [gis_web] Migrate to package:web. (flutter/packages#5581) 2023-12-07 kevmoo@users.noreply.github.com Drop quiver usage in several packages (flutter/packages#5595) 2023-12-07 stuartmorgan@google.com [video_player] Unfork publish: for macOS (flutter/packages#5578) 2023-12-07 kevmoo@users.noreply.github.com [web_benchmarks] migrate to pkg:web (flutter/packages#5592) 2023-12-07 43054281+camsim99@users.noreply.github.com [animations] Bump minimum supported Dart version to 3.2 (flutter/packages#5598) 2023-12-06 43054281+camsim99@users.noreply.github.com [animations] Bump minimum Flutter version (flutter/packages#5596) 2023-12-06 6655696+guidezpl@users.noreply.github.com Migrate Material curves to new names (flutter/packages#4898) 2023-12-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[pointer_interceptor] Add ios implementation and move web implementation to federated structure" (flutter/packages#5591) 2023-12-06 louisehsu@google.com [pointer_interceptor] Add ios implementation and move web implementation to federated structure (flutter/packages#5500) 2023-12-06 stuartmorgan@google.com [pigeon] Use dart:io output inheritance for tooling (flutter/packages#5536) 2023-12-06 43759233+kenzieschmoll@users.noreply.github.com Fix benchmark reload bug and migrate away from deprecated `js_util` APIs (flutter/packages#5577) 2023-12-06 stuartmorgan@google.com [google_sign_in] Add macOS support (flutter/packages#4962) 2023-12-06 ian@hixie.ch [rfw,flutter_markdown] Apparently you need a comma to end an //ignore (flutter/packages#5582) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Adds macOS support to `google_sign_in_ios` by moving the existing source to `darwin/`, adjusting it somewhat to be macOS-compatible, and enabling `sharedDarwinSource` in the podspec. Unlike previous cases of code sharing, this doesn't rename the package to be something more generic, since when we do that the name is based on the underlying SDK, and the SDK is still mostly called "Google Sign-In for iOS", so I don't think there's a better name to give it at the moment. Most of flutter/flutter#46157 (Once this lands, there will be a follow-up PR to update the app-facing package to endorse it and list it in the README.)

stuartmorgan added 11 commits August 15, 2023 12:16

Initial code changes

d56cc66

Add macOS to example

2a48c65

Move to shared source, and add macOS metadata

a479991

Move unit tests, fix code

4637d58

Wire up native unit tests on macOS and get everything compiling and p…

1048d79

…assing

Merge branch 'main' into google-sign-in-macos

b1bbb96

Fix unit test compilation for stricter macOS settings

2a83db4

Update README

be2a329

Version bump

b0751d4

Remove symlinks

05978ee

Placeholder SDK update

cd4c3e6

stuartmorgan requested a review from cbracken September 21, 2023 14:50

stuartmorgan requested a review from vashworth as a code owner September 21, 2023 14:50

github-actions bot added p: google_sign_in platform-ios platform-macos labels Sep 21, 2023

stuartmorgan commented Sep 21, 2023

View reviewed changes

Update false_secrets path

0c30045

vashworth reviewed Sep 25, 2023

View reviewed changes

vashworth approved these changes Sep 25, 2023

View reviewed changes

stuartmorgan added the waiting for stable update Can't be landed until functionality reaches the stable channel label Oct 5, 2023

Add missing secrets path

b9011ad

cbracken approved these changes Oct 12, 2023

View reviewed changes

Merge branch 'main' into google-sign-in-macos

d7b1e2e

stuartmorgan added 4 commits December 4, 2023 11:28

Merge branch 'main' into google-sign-in-macos

289575d

Update false_secrets path

d81fbef

Add a pod repo update step to try to address stale pods

25bd9a6

Copypasta

97abbfc

stuartmorgan mentioned this pull request Dec 5, 2023

[video_player] Unfork publish: for macOS #5578

Merged

11 tasks

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 6, 2023

auto-submit bot merged commit afd4517 into flutter:main Dec 6, 2023

stuartmorgan deleted the google-sign-in-macos branch December 6, 2023 20:14

engine-flutter-autoroll mentioned this pull request Dec 7, 2023

Roll Packages from ca16173e67e4 to 15584a394d36 (13 revisions) flutter/flutter#139736

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 7, 2023

afd4517 [google_sign_in] Add macOS support (flutter/packages#4962)

79c09a2

reidbaker mentioned this pull request Jan 12, 2024

Dialog Debug #5881

Closed

reidbaker mentioned this pull request Jan 19, 2024

[in_app_purchase] Add play country code api #5941

Merged

12 tasks

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_sign_in] Add macOS support #4962

[google_sign_in] Add macOS support #4962

stuartmorgan commented Sep 21, 2023 •

edited

Loading

flutter-dashboard bot commented Sep 21, 2023

stuartmorgan commented Sep 21, 2023

stuartmorgan commented Sep 21, 2023

stuartmorgan Sep 21, 2023

stuartmorgan Sep 21, 2023

cbracken Oct 12, 2023

stuartmorgan Sep 21, 2023

stuartmorgan commented Sep 22, 2023

vashworth left a comment

stuartmorgan commented Sep 25, 2023

vashworth left a comment

cbracken left a comment

behrangnm commented Oct 16, 2023

stuartmorgan commented Oct 16, 2023

FelixMethe commented Oct 17, 2023

chenenyu commented Nov 20, 2023

stuartmorgan commented Nov 27, 2023

[google_sign_in] Add macOS support #4962

[google_sign_in] Add macOS support #4962

Conversation

stuartmorgan commented Sep 21, 2023 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Sep 21, 2023

stuartmorgan commented Sep 21, 2023

stuartmorgan commented Sep 21, 2023

stuartmorgan Sep 21, 2023

Choose a reason for hiding this comment

stuartmorgan Sep 21, 2023

Choose a reason for hiding this comment

cbracken Oct 12, 2023

Choose a reason for hiding this comment

stuartmorgan Sep 21, 2023

Choose a reason for hiding this comment

stuartmorgan commented Sep 22, 2023

vashworth left a comment

Choose a reason for hiding this comment

stuartmorgan commented Sep 25, 2023

vashworth left a comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

behrangnm commented Oct 16, 2023

stuartmorgan commented Oct 16, 2023

FelixMethe commented Oct 17, 2023

chenenyu commented Nov 20, 2023

stuartmorgan commented Nov 27, 2023

stuartmorgan commented Sep 21, 2023 •

edited

Loading