Skip to content

Fix Firestore podspec overlay build #866

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 4 commits into from
Feb 28, 2018

Conversation

paulb777
Copy link
Member

Add missing nanopb directory to podspec.
Otherwise imports of headers in Protos/nanopb fail to compile.

@@ -31,6 +31,7 @@ Google Cloud Firestore is a NoSQL document database built for automatic scaling,
s.source_files = [
'Firestore/Source/**/*',
'Firestore/Port/**/*',
'Firestore/Protos/nanopb/**/*.[hm]',
Copy link
Contributor

Choose a reason for hiding this comment

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

These are all .c files, not .m, so this should be *.[hc].

@paulb777
Copy link
Member Author

@wilhuff I had to add a C Flag to get the C files to build. Should these files really be part of the library? They don't seem necessary for the unit tests.

Please reconfirm your LGTM.

@wilhuff
Copy link
Contributor

wilhuff commented Feb 28, 2018

So we could conceivably exclude these for now. To do so, we'd also need to exclude Firestore/core/src/firebase/firestore/remote/serializer.{h,cc}.

Background: we're in the midst of rewriting our serializer from using Objective-C protobuf to using nanopb. This work is incomplete though and the production code does not use this yet. There's no harm in including this code, and excluding it will make development slightly harder. However, development is currently proceeding mostly under the CMake build so we could exclude this without harming velocity.

Regarding this specific flag, it matches what we've done in the CMake build:

Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -67,7 +68,7 @@ Google Cloud Firestore is a NoSQL document database built for automatic scaling,
'"${PODS_TARGET_SRCROOT}/Firestore/third_party/abseil-cpp" ' +
'"${PODS_ROOT}/nanopb" ' +
'"${PODS_TARGET_SRCROOT}/Firestore/Protos/nanopb"',
'OTHER_CFLAGS' => '-DFIRFirestore_VERSION=' + s.version.to_s
'OTHER_CFLAGS' => '-DFIRFirestore_VERSION=' + s.version.to_s + " -DPB_FIELD_16BIT"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use single quotes to match the rest here

@paulb777
Copy link
Member Author

Sounds good to optimize for best development. I found the flag in firebase-ios-sdk/Firestore/Protos/CMakeLists.txt :)

@paulb777 paulb777 merged commit 8af210f into release-4.10.0 Feb 28, 2018
@paulb777 paulb777 deleted the pb-fix-firestore-overlay-build branch February 28, 2018 18:10
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
@firebase firebase locked and limited conversation to collaborators Nov 6, 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.

3 participants