-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Conversation
FirebaseFirestore.podspec
Outdated
@@ -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]', |
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.
These are all .c files, not .m, so this should be *.[hc]
.
@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. |
So we could conceivably exclude these for now. To do so, we'd also need to exclude 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:
|
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
FirebaseFirestore.podspec
Outdated
@@ -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" |
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.
Use single quotes to match the rest here
Sounds good to optimize for best development. I found the flag in firebase-ios-sdk/Firestore/Protos/CMakeLists.txt :) |
Add missing nanopb directory to podspec.
Otherwise imports of headers in Protos/nanopb fail to compile.