google / GoogleSignIn-iOS

Enables iOS and macOS apps to sign in with Google.
https://developers.google.com/identity/sign-in/ios
Apache License 2.0
472 stars 189 forks source link

Upgrading to 7.1.0 from 7.0.0, now we can't `po` anything in lldb due to GTMSessionFetcher import issue. #396

Closed marcpalmer closed 3 months ago

marcpalmer commented 4 months ago

Describe the bug

We are seeing issues in lldb related to GTMSessionFetcher since updating to 7.1.0, where it complains it cannot import GTMSessionFetcher any time we try to po a value from our code in the debugger — which is obviously very bad news.

I'm far from an expert on this but in our case the problem "goes away" if we change the conditional import in GIDGoogleUser.h https://github.com/google/GoogleSignIn-iOS/blob/ec20caf39917e3a50905776db1713b37c6b33840/GoogleSignIn/Sources/Public/GoogleSignIn/GIDGoogleUser.h#L26 so that it never uses the @import modular import variant.

Why this is suddenly a problem - as this has been in the GoogleSignIn code for a while – is not clear. We relatively recently (months back) added one SPM package to our app. The rest are all Cocoapods, including GSI and its dependencies. Perhaps this has triggered the problem, but we certainly only noticed it recently when switching from 7.0.0 to 7.1.0.

The clear implication is that SWIFT_PACKAGE is now defined for our builds where it wasn't previously, but this is breaking lldb for some reason, perhaps because GSI is still built with Cocoapods, not SPM?

To Reproduce

Sorry, we don't know.

Expected behavior

We should not have any issues using the debugger, least of all complaints about failure of lldb to import GTMSessionFetcher.

Screenshots

Environment

Additional context

mdmathias commented 4 months ago

cc @thomasvl for any ideas on this.

mdmathias commented 4 months ago

I was able to reproduce this problem in a sample app (linked below). I created an app with a Podfile depending on GoogleSignIn. I also added an arbitrary Swift Package from Apple. I then did pod install to get GSI 7.1.0, and built and ran from the .xcworkspace.

All pos did not cause an lldb error. Only pos like po GIDSignIn.sharedInstance.currentUser caused an error. Note, if I did let user = GIDSignIn.sharedInstance.currentUser, and then po user, then there would be no lldb error. This might be a workaround for you...

One thing to note is that GSI 7.0.0 depended upon GTMAppAuth 2.0.0, where GSI 7.1.0 depends upon GTMAppAuth 4.1.1+. GTMAppAuth release 4.0.0 rewrote that library in Swift. In between GSI 7.0.0 and 7.1.0, this import in GIDGoogleUser.h:

// We have to import GTMAppAuth because forward declaring the protocol does
// not generate the `fetcherAuthorizer` property below for Swift.
#ifdef SWIFT_PACKAGE
@import GTMAppAuth;
#else
#import <GTMAppAuth/GTMAppAuthFetcherAuthorization.h>
#endif

changed to

#ifdef SWIFT_PACKAGE
@import GTMSessionFetcherCore;
#else
#import <GTMSessionFetcher/GTMSessionFetcher.h>
#endif

I made this change because GIDGoogleUser.h refers to GTMFetcherAuthorizationProtocol here, which is presumably why GTMAppAuth was previously imported in GIDGoogleUser.h (to grab GTMFetcherAuthorizationProtocol transitively via GTMAppAuth, which depends upon GTMSessionFetcher). GIDGoogleUser does not itself need anything from GTMAppAuth.

I created another test app that was pure Swift Package Manager (linked below). I put a breakpoint in a spot after I used let user = GIDSignIn.sharedInstance.currentUser, and did: po user and also po GIDSignIn.sharedInstance.currentUser. Neither produced an lldb error.

I think the issue has something to do with GTMAppAuth's change to Swift combined with CocoaPods...something. I'm not sure yet. This problem reminds me https://github.com/google/gtm-session-fetcher/issues/384, but looks like that was included in https://github.com/google/gtm-session-fetcher/releases/tag/v3.4.0, which I can confirm is being used in my tests below. Not sure...

I made a final test app that is pure CocoaPods (see link below) that depends upon GSI. I was able to po user and po GIDSignIn.sharedInstance.currentUser without a problem. So, I think this means that there is some issue with lldb and projects that are made from a combination of SPM and CocoaPods. It may or may not have anything to do with GTMAppAuth and GTMSessionFetcher.

I will do some more digging and ask around.

Test.zip (SPM + CocoaPods) Test (SPM).zip Test (CocoaPods Only).zip

mdmathias commented 4 months ago

Anecdotally, I think this issue demonstrates that mixing SPM and CocoaPods leads to some complexity. Is it possible for you to use one or the other?

marcpalmer commented 4 months ago

First, @mdmathias thank you so much for going to the effort to repro this. It is really appreciated.

For us lldb chokes on anything we try — although if memory serves that is probably Swift symbols only as I don't recall recently trying to po ObjC expressions.

I suspect your theory is correct re: mixing SPM and Cocoapods however I know that we did not have this problem until updating to 7.1.0 — which we did ~2 weeks ago and we've had the SPM dep in there for months. So I think it is more nuanced than this. On paper I don't think there is any reason that mixing these should cause problems - they both just build libraries and frameworks. I don't think switching over fully to SPM is viable for us due to some legacy dependencies but it is already on my to-do list to investigate this.

Perhaps there is an issue with GSI where it does not have the dependency on GTMSessionFetcher fully expressed, or at least it seems like lldb doesn't see it available as a dependency so maybe there lies a clue? Because again, even with that SWIFT_PACKAGE check it worked before when it was transitive via GTMAppAuth.

marcpalmer commented 4 months ago

Oh @mdmathias I will add... isn't it wrong that the SWIFT_PACKAGE branch of the #if is being used at all, because we're exclusively building GoogleSignIn and its deps using Cocoapods.

The fact that we do have SPM enabled in Xcode causes SWIFT_PACKAGE to be defined doesn't it? So this is falsely thinking that it has built modules and can use module imports for GTMSessionFetcherCore which it hasn't? So presumably when building with Cocoapods that is not defined or we wouldn't even get as far as a successful build — and so when lldb is running it evals the header (my poor understanding is that lldb will re-evaluate headers/re-imports) and SWIFT_PACKAGE is defined then so it fails?

EDIT: This still doesn't explain why it was OK previously with GTMAppAuth as the dep.

mdmathias commented 4 months ago

I agree that the issue sits somewhere in between GTMAppAuth's conversion to Swift in versions 4+ and the mixing of SPM and CocoaPods, with what I expect to be the majority of the onus on the mixing of SPM and CocoaPods.

I don't think we can point too strongly to GTMAppAuth's Swift conversion since that is more or less moot: the library has full rights to be Swift or Objective-C. So, IMO, the issue has more to do with mixing the package managers.

This leads me to your next point about how the .xcworkspace project is somehow hitting the positive case in the #ifdef SWIFT_PACKAGE check in GIDGoogleUser.h. I'm not exactly sure how or why that's happening, but you're theory sounds right. I guess that kind of highlights my point about combining CocoaPods and SPM being a bit brittle and yielding undefined behavior.

I will keep investigating this on my end.

marcpalmer commented 4 months ago

I think that the key thing is probably (?) that SWIFT_PACKAGE does not mean "built with SPM". It just means "SPM is supported by this toolchain". If correct, this makes it the wrong thing to switch on here... although I'm not sure there is a "right thing" available. I think what it's trying to do is determine if GTMAppAuth was built as a modular framework, which seems like quite a different thing.

fuchenxi commented 3 months ago

I have a similar issue in my project, which involves a mix of Objective-C and Swift.

When I use pod 'GoogleSignIn' to import your library and execute the po command in a Swift file, I receive the following error:

error: couldn't IRGen expression: Clang importer error
error: /Users/Product***/Pods/GoogleSignIn/GoogleSignIn/Sources/Public/GoogleSignIn/GIDGoogleUser.h:27:9: error: module 'GTMSessionFetcherCore' not found
@import GTMSessionFetcherCore;

If I add @import GoogleSignIn; or#import <GoogleSignIn/GoogleSignIn.h>to the bridging-header.h file, and then execute the po command again, I get the following error:

error: Expression evaluation failed. Retrying without binding generic parameters
error: Could not evaluate the expression without binding generic types.

This problem has been bothering me for a long time and I hope to get help。

Currently, I've switched to integrating your library using SPM, and I don't encounter this issue anymore.

mdmathias commented 3 months ago

@fuchenxi I'm not sure how your project is set up, but our Swift sample project using CocoaPods does not have this issue.

fuchenxi commented 3 months ago

If I provide my podfile file, can you analyze the problem?

Kharauzov commented 3 months ago

I can confirm the same issue, mentioned by @marcpalmer . I tried this hot fix:

I'm far from an expert on this but in our case the problem "goes away" if we change the conditional import in GIDGoogleUser.h

And it works for me.

But I wish there can be some fulfil solution to this bug.

mdmathias commented 3 months ago

@marcpalmer I've created #403 to address this. There is a sample project linked in a comment on that PR, but I would appreciate you pointing your dependency to https://github.com/google/GoogleSignIn-iOS/tree/mdmathias/fix-lldb-crash and testing in your project as well. Would you mind doing that? Let me know if this change fixes the issue for you. Thanks!

mdmathias commented 3 months ago

@marcpalmer gentle ping to test the branch in #403 if you don't mind. :)

heidiproske-qz commented 3 months ago

@mdmathias I know I'm not Marc but I was encountering the same (or very similar issue). We mostly use cocoapods but also started integrating some Swift packages too and our po has been broken for a while so I can easily reproduce it and just saw it again this afternoon:

Screenshot 2024-05-05 at 7 38 37 PM

Once I updated our Podfile to use your branch (instead of the previous 7.1.0 version):

-  - GoogleSignIn (= 7.1.0)
+  - GoogleSignIn (from `https://github.com/google/GoogleSignIn-iOS.git`, branch `mdmathias/fix-lldb-crash`)

it no longer errors on me when I run a po command :) 👍

mdmathias commented 3 months ago

Thanks @heidiproske-qz!

cederache commented 2 months ago

Hi, Any chance to have a new release with this fix ?

mdmathias commented 2 months ago

@cederache The draft release for v7.2.0 contains https://github.com/google/GoogleSignIn-iOS/pull/403, which fixes this issue.

I am hoping to get v7.2.0 released in the next few weeks (before the end of June).

marcpalmer commented 2 months ago

Sorry I missed the past pings. I'll try to check this today!

yarodevuci commented 2 months ago

@heidiproske-qz link to git branch you posted is deleted.. do we just use main branch instead?

CWftw commented 1 month ago

Hi, we've also been experiencing this issue. I see 8.0.0 on https://developers.google.com/identity/sign-in/ios/release but nothing on Cocoapods past 7.1.0. What's the status of 7.2.0/8.0.0?

mdmathias commented 1 month ago

v8.0.0 should be released the week of July 15. Apologies for that delay. In the meantime, you can depend upon the main branch, which should take care of this issue.

yarodevuci commented 1 month ago

v8.0.0 should be released the week of July 15. Apologies for that delay. In the meantime, you can depend upon the main branch, which should take care of this issue.

@mdmathias main branch has same issue...

mdmathias commented 1 month ago

@yarodevuci are you sure? #403 fixes the issue and I cannot reproduce. Also, anecdotally, folks above with this issue confirmed that #403 fixes the problem.

If you still see this problem, could you please share reproduction steps?

yarodevuci commented 1 month ago

@yarodevuci are you sure? #403 fixes the issue and I cannot reproduce. Also, anecdotally, folks above with this issue confirmed that #403 fixes the problem.

If you still see this problem, could you please share reproduction steps?

Maybe I pulled the wrong branch ? I used main I used pods to pull latest from main branch

DeezeriOSTeam commented 1 month ago

Hello @mdmathias, any news of publication of 7.2.0 or 8.0.0 version ? It was expected this week but still nothing.. It's been a while since the bug have been fixed and aside from reverting back to 7.0.0 there isn't a clean solution proposed for us to get it (we are currently adding Google Sign-In via Firebase.zip asset)

mdmathias commented 1 month ago

@DeezeriOSTeam https://github.com/google/GoogleSignIn-iOS/issues/418