-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
…clashing, platform support, issues for builds
…essor, to fix build.swift
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! |
CLAs look good, thanks! |
Wow! Thanks for the substantial contribution! We'll get a review going in the next few days. |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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| |
There was a problem hiding this comment.
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 @@ | |||
/* |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
@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! |
@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:
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. |
@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)?
|
@mhuusko5 Thanks for the update on your timing and let us know if you'd like more clarity on anything else.
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. |
* Initialize core-mirror with 2017-04-05 version * core piper update 2017-04-26 - CL: 154301610
@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. |
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