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

Prepare Firestore for use as a static library #1210

Merged
merged 6 commits into from
May 3, 2018
Merged

Conversation

wilhuff
Copy link
Contributor

@wilhuff wilhuff commented May 2, 2018

This change prepares for building/testing Firestore as a static library. It fixes the errors encountered putting together #1056, but does not yet add any direct support for building the static version of Firestore--that will come separately.

Concretely, this change:

  • Fixes the integration tests that were broken in Rename iOS-specific targets with iOS-specific names #1150: I left references to targets that longer exist
  • Removes the OCMock and leveldb dependencies from the integration tests; they're not needed
  • Flattens the Podfile, making test targets independent from the example app
  • Adds a workaround that prevents dependency duplication between the app and tests, resolving the duplicate class warnings that would otherwise be seen in this configuration.

Further discussion of the underlying issue is here CocoaPods/CocoaPods#7155

wilhuff added 4 commits May 2, 2018 07:47
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Thanks for working through this stuff! I reviewed to the best of my ability but mostly counting on build / tests / our users to validate. :-)

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.

Thanks for working through all of this @wilhuff!

This can be addressed separately:
Seeing one Firestore build warning in default build with Xcode 9.3 - /Users/paulbeusterien/pure/firebase-ios-sdk/Firestore/Source/Remote/FSTRemoteEvent.mm:47:17: warning: method definition for 'filterUpdatesUsingExistingKeys:' not found [-Wincomplete-implementation]
@implementation FSTTargetMapping
^
In file included from /Users/paulbeusterien/pure/firebase-ios-sdk/Firestore/Source/Remote/FSTRemoteEvent.mm:17:
/Users/paulbeusterien/pure/firebase-ios-sdk/Firestore/Example/Pods/../../../Firestore/Source/Remote/FSTRemoteEvent.h:52:1: note: method 'filterUpdatesUsingExistingKeys:' declared here

  • (void)filterUpdatesUsingExistingKeys:(FSTDocumentKeySet *)existingKeys;
    ^
    1 warning generated.

@@ -11,26 +11,54 @@ pod 'FirebaseFirestore', :path => '../../'

target 'Firestore_Example_iOS' do
platform :ios, '8.0'
end
Copy link
Member

Choose a reason for hiding this comment

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

A comment here to look below for construction of the dependent targets could be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

wilhuff added a commit that referenced this pull request May 3, 2018
method definition for 'filterUpdatesUsingExistingKeys:' not found

There are no abstract methods in Objective-C so the base class needs an
implementation. It can throw like the other abstract-like methods do or
just provide a default behavior. I've opted for the latter here.
wilhuff added a commit that referenced this pull request May 3, 2018
method definition for 'filterUpdatesUsingExistingKeys:' not found

There are no abstract methods in Objective-C so the base class needs an
implementation.
@wilhuff wilhuff merged commit a133948 into master May 3, 2018
@wilhuff wilhuff deleted the wilhuff/project-fix branch May 9, 2018 17:47
paulb777 added a commit that referenced this pull request May 14, 2018
* Update travis to use CocoaPods 1.5.2
* CODE_SIGNING_ALLOWED=YES
* Remove workaround and iPhone 8
* Remove #1210 static library Podfile workaround
* Add -all_load to Firestore Example so all C++ is available for tests
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
…1218)

method definition for 'filterUpdatesUsingExistingKeys:' not found

There are no abstract methods in Objective-C so the base class needs an
implementation.
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
* Remove spurious dependencies on other project targets in the integration tests

* Integration tests don't depend on OCMock

... and shouldn't

* Integration tests don't directly depend upon leveldb either

* Flatten Firestore example Podfile

Make the targets independent in Firestore/Example/Podfile and add a
workaround to deduplicate dependencies.

* Project file output from running pod update
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
* 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
@firebase firebase locked and limited conversation to collaborators Nov 3, 2019
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