Shorten CMake Objective-C targets #5323

wilhuff · 2020-04-09T01:56:09Z

This is a continuation of the work in #5321, renaming Objective-C and other nonessential targets from the point of view of the Windows build to match the pattern set out there.

The intent is that targets with a firestore_objc_ prefix are built from Firestore/Sources and Firestore/Example. Those directories will eventually move to into Firestore/ObjC (see go/firestore-spm for details). This PR anticipates that move.

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

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.

... making them more like built-in functions. This makes it possible to combine other CMake functions in ways that weren't possible with the original all-in-one approach. This includes: * firebase_ios_add_executable * firebase_ios_add_test * firebase_ios_add_library All of which can be combined with target_link_libraries, etc.

... and migrate it to new CMake primitives.

... and migrate to firebase_ios_add_test

... and migrate to firebase_ios_add_test.

Rename * firebase_firestore_protos_nanopb => firestore_protos_nanopb * firebase_firestore_protos_libprotobuf => firestore_protos_protobuf Also delete dead code in Firestore/Protos/CMakeLists.txt

... and migrate to firebase_ios_add_library.

... and: * migrate to firebase_ios_add_test * use globs to find sources

... and: * migrate to firebase_ios_add_test * use globs to find sources * shorten firebase_firestore_remote_testing too

... and: * migrate to firebase_ios_add_test * collapse all tests down into just firestore_util_test

Now that we're committed to using globs for finding sources, this check is incompatible and no longer useful.

... and: * Migrate to firebase_ios_add_executable * Use globs for specifying sources

Rename firebase_firestore_objc_test_util to firestore_objc_testing.

Rewrite firebase_ios_cc_fuzz_test to match the other primitives.

google-oss-bot · 2020-04-09T01:56:58Z

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

Generated by 🚫 Danger

…tests

…-objc

…tests

…-objc

dconeybe

LGTM. Just a few comments about the usage of FIREBASE_IOS_BUILD_TESTS.

dconeybe · 2020-04-16T17:16:13Z

Firestore/Example/Tests/API/CMakeLists.txt

-      firebase_firestore_objc_test_util
-  )
-endif(APPLE)
+if(NOT FIREBASE_IOS_BUILD_TESTS OR NOT APPLE)


Testing the value of FIREBASE_IOS_BUILD_TESTS and APPLE is different from the original file, which only tested APPLE. Can you confirm that this is an intended change?

Great question. Sorry, I should have added a comment about this to the PR.

Previously, when using firebase_ios_objc_test, that command could check FIREBASE_IOS_BUILD_TESTS for itself and omit setting up the target because the command was (essentially) self-contained.

Now that I've switched us to using an arrangement where multiple commands are used to build up a target, it's no longer possible to usefully check for FIREBASE_IOS_BUILD_TESTS in a command like firebase_ios_add_objc_test. The problem is that if we omit the target, subsequent commands like target_link_libraries will fail if the target doesn't exist. That means we have to check outside firebase_ios_add_objc_test, and checking at the file level makes sense because this isn't necessarily uniform (though it is for this PR).

dconeybe · 2020-04-16T17:17:31Z

Firestore/Example/Tests/SpecTests/CMakeLists.txt

-    WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
-  )
-endif(APPLE)
+if(NOT FIREBASE_IOS_BUILD_TESTS OR NOT APPLE)


Same here... please confirm that checking FIREBASE_IOS_BUILD_TESTS is indeed intended.

This one's correct.

dconeybe · 2020-04-16T17:18:30Z

Firestore/Example/Tests/Integration/CMakeLists.txt

@@ -16,26 +16,17 @@ if(NOT APPLE)
  return()


Should FIREBASE_IOS_BUILD_TESTS be checked in this file as well?

Yes! Fixed.

* Shorten firebase_firestore_example_app ... and: * Migrate to firebase_ios_add_executable * Use globs for specifying sources * Migrate Example/Tests/API to new CMake primitives * Migrate Example/Tests/Integration to new CMake primitives * Migrate Example/Tests/SpecTests to new CMake primitives * Migrate Example/Tests/Util to new CMake primitives Rename firebase_firestore_objc_test_util to firestore_objc_testing. * Migrate fuzzing to new CMake primitives Rewrite firebase_ios_cc_fuzz_test to match the other primitives. * Add firebase_ios_add_objc_test * Migrate Example/Benchmarks to new CMake primitives

wilhuff added 27 commits April 8, 2020 09:19

Fix previously unknown warnings

1c3fe2d

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

style.sh generated changes

c1bc085

Shorten firebase_firestore_testutil

05d4a0c

... and migrate it to new CMake primitives.

Shorten firebase_firestore_types_test

2cd4a3a

... and migrate to firebase_ios_add_test

Shorten firebase_firestore_api_test

2da6413

... and migrate to firebase_ios_add_test.

Shorten firebase_firestore_auth_test

ab4e383

... and migrate to firebase_ios_add_test.

Shorten firebase_firestore_core_test

848dccd

... and migrate to firebase_ios_add_test.

Shorten firebase_firestore_immutable_test

874512c

... and migrate to firebase_ios_add_test.

Shorten firebase_firestore_protos_*

c8299f1

Rename * firebase_firestore_protos_nanopb => firestore_protos_nanopb * firebase_firestore_protos_libprotobuf => firestore_protos_protobuf Also delete dead code in Firestore/Protos/CMakeLists.txt

Shorten firebase_firestore_local_testing

4689b63

... and migrate to firebase_ios_add_library.

Shorten firebase_firestore_local_test

6f8fead

... and: * migrate to firebase_ios_add_test * use globs to find sources

Shorten firebase_firestore_model_test

cca0d4b

... and: * migrate to firebase_ios_add_test * use globs to find sources

Shorten firebase_firestore_nanopb_test

e9d6c56

... and: * migrate to firebase_ios_add_test * use globs to find sources

Shorten firebase_firestore_objc_test

01127eb

... and: * migrate to firebase_ios_add_test * use globs to find sources

Shorten firebase_firestore_remote_test

43fba1b

... and: * migrate to firebase_ios_add_test * use globs to find sources * shorten firebase_firestore_remote_testing too

Shorten firebase_firestore_util_test

84629db

... and: * migrate to firebase_ios_add_test * collapse all tests down into just firestore_util_test

Remove CMake file inclusion check

564c2f9

Now that we're committed to using globs for finding sources, this check is incompatible and no longer useful.

Add firebase_ios_add_objc_test

b46245c

Shorten firebase_firestore_example_app

b4acc4c

... and: * Migrate to firebase_ios_add_executable * Use globs for specifying sources

Migrate Example/Benchmarks to new CMake primitives

5db5510

Migrate Example/Tests/API to new CMake primitives

a45ad45

Migrate Example/Tests/Integration to new CMake primitives

aef203b

Migrate Example/Tests/SpecTests to new CMake primitives

8c08280

Migrate Example/Tests/Util to new CMake primitives

cc01e06

Rename firebase_firestore_objc_test_util to firestore_objc_testing.

Migrate fuzzing to new CMake primitives

489757a

Rewrite firebase_ios_cc_fuzz_test to match the other primitives.

wilhuff added the api: firestore label Apr 9, 2020

wilhuff requested a review from dconeybe April 9, 2020 01:56

wilhuff assigned dconeybe Apr 9, 2020

googlebot added the cla: yes label Apr 9, 2020

wilhuff added 9 commits April 10, 2020 14:11

Review feedback

60882c2

Merge branch 'wilhuff/cmake-no-framework' into wilhuff/cmake-shorten-…

9935bb1

…tests

Merge branch 'wilhuff/cmake-shorten-tests' into wilhuff/cmake-shorten…

e2a8744

…-objc

More review feedback

6173b37

Merge branch 'master' into wilhuff/cmake-no-framework

c7e5257

Merge branch 'wilhuff/cmake-no-framework' into wilhuff/cmake-shorten-…

6386ffa

…tests

Merge branch 'wilhuff/cmake-shorten-tests' into wilhuff/cmake-shorten…

20f70d7

…-objc

Merge branch 'master' into wilhuff/cmake-shorten-tests

420a10e

Merge branch 'wilhuff/cmake-shorten-tests' into wilhuff/cmake-shorten…

3b38dec

…-objc

wilhuff changed the base branch from wilhuff/cmake-shorten-tests to master April 15, 2020 18:03

dconeybe approved these changes Apr 16, 2020

View reviewed changes

dconeybe assigned wilhuff and unassigned dconeybe Apr 16, 2020

Review feedback

a3fedc5

wilhuff merged commit 314f137 into master Apr 20, 2020

wilhuff deleted the wilhuff/cmake-shorten-objc branch April 20, 2020 17:30

firebase locked and limited conversation to collaborators May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Shorten CMake Objective-C targets #5323

Shorten CMake Objective-C targets #5323

Uh oh!

wilhuff commented Apr 9, 2020

Uh oh!

google-oss-bot commented Apr 9, 2020

Uh oh!

dconeybe left a comment

Uh oh!

dconeybe Apr 16, 2020

Uh oh!

wilhuff Apr 17, 2020

Uh oh!

dconeybe Apr 16, 2020

Uh oh!

wilhuff Apr 17, 2020

Uh oh!

dconeybe Apr 16, 2020

Uh oh!

wilhuff Apr 17, 2020

Uh oh!

Uh oh!

Shorten CMake Objective-C targets #5323

Shorten CMake Objective-C targets #5323

Uh oh!

Conversation

wilhuff commented Apr 9, 2020

Uh oh!

google-oss-bot commented Apr 9, 2020

Uh oh!

dconeybe left a comment

Choose a reason for hiding this comment

Uh oh!

dconeybe Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

wilhuff Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

dconeybe Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

wilhuff Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

dconeybe Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

wilhuff Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!