[CMake] Implement absl_cc_library as Bazel's cc_library by rongjiecomputer · Pull Request #136 · abseil/abseil-cpp

rongjiecomputer · 2018-06-29T01:00:58Z

absl_cc_library is a cc_library-like CMake function to replace absl_library and absl_header_library.

Please do not start any migration until absl_cc_binary and absl_cc_test are implemented in future PRs. I migrated a few targets just to showcase how absl_cc_library can be used.

#114

@JonathanDCohen PTAL

JonathanDCohen · 2018-08-21T17:27:18Z

CMake/AbseilHelpers.cmake

+# COPTS: List of private compile options
+# DEFINES: List of public defines
+# LINKOPTS: List of link options
+# VISIBILITY_PUBLIC: Add this so that this library will be exported under absl:: (see Note).


Maybe just PUBLIC? Or is that reserved by cmake?

PUBLIC is not reserved by CMake, but I just thought that VISIBILITY_PUBLIC is easier to understand for Abseil team since Bazel uses //visibility:public to mean that "targets outside Abseil can depend on it". I can change it to PUBLIC if you think that is better.

I think just PUBLIC is fine

JonathanDCohen · 2018-08-21T17:29:43Z

CMake/AbseilHelpers.cmake

+#
+# Note:
+#
+# By default, absl_cc_library will always create a library named absl_${NAME},


Here's an idea, while we're here. If VISIBILITY_PUBLIC is not set, the prefix should be absl_internal_blah, otherwise it should be absl::blah. What do you think? I'm okay with "internal" appearing a bunch of times in the name.

absl_internal_ preffix will reduce the chance of name collision, but we might still face name collision within Abseil itself (some targets have name that are too general like test_util in //absl/time package). I have a longer response at https://groups.google.com/forum/#!topic/abseil-io/_Pq2NuCuYcc

I am settling at absl_ preffix for now since Abseil's CMake files use absl_ preffix too (e.g. absl_throw_delegate), so I don't have to do too much manual editing when showing some possible migrations in this PR.

I'm not really worried about name collisions as much as I'm trying to make it as inconvenient as possible to rely on abseil internals, since we have no true hiding mechanism as we do in Bazel. Does that make more sense?

JonathanDCohen · 2018-08-21T17:30:32Z

CMake/AbseilHelpers.cmake

+    ${ARGN}
+  )
+
+  if (NOT ABSL_CC_LIB_TESTONLY OR BUILD_TESTING)


should this be ABSL_RUN_TESTS?

You mean replace BUILD_TESTING with ABSL_RUN_TESTS? There is no difference actually.

When ABSL_RUN_TESTS is not set, CTest won't be included and BUILD_TESTING won't be specified. Un-specified variable has default value of "false" when accessed.

I realized that BUILD_TESTING is global, not project-specific, so depending on BUILD_TESTING will force consumer to run Abseil tests when they just want to run their own tests. Done.

JonathanDCohen · 2018-08-21T17:34:26Z

CMake/AbseilHelpers.cmake

+    if(ABSL_CC_LIB_SRCS_LEN)
+      add_library(${_NAME} STATIC ${ABSL_CC_LIB_SRCS} ${ABSL_CC_LIB_HDRS})
+    else()
+      set(__dummy_header_only_lib_file "${CMAKE_CURRENT_BINARY_DIR}/${_NAME}_header_only_dummy.cc")


Let's take the chance to improve header-only libs while we're here. Can we not make header-only libs INTERFACE libraries? Then we just declare their deps as normal and the headers get picked up by ABSL_COMMON_INCLUDE_DIRS.

Yes we can! Thanks for the reminder. Done.

JonathanDCohen · 2018-08-21T18:27:42Z

absl/strings/CMakeLists.txt

-    ${STRINGS_PUBLIC_LIBRARIES}
-  EXPORT_NAME
+absl_cc_library(
+  NAME


I would rather we didn't immediately migrate something so widely used as strings. I think the exception-safety testing is fine, it has a small footprint. If you want another trial case, let's try something internal instead.

JonathanDCohen · 2018-08-21T18:28:08Z

Thanks for this! It's been gathering dust on my todo list for a while

JonathanDCohen · 2018-08-23T18:22:42Z

CMake/AbseilHelpers.cmake

+# COPTS: List of private compile options
+# DEFINES: List of public defines
+# LINKOPTS: List of link options
+# VISIBILITY_PUBLIC: Add this so that this library will be exported under absl:: (see Note).


I think just PUBLIC is fine

CMake/AbseilHelpers.cmake

rongjiecomputer · 2018-08-28T10:34:38Z

I have no more changes to make until next round of review.

rongjiecomputer · 2018-09-18T06:59:20Z

@JonathanDCohen Is there anything else I need to address?

rongjiecomputer · 2018-10-05T08:48:57Z

Ping @JonathanDCohen

CMake/AbseilHelpers.cmake

JonathanDCohen · 2018-10-09T15:31:27Z

Sorry for the delay, this fell off of my radar

cc_library

CMake/AbseilHelpers.cmake

Mizux · 2018-10-10T11:15:42Z

CMake/AbseilHelpers.cmake

+#     absl_internal_awesome_lib # not "awesome_lib"!
+# )
+#
+# If PUBLIC is set, absl_cc_library will also create an alias absl::${NAME}


why don't using "absl_" prefix for PUBLIC enable libraries ?

It is already there. The following code will create absl_strings and absl::strings (alias). CMake user should depend on absl::strings only.

absl_cc_library( NAME strings PUBLIC )

no it will create absl_internal_strings and absl::strings line 125
you should have something like this:

if (ABSL_CC_LIB_PUBLIC) set(_NAME "absl_${ABSL_CC_LIB_NAME}") else() set(_NAME "absl_internal_${ABSL_CC_LIB_NAME}") endif()

no it will create absl_internal_strings and absl::strings line 125

Ah, I forgot that the prefix has changed to absl_internal_ (it was initially just absl_, but Jonathan requested to always use absl_internal_ for non-public targets and I didn't think of creating exception for public targets).

Is it better for user to use absl_strings instead of absl::strings? Because I want to make it clear to CMake user that only absl:: can be depended outside Abseil. (No strong objection here, just want your opinion).

Since:

Abseil force user to use abseil-cpp through add_subdirectory(abseil-cpp),
So meta project who want to add an install rules will fail at configure time if they don't add absl_string to one of their install(TARGETS ... (that's why so much people want install rule cf Step 6: Allow Abseil to be installed and used with find_package() #111).

Twist:

An ALIAS target may not be installed or exported.

src: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries

-> 1 + 2 -> you can't integrate abseil-cpp in your project (and only use absl::) unless you'll never install your awesome target...
(i.e. unless target dependency is an imported target, it must be part of your install, aka avoid to install half target in the build tree)
note: for GTEST etc it works to use add_subdirectory() tricks since usually no one install test executables (and all their dependencies needed and build in the build_dir aka gtest)

CMake/AbseilHelpers.cmake

rongjiecomputer · 2018-10-10T15:26:36Z

@Mizux Really appreciate your comments, keep them coming! I really need to go to sleep now, so don't wait for my response.

Mizux · 2018-10-11T09:37:50Z

LGTM to me thanks @rongjiecomputer
@JonathanDCohen seems odd that PR don't have any CI job associated to them (you should strongly consider having Travis/Appveyor jobs than only kokoro ones)

CMake/AbseilHelpers.cmake

JonathanDCohen

looks good to me besides the small comment change. @Mizux look good for you?

Mizux · 2018-10-15T06:28:01Z

Yes everything sound good to me.

note: I will rework #182 once this one is merged

pifon2a · 2018-10-17T13:04:39Z

@JonathanDCohen is it ready to merge?

-- 4e043a11b4c10a24e84046827ee16f47e11e35cc by Abseil Team <absl-team@google.com>: Merge of #136 PiperOrigin-RevId: 218197648 -- e61f06e1e601061a443feaa8c5207c52437bd641 by Abseil Team <absl-team@google.com>: Don't include <iostream> into int128, it's wasteful Including iostream emits a global constructor for initializing std::cout and friends, which isn't actually used by this file. PiperOrigin-RevId: 218156386 -- 8a6c82396e4c956be7f285328aec131cb4965f16 by Xiaoyi Zhang <zhangxy@google.com>: Fix MSVC compiler warnings on discarding return values of functions with 'nodiscard' attribute. PiperOrigin-RevId: 217883401 -- abf3e3a0f22bc4070df9dbc9a4ef4d883ed686bf by Tom Manshreck <shreck@google.com>: Update public README to add new libraries PiperOrigin-RevId: 217879399 -- 43b3b420a4e861711abbfbd497b8f2b3de17ec8c by Abseil Team <absl-team@google.com>: Import of CCTZ from GitHub. PiperOrigin-RevId: 217780963 -- 1c8831947ca6a65a63842e6bd5f37a7c102a4e1b by Abseil Team <absl-team@google.com>: Fix typo in a comment (missing comma in usage example). PiperOrigin-RevId: 217776645 GitOrigin-RevId: 4e043a11b4c10a24e84046827ee16f47e11e35cc Change-Id: I8999ae928da7a0030b4ecfd8d13da8522fdd013a

-- 4e043a11b4c10a24e84046827ee16f47e11e35cc by Abseil Team <absl-team@google.com>: Merge of abseil/abseil-cpp#136 PiperOrigin-RevId: 218197648 -- e61f06e1e601061a443feaa8c5207c52437bd641 by Abseil Team <absl-team@google.com>: Don't include <iostream> into int128, it's wasteful Including iostream emits a global constructor for initializing std::cout and friends, which isn't actually used by this file. PiperOrigin-RevId: 218156386 -- 8a6c82396e4c956be7f285328aec131cb4965f16 by Xiaoyi Zhang <zhangxy@google.com>: Fix MSVC compiler warnings on discarding return values of functions with 'nodiscard' attribute. PiperOrigin-RevId: 217883401 -- abf3e3a0f22bc4070df9dbc9a4ef4d883ed686bf by Tom Manshreck <shreck@google.com>: Update public README to add new libraries PiperOrigin-RevId: 217879399 -- 43b3b420a4e861711abbfbd497b8f2b3de17ec8c by Abseil Team <absl-team@google.com>: Import of CCTZ from GitHub. PiperOrigin-RevId: 217780963 -- 1c8831947ca6a65a63842e6bd5f37a7c102a4e1b by Abseil Team <absl-team@google.com>: Fix typo in a comment (missing comma in usage example). PiperOrigin-RevId: 217776645 GitOrigin-RevId: 4e043a11b4c10a24e84046827ee16f47e11e35cc Change-Id: I8999ae928da7a0030b4ecfd8d13da8522fdd013a

googlebot added the cla: yes label Jun 29, 2018

derekmauro assigned JonathanDCohen Jun 29, 2018

derekmauro requested a review from JonathanDCohen June 29, 2018 01:27

JonathanDCohen suggested changes Aug 21, 2018

View reviewed changes

rongjiecomputer force-pushed the cc_library branch from c142746 to 162c1a9 Compare August 22, 2018 08:34

JonathanDCohen reviewed Aug 24, 2018

View reviewed changes

rongjiecomputer force-pushed the cc_library branch from 8849b05 to 59a1f35 Compare September 5, 2018 00:20

JonathanDCohen reviewed Oct 9, 2018

View reviewed changes

CMake/AbseilHelpers.cmake Show resolved Hide resolved

CMake/AbseilHelpers.cmake Show resolved Hide resolved

rongjiecomputer added 9 commits October 9, 2018 23:33

[CMake] Implement absl_cc_library as Bazel's cc_library

ac533e1

cc_library

Implement header-only target as INTERFACE lib

fcc476c

Use ABSL_RUN_TESTS instead of BUILD_TESTING

027ebfd

Migrate some internal base targets

73b908f

Revert migration for //absl/strings/...

f1c17a3

Set default prefix to absl_internal_

1ecb60b

Fix one comment

67058cb

Convert VISIBILITY_PUBLIC to PUBLIC

6b16ba0

Add comment and todo

a76ceed

rongjiecomputer force-pushed the cc_library branch from 59a1f35 to a76ceed Compare October 9, 2018 15:36

JonathanDCohen mentioned this pull request Oct 9, 2018

[WIP] CMake Install rule proposal #182

Closed

2 tasks

Mizux reviewed Oct 10, 2018

View reviewed changes

CMake/AbseilHelpers.cmake Show resolved Hide resolved

Mizux reviewed Oct 10, 2018

View reviewed changes

CMake/AbseilHelpers.cmake Show resolved Hide resolved

Mizux reviewed Oct 10, 2018

View reviewed changes

CMake/AbseilHelpers.cmake Show resolved Hide resolved

rongjiecomputer added 2 commits October 10, 2018 23:15

Use absl_ prefix for public target

fc2185a

Update comments too

f59e0f1

Use target_sources and remove one non-existent file

dadcd56

JonathanDCohen reviewed Oct 12, 2018

View reviewed changes

CMake/AbseilHelpers.cmake Outdated Show resolved Hide resolved

JonathanDCohen reviewed Oct 12, 2018

View reviewed changes

Update TESONLY comment

bd2d9a4

Mizux mentioned this pull request Oct 15, 2018

[CMAKE] absl::container target should depend on absl::hash #183

Closed

JonathanDCohen approved these changes Oct 18, 2018

View reviewed changes

JonathanDCohen added the kokoro:run label Oct 18, 2018

kokoro-team removed kokoro:run labels Oct 18, 2018

ahedberg merged commit 5fbde92 into abseil:master Oct 22, 2018

rongjiecomputer deleted the cc_library branch October 23, 2018 00:16

Conversation

rongjiecomputer commented Jun 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanDCohen commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rongjiecomputer commented Aug 28, 2018

Uh oh!

rongjiecomputer commented Sep 18, 2018

Uh oh!

rongjiecomputer commented Oct 5, 2018

Uh oh!

Uh oh!

Uh oh!

JonathanDCohen commented Oct 9, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rongjiecomputer Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mizux Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rongjiecomputer commented Oct 10, 2018

Uh oh!

Mizux commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JonathanDCohen left a comment

Choose a reason for hiding this comment

Uh oh!

Mizux commented Oct 15, 2018

rongjiecomputer Oct 10, 2018 •

edited

Loading

Mizux Oct 10, 2018 •

edited

Loading

Mizux commented Oct 11, 2018 •

edited

Loading