Skip to content

Conversation

paulb777
Copy link
Member

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

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

'"${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