-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Firestore and Swift Package Manager #6072
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
Generated by 🚫 Danger |
ab7e4ba
to
b895e5d
Compare
b895e5d
to
6583ad5
Compare
11ef9a3
to
7d94469
Compare
329f721
to
d8bb29e
Compare
.product(name: "gRPC-cpp", package: "gRPC"), | ||
], | ||
path: "Firestore", | ||
exclude: [ |
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.
Why are all these entries necessary? Shouldn't this only consist of things that would otherwise match the sources
list?
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.
It's everything under Firestore that is not in sources
. A better directory structure for SPM would be putting all sources in one directory. I've sorted the list now.
Package.swift
Outdated
.headerSearchPath("../"), | ||
.headerSearchPath("Source/Public"), | ||
.headerSearchPath("Protos/nanopb"), | ||
.headerSearchPath("Protos/objc/google/api"), |
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.
This is obsolete: we no longer use Objective-C protocol buffers
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.
Removed here and in the podspec
@@ -84,14 +96,18 @@ let package = Package( | |||
url: "https://github.com/paulb777/nanopb.git", | |||
.revision("849a9b82233aaa5af4286c6c594cd831ca48651c") | |||
), | |||
.package( | |||
name: "abseil", | |||
url: "https://github.com/paulb777/abseil-cpp.git", |
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.
For the purposes of internal experimentation this seems reasonable, but I don't think we should publish this to end users until we've settled how we're going to actually make these dependencies work. The outcome I don't want is that we end up stuck maintaining these ourselves because we've conditioned enough users to use Firestore via SPM that we can't call the experiment a failure.
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.
Side note: I wouldn't be surprised to see SPM establish itself as a C++ dependecy manager, even if it currently lacks features vs CMake. Looking forward to see how Windows support in 5.3 will push that. I also would love to see Android NDK support Swift/SPM..
d8bb29e
to
8da13b2
Compare
I'll make a separate PR for the nanopb change after it tests here. |
8da13b2
to
abde9e7
Compare
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
This reverts commit bd0098a.
abde9e7
to
48e5c0f
Compare
Current status is a successful build and run of the Firestore quickstart.
Next steps are to consider merging to master to facilitate early evaluation and to continue conversations about dependency open questions
Dependency Summary:
General porting notes:
boringssl - branched from January 22 version
grpc - gRPC-Core 1.28.2 gRPC-C++ 1.28.2 - tagged May 20
absl - branched from master July 13
#pragma clang diagnostic ignored "-Wmodules-import-nested-redundant"
to workaround build errorleveldb - PR