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.

Sign up for GitHub

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

Jump to bottom

Add a cc_library to the CMake build #670

Merged
merged 5 commits into from
Jan 17, 2018
Merged

Add a cc_library to the CMake build #670

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e9b7758
Rewrite cc_test to take named arguments
wilhuff Jan 13, 2018
37c3a5a
Add a cc_library rule to parallel cc_test
wilhuff Jan 13, 2018
d349188
Automatically add OBJC_FLAGS to cc_libraries if applicable
wilhuff Jan 13, 2018
c97dcde
Exclude platform-specific libraries from 'all'
wilhuff Jan 16, 2018
1815899
Restore secure_random_test.cc; clean-up comments
wilhuff Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions Firestore/core/src/firebase/firestore/remote/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@
# See the License for the specific language governing permissions and
# limitations under the License.

add_library(
cc_library(
firebase_firestore_remote
datastore.h
datastore.cc
)
target_link_libraries(
firebase_firestore_remote
grpc::grpc
SOURCES
datastore.h
datastore.cc
DEPENDS
grpc::grpc
)
93 changes: 45 additions & 48 deletions Firestore/core/src/firebase/firestore/util/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,69 +16,66 @@
# libraries in here are an implementation detail of making this a
# mutli-platform build.

add_library(
cc_library(
firebase_firestore_util_base
secure_random_arc4random.cc
string_printf.cc
)
target_link_libraries(
firebase_firestore_util_base
PUBLIC
absl_base
SOURCES
secure_random.h
secure_random_arc4random.cc
string_printf.cc
string_printf.h
DEPENDS
absl_base
)

## assert and log

# stdio-dependent bits can be built and tested everywhere
add_library(
firebase_firestore_util_stdio
assert_stdio.cc
log_stdio.cc
)
target_link_libraries(
cc_library(
firebase_firestore_util_stdio
PUBLIC
firebase_firestore_util_base
SOURCES
assert_stdio.cc
log_stdio.cc
DEPENDS
firebase_firestore_util_base
absl_base
)

# apple-dependent bits can only built and tested on apple plaforms
Copy link
Member

Choose a reason for hiding this comment

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

You've dropped 'if(APPLE)'. Was that on purpose? If so, I'd recommend updating this comment.

Update: Ah, I see you have an ifAPPLE down on line 61 that effectively does this. It's probably simplest to just move this comment down there. Or alternatively, just delete the comment; the 'ifAPPLE' seems descriptive enough.

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've deleted the comment though I'm still not sure this is the best approach. I was hoping that there would at least be some clue as to why we're excluding these targets from 'all'.

if(APPLE)
add_library(
firebase_firestore_util_apple
cc_library(
firebase_firestore_util_apple
SOURCES
assert_apple.mm
log_apple.mm
)
target_compile_options(
firebase_firestore_util_apple
PRIVATE
${OBJC_FLAGS}
)
target_link_libraries(
firebase_firestore_util_apple
PUBLIC
string_apple.h
DEPENDS
FirebaseCore
)
endif(APPLE)

add_library(
firebase_firestore_util
autoid.cc
)
target_compile_options(
Copy link
Member

Choose a reason for hiding this comment

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

Update: Ah, I see you've already handled this in the next commit. I'll leave my comment here, as it's an alternative to consider, but I think yours is just fine too. No action required.

Since this really only applies to the above cc_library, can we move it right into it like you've done with DEPENDS? i.e. something like this:

cc_library(
  firebase_firestore_util_apple
  SOURCES
    ...
  DEPENDS
    ...
  COMPILE_OPTIONS
    PRIVATE
    ${OBJC_FLAGS}
)

Or something like that. (No idea if that's possible with cmake or not.)

If you do elect to do this (assuming it's even possible) then I don't think it needs to be part of this PR (though I have no objections to that either.)

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'd prefer to avoid having to think about this and get this right so I'll stick with the automatic approach.

firebase_firestore_util_apple
PRIVATE
${OBJC_FLAGS}
)

# Export a dependency on the correct logging library for this platform. All
# buildable libraries are built and tested but only the best fit is exported.
if(APPLE)
target_link_libraries(
firebase_firestore_util
PUBLIC
firebase_firestore_util_apple
firebase_firestore_util_base
)
list(APPEND UTIL_DEPENDS firebase_firestore_util_apple)
else()
list(APPEND UTIL_DEPENDS firebase_firestore_util_stdio)
endif()

else(NOT APPLE)
target_link_libraries(
firebase_firestore_util
PUBLIC
firebase_firestore_util_stdio
firebase_firestore_util_base
)

endif(APPLE)
## main library

cc_library(
firebase_firestore_util
SOURCES
autoid.cc
autoid.h
firebase_assert.h
log.h
DEPENDS
${UTIL_DEPENDS}
firebase_firestore_util_base
absl_base
)
23 changes: 23 additions & 0 deletions cmake/utils.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,29 @@

include(CMakeParseArguments)

# cc_library(
# target
# SOURCES sources...
# DEPENDS libraries...
# )
#
# Defines a new library target with the given target name, sources, and dependencies.
function(cc_library name)
set(multi DEPENDS SOURCES)
cmake_parse_arguments(ccl "" "" "${multi}" ${ARGN})

add_library(
${name}
${ccl_SOURCES}
)
target_link_libraries(
${name}
PUBLIC
${ccl_DEPENDS}
)

endfunction()

# cc_test(
# target
# SOURCES sources...
Expand Down