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

Code reorg to move all public headers under src/api #14371

Open
wants to merge 4 commits into
base: cheryllin/firestoreSwiftCpp
Choose a base branch
from

Conversation

wu-hui
Copy link
Contributor

@wu-hui wu-hui commented Jan 22, 2025

This PR re-organize the code structure to better support Firestore's Swift/Cpp interop.

For SPM we will have 3 targets:

  • FirebaseFirestoreCpp is the core Cpp SDK implementation.
  • FirebaseFirestoreObjCpp is the ObjectiveC API layer providing most of Firestore's APIs today. It depends on FirebaseFirestoreCpp.
  • FirebaseFirestore is the user facing target that depends on above two, and adds swift APIs on top of them. It currently provides Async/Await and codable support on top of ObjCpp, and in the future it will provide new APIs by utilizing FirebaseFirestoreCpp directly, skipping ObjCpp.

For cocoapods the structure stays unchanged, we will continue to two targets:

  • FirebaseFirestoreInternal is the combination of FirebaseFirestoreCpp and FirebaseFirestoreObjCpp above.
  • FirebaseFirestore is the user facing target on top of FirebaseFirestoreInternal.

This PR also eliminates interfaceForSwift folder for Cpp code exposed to Swift, and use existing src/api instead. There is also changes to allow using absolute path (from SDK repo root) as opposed to relative path.

cocoapods integration tests failures:

AggregationIntegrationTests.testThrowsAnErrorWhenGettingTheResultOfAnUnrequestedAggregation()
AsyncAwaitIntegrationTests.testAutoIndexCreationAfterFailsTermination()
PipelineTests.testCreatePipeline()

@wu-hui wu-hui changed the base branch from cheryl/pipeline to cheryllin/firestoreSwiftCpp January 22, 2025 17:30
@cherylEnkidu cherylEnkidu force-pushed the cheryllin/firestoreSwiftCpp branch from 504ee98 to fe4ec87 Compare January 27, 2025 20:10
@wu-hui wu-hui force-pushed the wuandy/SwiftCppFixes branch 2 times, most recently from d418e06 to 317ccfb Compare January 29, 2025 19:08
@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

Copy link
Contributor

github-actions bot commented Jan 29, 2025

Apple API Diff Report

Commit: 0cfe3e5
Last updated: Fri Jan 31 13:24 PST 2025
View workflow logs & download artifacts


[BUILD ERROR] FirebaseFirestore


[BUILD ERROR] FirebaseFirestoreInternal


@wu-hui wu-hui force-pushed the wuandy/SwiftCppFixes branch from 5b61b67 to 357e877 Compare January 31, 2025 15:29
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.

CI is showing multiple build failures. Are you stuck on solving those?

The general direction looks fine - although I need to think harder about the new SPM structure - and that might be fresher for @ncooke3.

It might be easier to get CocoaPods working first and then address SPM.

@@ -35,6 +35,7 @@ Google Cloud Firestore is a NoSQL document database built for automatic scaling,
s.pod_target_xcconfig = {
# Enables C++ <-> Swift interop (by default it's only C)
"SWIFT_OBJC_INTEROP_MODE" => "objcxx",
'HEADER_SEARCH_PATHS' => '${PODS_TARGET_SRCROOT} "${PODS_TARGET_SRCROOT}/Firestore/core/src/api"'
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed when interfaceForSwift did not need to be here before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure exactly why, I suspect it is because headers under interfaceForSwift were using relative paths before. After the move, they are using absolute paths from SRCROOT. But we need the second search path here because the umbrella header still include FirebaseFirestoreCpp.h without absolute path.

@wu-hui
Copy link
Contributor Author

wu-hui commented Feb 6, 2025

CI is showing multiple build failures. Are you stuck on solving those?

I am not stuck, but it will take some time to resolve all of them. I intend to merge to cheryl's branch first, just so she can start using the new structure, while i fix the rest of failures.

The general direction looks fine - although I need to think harder about the new SPM structure - and that might be fresher for @ncooke3.

It might be easier to get CocoaPods working first and then address SPM.

FWIW, cocoapods build is "working", in that it builds fine. There are 3 test failures i listed in the PR description. They might have something to do with the actual changes Cheryl is doing.

#import "Firestore/core/src/api/pipeline.h"
#import "Firestore/core/src/api/pipeline_result.h"
#import "Firestore/core/src/api/pipeline_source.h"
#import "Firestore/core/src/api/stage.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the main reasons for restructuring the iOS SDK is to enable Swift source files to access all C++ APIs. Are there any technical limitations or restrictions in moving the existing API headers, like document reference header, into this file?

.headerSearchPath("api"), // Ensure the header search path is correct
.headerSearchPath("../../"),
.headerSearchPath("Public/FirebaseFirestore/"),
.headerSearchPath("../Protos/nanopb"),
]
)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a todo here to remind me to change the binary building process?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants