Rework gRPC certificate loading one last time #2606
Conversation
| @@ -84,10 +84,15 @@ | |||
| "idiom" : "ipad", | |||
| "size" : "83.5x83.5", | |||
| "scale" : "2x" | |||
| }, | |||
| { | |||
| "idiom" : "ios-marketing", | |||
There was a problem hiding this comment.
Nit: are changes to this file intentional?
There was a problem hiding this comment.
Yes. They happen any time you manipulate the macOS project. I can put this in a separate PR if you prefer.
| @@ -35,7 +35,6 @@ - (void)applicationDidFinishLaunching:(NSNotification *)aNotification { | |||
|
|
|||
| // do the timestamp fix | |||
| FIRFirestoreSettings *settings = db.settings; | |||
| settings.timestampsInSnapshotsEnabled = true; | |||
| db.settings = settings; | |||
There was a problem hiding this comment.
This change seems unrelated?
There was a problem hiding this comment.
This shows up as a deprecation warning while building the macOS_example, which was the primary means by which I verified this change was correct.
I can move this into a separate cleanup grab-bag PR if you prefer.
There was a problem hiding this comment.
No, I think that would be overkill (applies to other changes of that kind in this PR). As long as this is done on purpose, I'm fine with it being the part of the same PR.
| @@ -561,7 +561,7 @@ | |||
| B9C261C26C5D311E1E3C0CB9 /* query_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = query_test.cc; sourceTree = "<group>"; }; | |||
| BB92EB03E3F92485023F64ED /* Pods_Firestore_Example_iOS_Firestore_SwiftTests_iOS.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Example_iOS_Firestore_SwiftTests_iOS.framework; sourceTree = BUILT_PRODUCTS_DIR; }; | |||
There was a problem hiding this comment.
Nit: are changes to this file intended as part of this PR?
There was a problem hiding this comment.
As above, these are changes that are happening whenever you touch the project.
| #include <string> | ||
|
|
||
| #import "Firestore/Source/Core/FSTFirestoreClient.h" |
There was a problem hiding this comment.
Optional: I'm not sure this import is necessary, maybe try just deleting it?
There was a problem hiding this comment.
Done. I think we had this before in order to find a class to load the FirebaseFirestore framework's bundle, but doing this reflectively seems cleaner from a layering perspective.
| /** | ||
| * Finds the path to the roots.pem certificates file, wherever it may be. | ||
| * | ||
| * |
There was a problem hiding this comment.
Ultranit: extra empty line.
|
Several of the things you pointed out were unrelated to the core change here though I've included them for convenience. If you prefer I can split these into a separate PR. |
|
I'm okay with these changes being in this PR, was just making sure they were intentional. |
The prior implementation happened to work but only because it made implicit assumptions about the structure of the bundles we load.
In particular, by looking for
@"gRPCCertificates.bundle/roots"the code was assuming the iOS bundle layout for the app was in effect, but this doesn't work on macOS, in which app/framework bundles have an additionalContents/Resourcesin the path.This change gets rid of the implicit mapping of the bundle structure onto the directory structure and instead opens the nested
gRPCCertificates.bundleitself as a resource, making this code portable without having to understand the bundle layout under the covers.Fixes #2604. Should also remove the need for the workaround in #2177.