Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Firestore] Fix build-for-testing linking affecting CocoaPods (Approach 1) #11977

Merged
merged 3 commits into from
Oct 22, 2023

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Oct 20, 2023

@google-oss-bot
Copy link

google-oss-bot commented Oct 21, 2023

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestoreInternal.framework

    Overall coverage changed from 88.15% (4b26b31) to 88.09% (28d765f) by -0.06%.

    FilenameBase (4b26b31)Merge (28d765f)Diff
    exception.cc84.21%23.68%-60.53%
    leveldb_key.cc98.14%98.82%+0.69%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/N8d4tUDG5i.html

@paulb777
Copy link
Member

Great find! Any reason for weak_framework vs framework?

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM, when Gemfile is reverted

@paulb777
Copy link
Member

We may need something like this more generally for #11842 (comment), but we can sort that out, post 10.17

@ncooke3
Copy link
Member Author

ncooke3 commented Oct 21, 2023

Great find! Any reason for weak_framework vs framework?

Thanks! So that was informed from the docs for framework. The docs mention it's for system frameworks. It might still work, but weak_framework did not have that clarification.

Some other approaches I tried:

  1. Programmatically linking the class. See the diff: master...nc/firestore-fix-2 This seemed to work, but we'd need to do it for every public Firestore symbol being re-exported, which seemed less than ideal. I also couldn't fully reason through why some of the bits in that diff were necessary based on my testing. This approach seemed to work though, I just thought this PR's approach is simpler.
  2. Adding a s.pod_target_xcconfig with the s.pod_target_xcconfig = { 'OTHER_LDFLAGS' => -framework "FirebaseFirestoreInternal" }. This approach did not work in my testing.
  3. Removing the -fno-autolink flag in the FirebaseFirestoreTestingSupport.podspec. This also did not work.

@ncooke3 ncooke3 marked this pull request as ready for review October 21, 2023 23:10
@ncooke3 ncooke3 enabled auto-merge (squash) October 21, 2023 23:15
@paulb777 paulb777 merged commit e94c2bd into master Oct 22, 2023
1 check passed
@paulb777 paulb777 deleted the nc/firestore-fix branch October 22, 2023 01:07
@ncooke3
Copy link
Member Author

ncooke3 commented Oct 22, 2023

Thanks for the merge, will re-tag and stage.

@firebase firebase locked and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants