Skip to content

Per framework Xcode projects/dev environments, Podfiles, updated/pushable Podspecs #22

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

Closed
wants to merge 18 commits into from

Conversation

mhuusko5
Copy link
Contributor

@mhuusko5 mhuusko5 commented May 21, 2017

Issue (was late about it, sorry): #21

EDIT: More to do with #23 / #9. macOS component superseded by #38.

The main goal of this effort was to reintroduce macOS support. As part of that process though, it became clear that the mono podspec/project (or really, just tests project, no dev project at all) approach was holding things back, so I first introduced individual xcode projects/builds for each framework, separate podfiles to pull in their dependencies, and updated each's imports and podspecs along the way to make the various build methods work happily together.

The end result is..
Dev environments for each framework
Potential step towards Carthage support (Carthage needs an .xcodeproj to build)
All the frameworks can be built separately locally, separately by hosting the new podspecs (or locally/hosted with the mono FirebaseDev podspec as they could before), on iOS or macOS (except for Messaging), library or framework, Obj-C or Swift

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@mhuusko5
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@paulb777
Copy link
Member

Wow! Thanks for the substantial contribution!

We'll get a review going in the next few days.

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.

What's the best way to try out the MacOS support? Have you tried to integrate the unit tests? What kind of testing could be add to the Travis CI?

More comments/questions inline.

Thanks for the PR!


#import "FIRLogger.h"
#import FirebaseCore(FIRLogger.h)
Copy link
Member

Choose a reason for hiding this comment

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

Why the FirebaseCore macro? I can see that it makes import sources clearer. Any references about this style?

s.framework = 'Security'
# s.dependency 'FirebaseDev/Core'
s.dependency 'FirebaseCore', '~> 4.0.1'
Copy link
Member

Choose a reason for hiding this comment

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

This won't be publishable as long as the release version of FirebaseCore has closed source dependencies -
https://github.com/CocoaPods/Specs/blob/master/Specs/8/b/d/FirebaseCore/4.0.0/FirebaseCore.podspec.json


s.source_files = '**/*.[mh]'
s.public_header_files =
Copy link
Member

@paulb777 paulb777 May 21, 2017

Choose a reason for hiding this comment

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

Removing the public headers from the podspecs is a problem. I'm seeing all the headers published in the framework instead of only the public ones, when I build with https://github.com/mhuusko5/firebase-ios-sdk/blob/master/BuildFrameworks/build.swift

@@ -20,7 +20,7 @@
#define STR(x) STR_EXPAND(x)
#define STR_EXPAND(x) #x

const double FirebaseAuthVersionNumber = FIRAuth_MINOR_VERSION;
const double FirebaseAuthFrameworkVersionNumber = FIRAuth_MINOR_VERSION;
Copy link
Member

Choose a reason for hiding this comment

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

Why this rename?

shared_pods
end

post_install do |installer|
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of the post_install?

@@ -0,0 +1,23 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to share the new _Internal.h/_Private files, since they're the same? At least they should be consistently named. The others are all _Private.h.

@@ -48,7 +48,7 @@ @protocol FIRFirebaseAuthLike <NSObject>
* This is a hack that copies the definitions of Firebear error codes. If the error codes change in the original code, this
* will break at runtime due to undefined behavior!
*/
typedef NS_ENUM(NSUInteger, FIRErrorCode) {
typedef NS_ENUM(NSInteger, FIRErrorCode) {
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

@mhuusko5 mhuusko5 closed this May 21, 2017
@mhuusko5 mhuusko5 reopened this May 21, 2017
@mhuusko5
Copy link
Contributor Author

@paulb777 thanks for the speedy feedback! Timezone dictates that I won't get to the details until tomorrow, but I will preface them now by saying all the changes in this PR were from starting with these new isolated builds, targeting iOS/macOS, then tinkering the warnings/errors away. So some of the changes/hacks are weird, but so is the result/goal of FirebaseDev being buildable as a single pod/pile of source while also the having the individual frameworks being buildable as separate pods (with imports of internal headers from Core). Maybe there's another way, maybe it's not even a desired goal (I am interested to hear the overall planned deployment strategy); if so, it was an interesting exercise (/way to familiarize)!

I'll add an example project tomorrow that can serve to demonstrate macOS, demonstrate the interop of the updated podspecs, and serve as some context for discussion of issues I was seeing that lead to the various changes.

As for the unit tests/example mono project and macOS, I'd need to dig a bit to find out what that would take. To come!

@paulb777
Copy link
Member

@mhuusko5 Here's some background on the pod structure:

First of all, it's very much still a work in progress and one of the reasons we haven't yet open sourced the build and release process, which is something like the build.swift script. We'd like to solve two problems for which we haven't found ideal solutions with CocoaPods:

  1. The open source pods have product dependencies on closed source libraries/pods that we're not currently able to open source.

  2. We'd like to continue to build the pods as static library frameworks to avoid the cold start impact of dynamic frameworks. More details at Add support for source static library framework CocoaPods CocoaPods/CocoaPods#6651

The reason for the FirebaseDev monopod is so that when a Podfile specifies !use_frameworks, the Firebase pods get consolidated via the chosen subspecs into a single dynamic library framework instead of one dynamic framework for each component with the associated several hundred millisecond impact on cold start time.

@mhuusko5
Copy link
Contributor Author

mhuusko5 commented May 23, 2017

@paulb777 I'm actually probably not going to get back to this PR until a bit later in the week unfortunately. In the meantime, this deployment strategy topic is interesting/a bit of conundrum, and I'd like to make sure I've got the full picture:

On 1) just to confirm, there's not anything inherently problematic CocoaPods-wise about having Core/Database/Auth/etc. open source, while Analytics remains closed source, right? The open source ones could be deployed as source based pods (or mono pod with subspecs), or vendored frameworks based pods, and have a dependency on the definitely vendored framework based Analytics pod, as they currently do. In fact, if I understand correctly, they wouldn't even necessarily need a dependency on the Analytics pod (user's could just pull it in alongside if they desired), because it's a weak/optional dependency, but it's optimal for complete product exposure for it to be included by default..? That's a long way of saying I assume the source being open doesn't have to change the deployment strategy (assuming your previous build/deployment strategy didn't have intricacies/assumptions between the source that is now open, and that which is still closed)?

  1. Static vs. dynamic (or number of dynamics) seems to be the hot ticket. It seems though that there may not be many (good) options..
    1. Analytics static. The other frameworks built statically too, distributed as vendored framework pods separately. I imagine this was/is the 3.x setup? Not a bad option, just isn't great for those wrapping Firebase.
    2. Analytics static. Others mono source/dynamic framework with a dependency on it. If user's want to use the static Analytics, they have to get creative (Reject installation if a static library is used as a transitive dependency while using frameworks CocoaPods/CocoaPods#2926). Not a great option.
    3. Analytics dynamic. Others mono source/dynamic or separate source/dynamics. Simple/flexible, but with launch cost – that said, is it really 600ms per framework or does it require specific conditions (I work on an integration heavy use_frameworks! app with 33 direct/transitive pods, and our launch time is ~3 seconds; not great, but not 20 seconds either)?

@paulb777
Copy link
Member

@mhuusko5 Thanks for the update on your timing and let us know if you'd like more clarity on anything else.

  1. The pods could be deployed as source only if (A) we removed the dependency on Analytics (since CocoaPods does not support source pods depending on static library pods) or (B) we built a dynamic library version of Analytics or (C) we removed the dependency.

We deploy them as vendored framework pods now. The downside is the disconnect between the source and the product build.

Right, the source being open doesn't have to change the deployment strategy. However, we'd like to make sure that we're fully understanding, mitigating any impact, and potentially reconsidering any deployment strategy constraints that are hindering open source quality.

2i. Yes, all static vendored_frameworks is both the 3.x and the 4.x product distribution scheme.

ii. Right, CocoaPods does not allow source libraries to depend on static pods. We would need to determine if "def installer.verify_no_static_framework_transitive_dependencies" or something else is a solution.

iii. Our measurements might actually be comparable. We were seeing 500-700 msec on an iPhone 6. Smaller libraries were faster than bigger ones, but still more than a half second. Newer devices are substantially faster. However, Firebase won't be viable for many potential apps if it doesn't support old phones reasonably well.

paulb777 added a commit that referenced this pull request May 25, 2017
* Initialize core-mirror with 2017-04-05 version

* core piper update 2017-04-26 - CL: 154301610
@mhuusko5 mhuusko5 mentioned this pull request May 27, 2017
@mhuusko5 mhuusko5 changed the title Per framework Xcode projects/dev environments, Podfiles, updated/pushable Podspecs, macOS support Per framework Xcode projects/dev environments, Podfiles, updated/pushable Podspecs May 27, 2017
@mhuusko5
Copy link
Contributor Author

mhuusko5 commented May 27, 2017

@paulb777 The distribution topic much of the changes in this PR touch is larger than I initially understood and I don't want it to hold up macOS support, so I have started #38 to cover the macOS compatibility changes in isolation, as well as getting the example/test suite and static build targeting macOS.

I'm going to close this PR for now, and maybe come back to the branch to salvage some of the organisational changes, depending on what direction the distribution ends up going.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants