Stop using built-in CMake Framework support (#5316)

wilhuff · web-flow · commit 77f92cd2c677 · 2020-04-14T08:40:33.000-07:00
* Fix previously unknown warnings

These become visible once flags are handled uniformly across all
Firestore targets in an upcoming build change.

* Stop using built-in CMake Framework support

CMake has built-in support for building Framework bundles (briefly:
bundles of libraries, associated headers, and resources) but this
doesn't work well for our needs.

1. In CMake 3.5.1 (our earliest supported CMake version) it doesn't copy
headers properly at build time.

2. Even up to CMake 3.15.5, CMake doesn't handle link-line multiplicity
correctly. CMake passes the static library location within the framework
and this seems to cause the linker to mishandle Objective-C metaclass
information resulting in duplicate symbols. Building with `-framework`
flags works correctly.

Previously we partially worked around the latter problem by building
some frameworks as shared instead of static frameworks, but this is
problematic because this mode of operation isn't actually intended and
is unsupported as far as the rest of Firebase is concerned. This turned
out to be fragile though and while refactoring libraries this workaround
broke.

Rather than piling on additional workarounds, this change converts the
frameworks to simple library targets and emulates framework imports
through include paths.

Additionally, this starts refactoring our cmake helper functions to make
them more like built-in functions. This makes it possible to freely use
different CMake features that were made difficult by trying to wrap
everything up in a single CMake command.

* style.sh generated changes

* Review feedback

* More review feedback
diff --git a/FirebaseCore/CMakeLists.txt b/FirebaseCore/CMakeLists.txt
@@ -16,28 +16,29 @@ if(NOT APPLE)
   return()
 endif()
 
-file(GLOB sources Sources/*.m)
 file(GLOB headers Sources/Private/*.h Sources/Public/*.h)
+file(GLOB sources Sources/*.m)
 
 podspec_version(version ${PROJECT_SOURCE_DIR}/FirebaseCore.podspec)
-firebase_version(firebase_ios_version ${PROJECT_SOURCE_DIR}/FirebaseCore.podspec)
-
-firebase_ios_objc_framework(
-  FirebaseCore
-  SOURCES ${sources}
-  HEADERS ${headers}
-  VERSION ${version}
-  DEFINES
-    FIRCore_VERSION=${version}
-    Firebase_VERSION=${firebase_ios_version}
-  INCLUDES
-    ${PROJECT_SOURCE_DIR}
-  DEPENDS
-    FirebaseCoreDiagnosticsInterop
-    GoogleUtilities
-    "-framework Foundation"
-  DISABLE_STRICT_WARNINGS
-  EXCLUDE_FROM_ALL
+firebase_version(firebase_version ${PROJECT_SOURCE_DIR}/FirebaseCore.podspec)
+
+firebase_ios_add_framework(
+  FirebaseCore DISABLE_STRICT_WARNINGS EXCLUDE_FROM_ALL ${headers} ${sources}
+)
+
+firebase_ios_framework_public_headers(FirebaseCore ${headers})
+
+target_compile_definitions(
+  FirebaseCore PRIVATE
+  FIRCore_VERSION=${version}
+  Firebase_VERSION=${firebase_version}
+)
+
+target_link_libraries(
+  FirebaseCore PRIVATE
+  "-framework Foundation"
+  FirebaseCoreDiagnosticsInterop
+  GoogleUtilities
 )
 
 if(IOS)
diff --git a/Firestore/Example/Benchmarks/remote_document_cache_benchmark.mm b/Firestore/Example/Benchmarks/remote_document_cache_benchmark.mm
@@ -89,7 +89,7 @@
 
 void WaitForPendingWrites(FIRFirestore* db) {
   dispatch_semaphore_t done = dispatch_semaphore_create(0);
-  [db waitForPendingWritesWithCompletion:^(NSError* error) {
+  [db waitForPendingWritesWithCompletion:^(NSError*) {
     dispatch_semaphore_signal(done);
   }];
   dispatch_semaphore_wait(done, DISPATCH_TIME_FOREVER);
@@ -107,7 +107,7 @@ void WriteDocs(FIRCollectionReference* collection, int64_t count, bool match) {
 
 void Shutdown(FIRFirestore* db) {
   dispatch_semaphore_t done = dispatch_semaphore_create(0);
-  [db terminateWithCompletion:^(NSError* error) {
+  [db terminateWithCompletion:^(NSError*) {
     dispatch_semaphore_signal(done);
   }];
   dispatch_semaphore_wait(done, DISPATCH_TIME_FOREVER);
diff --git a/Firestore/Example/Tests/Integration/API/FIRListenerRegistrationTests.mm b/Firestore/Example/Tests/Integration/API/FIRListenerRegistrationTests.mm
@@ -31,14 +31,14 @@ - (void)testCanBeRemoved {
   FIRDocumentReference *docRef = [collectionRef documentWithAutoID];
 
   __block int callbacks = 0;
-  id<FIRListenerRegistration> one = [collectionRef
-      addSnapshotListener:^(FIRQuerySnapshot *_Nullable snapshot, NSError *_Nullable error) {
+  id<FIRListenerRegistration> one =
+      [collectionRef addSnapshotListener:^(FIRQuerySnapshot *, NSError *_Nullable error) {
         XCTAssertNil(error);
         callbacks++;
       }];
 
-  id<FIRListenerRegistration> two = [collectionRef
-      addSnapshotListener:^(FIRQuerySnapshot *_Nullable snapshot, NSError *_Nullable error) {
+  id<FIRListenerRegistration> two =
+      [collectionRef addSnapshotListener:^(FIRQuerySnapshot *, NSError *_Nullable error) {
         XCTAssertNil(error);
         callbacks++;
       }];
@@ -68,12 +68,11 @@ - (void)testCanBeRemovedTwice {
   FIRCollectionReference *collectionRef = [self collectionRef];
   FIRDocumentReference *docRef = [collectionRef documentWithAutoID];
 
-  id<FIRListenerRegistration> one = [collectionRef
-      addSnapshotListener:^(FIRQuerySnapshot *_Nullable snapshot, NSError *_Nullable error){
-      }];
-  id<FIRListenerRegistration> two = [docRef
-      addSnapshotListener:^(FIRDocumentSnapshot *_Nullable snapshot, NSError *_Nullable error){
+  id<FIRListenerRegistration> one =
+      [collectionRef addSnapshotListener:^(FIRQuerySnapshot *, NSError *){
       }];
+  id<FIRListenerRegistration> two = [docRef addSnapshotListener:^(FIRDocumentSnapshot *, NSError *){
+  }];
 
   [one remove];
   [one remove];
@@ -88,14 +87,14 @@ - (void)testCanBeRemovedIndependently {
 
   __block int callbacksOne = 0;
   __block int callbacksTwo = 0;
-  id<FIRListenerRegistration> one = [collectionRef
-      addSnapshotListener:^(FIRQuerySnapshot *_Nullable snapshot, NSError *_Nullable error) {
+  id<FIRListenerRegistration> one =
+      [collectionRef addSnapshotListener:^(FIRQuerySnapshot *, NSError *_Nullable error) {
         XCTAssertNil(error);
         callbacksOne++;
       }];
 
-  id<FIRListenerRegistration> two = [collectionRef
-      addSnapshotListener:^(FIRQuerySnapshot *_Nullable snapshot, NSError *_Nullable error) {
+  id<FIRListenerRegistration> two =
+      [collectionRef addSnapshotListener:^(FIRQuerySnapshot *, NSError *_Nullable error) {
         XCTAssertNil(error);
         callbacksTwo++;
       }];
@@ -137,7 +136,7 @@ - (void)testCanOutliveDocumentReference {
   @autoreleasepool {
     FIRDocumentReference *docRef = [collectionRef documentWithAutoID];
     documentID = docRef.documentID;
-    registration = [docRef addSnapshotListener:^(FIRDocumentSnapshot *snapshot, NSError *error) {
+    registration = [docRef addSnapshotListener:^(FIRDocumentSnapshot *snapshot, NSError *) {
       if (snapshot.exists) {
         [seen fulfill];
       }
diff --git a/Firestore/Example/Tests/Integration/API/FIRServerTimestampTests.mm b/Firestore/Example/Tests/Integration/API/FIRServerTimestampTests.mm
@@ -137,11 +137,11 @@ - (void)verifySnapshotWithResolvedTimestamps:(FIRDocumentSnapshot *)snapshot {
 - (void)runTransactionBlock:(void (^)(FIRTransaction *transaction))transactionBlock {
   XCTestExpectation *expectation = [self expectationWithDescription:@"transaction complete"];
   [_docRef.firestore
-      runTransactionWithBlock:^id(FIRTransaction *transaction, NSError **pError) {
+      runTransactionWithBlock:^id(FIRTransaction *transaction, NSError **) {
         transactionBlock(transaction);
         return nil;
       }
-      completion:^(id result, NSError *error) {
+      completion:^(id, NSError *error) {
         XCTAssertNil(error);
         [expectation fulfill];
       }];
@@ -298,11 +298,11 @@ - (void)testServerTimestampsFailViaUpdateOnNonexistentDocument {
 - (void)testServerTimestampsFailViaTransactionUpdateOnNonexistentDocument {
   XCTestExpectation *expectation = [self expectationWithDescription:@"transaction complete"];
   [_docRef.firestore
-      runTransactionWithBlock:^id(FIRTransaction *transaction, NSError **pError) {
+      runTransactionWithBlock:^id(FIRTransaction *transaction, NSError **) {
         [transaction updateData:self->_updateData forDocument:self->_docRef];
         return nil;
       }
-      completion:^(id result, NSError *error) {
+      completion:^(id, NSError *error) {
         XCTAssertNotNil(error);
         XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
         XCTAssertEqual(error.code, FIRFirestoreErrorCodeNotFound);
diff --git a/Firestore/Example/Tests/Integration/FSTSmokeTests.mm b/Firestore/Example/Tests/Integration/FSTSmokeTests.mm
@@ -42,7 +42,7 @@ - (void)testCanReadAWrittenDocument {
 }
 
 - (void)testObservesExistingDocument {
-  [self readerAndWriterOnDocumentRef:^(NSString *path, FIRDocumentReference *readerRef,
+  [self readerAndWriterOnDocumentRef:^(NSString *, FIRDocumentReference *readerRef,
                                        FIRDocumentReference *writerRef) {
     NSDictionary<NSString *, id> *data = [self chatMessage];
     [self writeDocumentRef:writerRef data:data];
@@ -59,7 +59,7 @@ - (void)testObservesExistingDocument {
 }
 
 - (void)testObservesNewDocument {
-  [self readerAndWriterOnDocumentRef:^(NSString *path, FIRDocumentReference *readerRef,
+  [self readerAndWriterOnDocumentRef:^(NSString *, FIRDocumentReference *readerRef,
                                        FIRDocumentReference *writerRef) {
     id<FIRListenerRegistration> listenerRegistration =
         [readerRef addSnapshotListener:self.eventAccumulator.valueEventHandler];
diff --git a/Firestore/Example/Tests/Util/FSTEventAccumulator.mm b/Firestore/Example/Tests/Util/FSTEventAccumulator.mm
@@ -97,7 +97,7 @@ - (BOOL)hasPendingWrites:(id)event {
 }
 
 - (void (^)(id _Nullable, NSError *_Nullable))valueEventHandler {
-  return ^void(id _Nullable value, NSError *_Nullable error) {
+  return ^void(id _Nullable value, NSError *) {
     // We can't store nil in the _events array, but these are still interesting to tests so store
     // NSNull instead.
     id event = value ? value : [NSNull null];
diff --git a/Firestore/Example/Tests/Util/FSTHelpers.mm b/Firestore/Example/Tests/Util/FSTHelpers.mm
@@ -142,7 +142,7 @@ PatchMutation FSTTestPatchMutation(const absl::string_view path,
 
   __block ObjectValue objectValue = ObjectValue::Empty();
   __block std::set<FieldPath> fieldMaskPaths;
-  [values enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *stop) {
+  [values enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *) {
     const FieldPath path = testutil::Field(util::MakeString(key));
     fieldMaskPaths.insert(path);
     if (![value isEqual:kDeleteSentinel]) {
diff --git a/Firestore/Source/API/FIRCollectionReference.mm b/Firestore/Source/API/FIRCollectionReference.mm
@@ -54,6 +54,8 @@ - (instancetype)initWithPath:(ResourcePath)path
 
 // Override the designated initializer from the super class.
 - (instancetype)initWithQuery:(api::Query &&)query {
+  (void)query;
+
   HARD_FAIL("Use FIRCollectionReference initWithPath: initializer.");
 }
 
diff --git a/Firestore/Source/API/FIRFieldPath.mm b/Firestore/Source/API/FIRFieldPath.mm
@@ -74,6 +74,7 @@ + (instancetype)pathWithDotSeparatedString:(NSString *)path {
 }
 
 - (id)copyWithZone:(NSZone *__nullable)zone {
+  (void)zone;
   return [[[self class] alloc] initPrivate:_internalValue];
 }
 
diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm
@@ -350,7 +350,7 @@ - (void)terminateWithCompletion:(nullable void (^)(NSError *_Nullable error))com
 
 - (id<FIRListenerRegistration>)addSnapshotsInSyncListener:(void (^)(void))listener {
   std::unique_ptr<core::EventListener<Empty>> eventListener =
-      core::EventListener<Empty>::Create([listener](const StatusOr<Empty> &v) { listener(); });
+      core::EventListener<Empty>::Create([listener](const StatusOr<Empty> &) { listener(); });
   std::unique_ptr<ListenerRegistration> result =
       _firestore->AddSnapshotsInSyncListener(std::move(eventListener));
   return [[FSTListenerRegistration alloc] initWithRegistration:std::move(result)];
diff --git a/Firestore/Source/API/FIRFirestoreSettings.mm b/Firestore/Source/API/FIRFirestoreSettings.mm
@@ -79,6 +79,8 @@ - (NSUInteger)hash {
 }
 
 - (id)copyWithZone:(nullable NSZone *)zone {
+  (void)zone;
+
   FIRFirestoreSettings *copy = [[FIRFirestoreSettings alloc] init];
   copy.host = _host;
   copy.sslEnabled = _sslEnabled;
diff --git a/Firestore/Source/API/FIRGeoPoint.mm b/Firestore/Source/API/FIRGeoPoint.mm
@@ -72,6 +72,7 @@ - (NSUInteger)hash {
 
 /** Implements NSCopying without actually copying because geopoints are immutable. */
 - (id)copyWithZone:(NSZone *_Nullable)zone {
+  (void)zone;
   return self;
 }
 
diff --git a/Firestore/Source/API/FIRQuery.mm b/Firestore/Source/API/FIRQuery.mm
@@ -358,10 +358,9 @@ - (FIRQuery *)queryFilteredUsingPredicate:(NSPredicate *)predicate {
     return [self queryFilteredUsingComparisonPredicate:predicate];
   } else if ([predicate isKindOfClass:[NSCompoundPredicate class]]) {
     return [self queryFilteredUsingCompoundPredicate:predicate];
-  } else if ([predicate isKindOfClass:[[NSPredicate
-                                          predicateWithBlock:^BOOL(id obj, NSDictionary *bindings) {
-                                            return true;
-                                          }] class]]) {
+  } else if ([predicate isKindOfClass:[[NSPredicate predicateWithBlock:^BOOL(id, NSDictionary *) {
+                          return true;
+                        }] class]]) {
     ThrowInvalidArgument("Invalid query. Block-based predicates are not supported. Please use "
                          "predicateWithFormat to create predicates instead.");
   } else {
diff --git a/Firestore/Source/API/FIRTimestamp.m b/Firestore/Source/API/FIRTimestamp.m
@@ -116,6 +116,7 @@ - (NSString *)description {
 
 /** Implements NSCopying without actually copying because timestamps are immutable. */
 - (id)copyWithZone:(NSZone *_Nullable)zone {
+  (void)zone;
   return self;
 }
 
diff --git a/Firestore/Source/API/FSTFirestoreComponent.mm b/Firestore/Source/API/FSTFirestoreComponent.mm
@@ -129,6 +129,8 @@ - (void)removeInstanceWithDatabase:(NSString *)database {
 #pragma mark - FIRComponentLifecycleMaintainer
 
 - (void)appWillBeDeleted:(FIRApp *)app {
+  (void)app;
+
   NSDictionary<NSString *, FIRFirestore *> *instances;
   @synchronized(_instances) {
     instances = [_instances copy];
diff --git a/Firestore/Source/API/FSTUserDataConverter.mm b/Firestore/Source/API/FSTUserDataConverter.mm
@@ -196,7 +196,7 @@ - (ParsedUpdateData)parsedUpdateData:(id)input {
   __block ParseContext context = accumulator.RootContext();
   __block ObjectValue updateData = ObjectValue::Empty();
 
-  [dict enumerateKeysAndObjectsUsingBlock:^(id key, id value, BOOL *stop) {
+  [dict enumerateKeysAndObjectsUsingBlock:^(id key, id value, BOOL *) {
     FieldPath path;
 
     if ([key isKindOfClass:[NSString class]]) {
@@ -295,7 +295,7 @@ - (FieldValue)parseDictionary:(NSDictionary<NSString *, id> *)dict
   } else {
     __block ObjectValue result = ObjectValue::Empty();
 
-    [dict enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *stop) {
+    [dict enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *) {
       absl::optional<FieldValue> parsedValue =
           [self parseData:value context:context.ChildContext(util::MakeString(key))];
       if (parsedValue) {
@@ -312,7 +312,7 @@ - (FieldValue)parseArray:(NSArray<id> *)array context:(ParseContext &&)context {
   __block FieldValue::Array result;
   result.reserve(array.count);
 
-  [array enumerateObjectsUsingBlock:^(id entry, NSUInteger idx, BOOL *stop) {
+  [array enumerateObjectsUsingBlock:^(id entry, NSUInteger idx, BOOL *) {
     absl::optional<FieldValue> parsedEntry = [self parseData:entry
                                                      context:context.ChildContext(idx)];
     if (!parsedEntry) {
diff --git a/Firestore/Source/CMakeLists.txt b/Firestore/Source/CMakeLists.txt
diff --git a/GoogleUtilities/CMakeLists.txt b/GoogleUtilities/CMakeLists.txt
diff --git a/Interop/Auth/CMakeLists.txt b/Interop/Auth/CMakeLists.txt
diff --git a/Interop/CoreDiagnostics/CMakeLists.txt b/Interop/CoreDiagnostics/CMakeLists.txt
diff --git a/cmake/cc_rules.cmake b/cmake/cc_rules.cmake

Original file line number	Diff line number	Diff line change
`@@ -97,7 +97,7 @@ - (BOOL)hasPendingWrites:(id)event {`
`97`	`97`	`}`
`98`	`98`
`99`	`99`	`- (void (^)(id _Nullable, NSError *_Nullable))valueEventHandler {`
`100`		`- return ^void(id _Nullable value, NSError *_Nullable error) {`
	`100`	`+ return ^void(id _Nullable value, NSError *) {`
`101`	`101`	`// We can't store nil in the _events array, but these are still interesting to tests so store`
`102`	`102`	`// NSNull instead.`
`103`	`103`	`id event = value ? value : [NSNull null];`
Original file line number	Diff line number	Diff line change
`@@ -54,6 +54,8 @@ - (instancetype)initWithPath:(ResourcePath)path`
`54`	`54`
`55`	`55`	`// Override the designated initializer from the super class.`
`56`	`56`	`- (instancetype)initWithQuery:(api::Query &&)query {`
	`57`	`+ (void)query;`
	`58`	`+`
`57`	`59`	`HARD_FAIL("Use FIRCollectionReference initWithPath: initializer.");`
`58`	`60`	`}`
`59`	`61`
Original file line number	Diff line number	Diff line change
`@@ -74,6 +74,7 @@ + (instancetype)pathWithDotSeparatedString:(NSString *)path {`
`74`	`74`	`}`
`75`	`75`
`76`	`76`	`- (id)copyWithZone:(NSZone *__nullable)zone {`
	`77`	`+ (void)zone;`
`77`	`78`	`return [[[self class] alloc] initPrivate:_internalValue];`
`78`	`79`	`}`
`79`	`80`