-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Update travis to use CocoaPods 1.5.2 #1103
Conversation
ios and tvOS tests failing with 2018-04-14 15:03:20.671 xcodebuild[7023:18568] iOSSimulator: task_name_for_pid(mach_task_self(), 8340, &task) returned 5 |
@paulb777, is this in a reviewable state? Your comments here indicate this isn't working. |
@wilhuff - It needs more investigation before being reviewable. I'm unlikely to be able to continue on it in the next few weeks. Anyone else is welcome to pick it up. :) |
Adding CODE_SIGNING_ALLOWED=YES to the Firebase builds gets them to go, but it looks like there's another issue with the Firestore builds that it can't find FirebaseAuth. |
Seem to be getting close - but for some reason the reorganized build causes 21 Firestore unit test failures .... FSTLocalSerializerTests
testEncodesQueryData, (([self.serializer encodedQueryData:queryData]) equal to (expected)) failed: ("<FSTPBTarget 0x60c0000d9240>: {
FSTSerializerBetaTests
testEncodesArrays, ((actualProto) equal to (value)) failed: ("<GCFSValue 0x60e0000d92e0>: {
testEncodesArrayTransformMutations, ((actualProto) equal to (proto)) failed: ("<GCFSWrite 0x60c000212a40>: {
testEncodesFirstLevelAncestorQueries, ((actualProto) equal to (proto)) failed: ("<GCFSTarget 0x60c000138700>: {
testEncodesFirstLevelKeyQueries, ((actualProto) equal to (proto)) failed: ("<GCFSTarget 0x60c00015ed40>: {
testEncodesLimits, ((actualProto) equal to (proto)) failed: ("<GCFSTarget 0x60c0001d2480>: {
testEncodesMultipleFiltersOnDeeperCollections, ((actualProto) equal to (proto)) failed: ("<GCFSTarget 0x60c0000cfe80>: {
testEncodesNanFilter, ((actualProto) equal to (proto)) failed: ("<GCFSTarget 0x60c00000c040>: {
testEncodesNestedAncestorQueries, ((actualProto) equal to (proto)) failed: ("<GCFSTarget 0x60c00000c640>: {
|
Now ready for review. Read the updated description at top for a summary. |
# Test targets are below to avoid problems with duplicate symbols when | ||
# building as a static library (see comments below). | ||
end | ||
pod 'FirebaseAuth', :path => '../../' |
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.
Did you intend to re-nest these here or is this just a bad merge? How does this work around all the duplicate class warnings we were getting from linking the framework into the example app?
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.
I don't intend to re-nest them. They ONLY get linked into the example app. I added -all_load to the Example app build OTHER_LDFLAGS
so that unused C++ functions get linked in. Then the test target doesn't need them to build and finds them at runtime from the app.
@@ -1864,6 +1533,10 @@ | |||
); | |||
INFOPLIST_FILE = "Firestore/Firestore-Info.plist"; | |||
MODULE_NAME = ExampleApp; | |||
OTHER_LDFLAGS = ( |
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 is the relevant change not from pod deprecate
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.
pod deprecate
doesn't seem to be a thing. Do you mean pod deintegrate
?
I'm not sure we're seeing much pain from having the project stored integrated. It does mean that as developers we need to run the same version of cocoapods, but that's pretty easy for us to coordinate within the Firestore team.
Meanwhile as we're migrating to C++ we're constantly changing the project to add C++ tests and remove Objective-C tests. It will be a serious pain to have to pod deintegrate
to even be able to read the diffs of those changes.
I get it that pod deintegrate
d projects are cleaner, but practically speaking they're much more cumbersome. I think we should leave this installed.
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.
Yes I meant pod deintegrate
. And undone now.
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.
The interesting parts of this LGTM, however see below.
Meanwhile: I've checked this out and run all the tests (including integration tests) and can verify that this works.
Nice sleuthing, BTW. This turns out to be a better solution even for CocoaPods 1.5.0. With -all_load
that workaround wasn't needed even before.
@@ -1864,6 +1533,10 @@ | |||
); | |||
INFOPLIST_FILE = "Firestore/Firestore-Info.plist"; | |||
MODULE_NAME = ExampleApp; | |||
OTHER_LDFLAGS = ( |
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.
pod deprecate
doesn't seem to be a thing. Do you mean pod deintegrate
?
I'm not sure we're seeing much pain from having the project stored integrated. It does mean that as developers we need to run the same version of cocoapods, but that's pretty easy for us to coordinate within the Firestore team.
Meanwhile as we're migrating to C++ we're constantly changing the project to add C++ tests and remove Objective-C tests. It will be a serious pain to have to pod deintegrate
to even be able to read the diffs of those changes.
I get it that pod deintegrate
d projects are cleaner, but practically speaking they're much more cumbersome. I think we should leave this installed.
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.
LGTM
* Update travis to use CocoaPods 1.5.2 * CODE_SIGNING_ALLOWED=YES * Remove workaround and iPhone 8 * Remove firebase#1210 static library Podfile workaround * Add -all_load to Firestore Example so all C++ is available for tests
CODE_SIGNING_ALLOWED=YES
toxcodebuild
command to work around TravisCI fails to boot Simulator with CocoaPods 1.5.x CocoaPods/CocoaPods#7734-all_load
to Firestore Example project build so that all C++ are linked in and satisfy linker dependencies from tests