Skip to content

Stop requiring pods to be static frameworks #6557

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

Merged
merged 10 commits into from
Sep 28, 2020
Merged

Conversation

paulb777
Copy link
Member

@paulb777 paulb777 commented Sep 24, 2020

Fix #2022

Remove static_framework specifier from all podspecs. Use the Podfile option use_frameworks! :linkage => :static to get 6.x Firebase linkage behavior.

  • Lets developers control whether Firebase gets statically or dynamically linked.
  • CocoaPods now has finer grain controls for static and dynamic linkage.
  • Better supports some app modularization schemes.
  • Firebase no longer has any dependencies on closed source static frameworks
  • Firebase closed source pods are still static_frameworks

Additionally, this PR makes several changes to support the different bundle configuration of dynamic frameworks and associated tests:

  • The only library change is an update to FIAM to find its Display resources in either location
  • Additionally, several test changes were made including centralizing a mockFIROption class to support FirebaseApp.configure() for testing without a GoogleService-Info.plist.

I was also sad to delete the excellent doc's on static and dynamic frameworks from @maksymmalyhin, but I'm not sure they're still relevant.

I plan to do a follow-up PR to leverage CocoaPods 1.10 to add additional CI for pod lib lint testing with static frameworks.

@google-oss-bot google-oss-bot added the api: inappmessaging Firebase In App Messaging label Sep 25, 2020
@paulb777 paulb777 marked this pull request as ready for review September 25, 2020 20:44
Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

Great, mostly LGTM except a couple questions.

NSBundle *containingBundle;
NSURL *bundleURL;
// The containing bundle is different whether FIAM is statically or dynamically linked.
for (containingBundle in @[ [NSBundle mainBundle], [NSBundle bundleForClass:myClass] ]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: wonder if we can reuse existing logic from Core?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't seem worth it. Only the one line internal relevantBundles function would match as is.

@@ -1,76 +0,0 @@
# Using Firebase from a framework or a library
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer adding a note explaining why it is not relevant for Firebase 7 instead of deleting. Also, it seems still to be a relevant concern for zip, Carthage and binary distributions where we keep using static binaries.

@maksymmalyhin
Copy link
Contributor

It's probably outside of the scope of this PR, but I think it may be useful to try to measure the difference of e.g. pre-main startup time between static and dynamic frameworks integration and add numbers to a README for developers to make more conscious decision on which to use in their project.

Copy link
Member Author

@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.

Restored the docs with a Firebase 7 add. Thanks!

NSBundle *containingBundle;
NSURL *bundleURL;
// The containing bundle is different whether FIAM is statically or dynamically linked.
for (containingBundle in @[ [NSBundle mainBundle], [NSBundle bundleForClass:myClass] ]) {
Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't seem worth it. Only the one line internal relevantBundles function would match as is.

@paulb777
Copy link
Member Author

@maksymmalyhin @morganchen12 Good idea to do some measurements to help developers with the dynamic/static trade offs. Based on the measurements we did a few years ago, there are definite start up costs to using dynamic linking, and we should reevaluate and quantify.

@paulb777 paulb777 merged commit 3ca4e99 into firebase7-main Sep 28, 2020
@paulb777 paulb777 deleted the pb-no-static branch September 28, 2020 21:08
@firebase firebase locked and limited conversation to collaborators Oct 29, 2020
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.

4 participants