Add CMake support #8

adevress · 2017-09-27T03:44:00Z

Initial partial CMake support for abseil, created after the discussion of this afternoon.

Do not merge yet, this is still incomplete.

I created the pull request for tracking and avoid duplicated effort.

Adev

kumagi · 2017-09-27T06:40:27Z

👍 My CLion requires CMake.

Orphis · 2017-09-27T09:06:15Z

If you are targeting CMake 3.0, you should consider exporting imported targets from your Find modules instead of passing string variables in & out of those.

tituswinters · 2017-09-27T15:55:23Z

The one constraint I know of on this: it needs to support back as far as CMake 2.8 (because I know that other Google projects like protobuf and gRPC require that, and I have to support those).

adevress · 2017-09-27T16:19:11Z

@tituswinters Thx Titus, I was not sure about that, that's why I kept compatibility with 2.8's style.

Orphis · 2017-09-27T23:06:43Z

Just being pedantic, but CMake 2.8 doesn't mean much, unless you mean 2.8.0. CMake didn't use semver before, so you should explicitly say which version of 2.8.x you intend to support.

adevress · 2017-09-28T05:54:19Z

@Orphis Fixed

Most of the staff should be there now. The remaining bits are types/utility/memory/meta
Abseil compiles with cmake and unittests passed without issues on OSX and RHEL6 on my side.

Orphis · 2017-09-28T14:44:16Z

Out of curiosity, where did you get the 2.8.5? I checked protobuf and it's using 2.8.12 for example, and it's way more advanced than 2.8.5.

adevress · 2017-09-28T15:36:38Z

Out of curiosity, where did you get the 2.8.5?

@Orphis 2.8.5, if my memory is correct, was the first initial verison shipped under RHEL5 and Debian Wheezy. Meaning it should build even under prehistoric monsters and still be compatible with protobuf / gRPC. There is very little difference between 2.8.5 / 2.8.11 anyway.

Orphis · 2017-09-28T22:02:52Z

Well, 2.8.12 added target_compile_options() which is currently used in the code above, so the minimum version should be that one. Other commands in the target_ family have been added and make it way more robust to work with since you deal less with strings and just target objects instead.

It doesn't change the fact you could define IMPORTED targets in your find modules instead, those have been around for a while, though I'm not sure if all the support functionality is there for them.

adevress · 2017-09-28T23:09:12Z

@Orphis Thx I ignored that. protobuf is 2.8.12, let's use 2.8.12 then.

I have also granted you rw access to my fork like you seem motivated to contribute.

agauniyal · 2017-09-29T04:16:33Z

Would this mean I don't have to use Bazel?

tituswinters · 2017-09-29T18:12:13Z

@agauniyal - Yes. Abseil team will still primarily support bazel, but we'll do our best to keep both working once something like this lands.

agauniyal · 2017-09-29T22:58:43Z

@tituswinters just to be clear I've nothing against bazel, its just that cmake is more popular right now in c++ ecosystem and maybe if bazel starts getting used I'll be happy to be on primary build system :)

tituswinters · 2017-09-29T23:13:59Z

@agauniyal - And to be clear - we've got nothing against cmake, it's just not what we're experienced with or primarily focused on.

Orphis · 2017-10-26T09:51:41Z

CMake/AbseilHelpers.cmake

+  )
+
+
+  add_library(${_NAME} SHARED ${__dummy_header_only_lib_file})


Why is this SHARED? STATIC would be more appropriate here.

Sorry for that, stupid late mistake.

I was thinking to provide a switch to compile everything static or everything shared for that since we are aiming to support embedded deployment by defaylt, enforcing STATIC does not really make a sense anymore

It doesn't make sense to build dummy libraries as SHARED though.

Orphis · 2017-10-26T09:53:11Z

CMakeLists.txt

+## please use these targets in your project
+#
+#
+add_library(absl::algorithm ALIAS absl_algorithm)


This would be better done in the macros defining the targets.

I did that on purpose to make explicit to the user what are the public target to use. I would prefer maintain it like that excepted if you have strong objections.

At some point, someone will add a new target and forget to add the line in there. It's better if it's done by the macro automatically.
This type of information needs to go in documentation actually.

I understand your point.
However, I am convince that 70% of your user will not even take the time to check the documentation like always :)

I fixed that. The alias is automatically generated in the macro if tagged explicitely, to allow private / public namespace and a comment is added in the main CMake file for the user.
I will also add this information in the doc

Orphis · 2017-10-26T09:54:29Z

absl/algorithm/CMakeLists.txt

+list(APPEND ALGORITHM_TEST_SRC
+  "algorithm_test.cc"
+  ${ALGORITHM_PUBLIC_HEADERS}
+  ${ALGORITHM_INTERNAL_HEADERS}


The test only has one file. Don't put the other headers in there.

It is there for a reason. The header need to be associated explicitely with the sources of a target if you want them to be visible into an IDE that supports CMake. I do not like it, but it is the recommended way.

I can however associate it to the header only target, since we have them now.

Orphis · 2017-10-26T09:55:00Z

absl/algorithm/CMakeLists.txt

+  TARGET
+    algorithm_test
+  SOURCES
+    ${ALGORITHM_TEST_SRC}


The test should link against its own library, even if it's header only.

Fine for me

Fixed. Other tests will adopt the same pattern.

Orphis · 2017-10-26T09:58:25Z

CMake/AbseilHelpers.cmake

+  set(_NAME ${ABSL_HO_LIB_TARGET})
+
+  set(__dummy_header_only_lib_file "${CMAKE_CURRENT_BINARY_DIR}/${_NAME}_header_only_dummy.cc")
+  file(WRITE ${__dummy_header_only_lib_file}


If you write all the time at generation step, you will cause rebuilds everytime. You should check the file doesn't exist before overwriting it.

JonathanDCohen · 2017-10-27T14:56:40Z

absl/container/CMakeLists.txt

+)
+
+
+# test test_instance_tracker_test


I heard you like tests bro

ahah.

On the way to the infinite test inception ✌️

I am not crazy enough to write all of that CMake code manually, I wrote an generator for the tests :)

My generator prefixed "test" before a test name "test" inside bazel and then "suffixed" test for the target naming reason. Who said we need more testing ? :)

Orphis · 2017-10-27T15:40:10Z

CMake/README.md

+    add_subdirectory(abseil-cpp)
+
+    add_executable(my_exe source.cpp)
+    target_link_libraries(my_exe base synchronization strings)


You'll need to fix the documentation too with the new scoped names

JonathanDCohen · 2017-10-27T15:00:26Z

absl/debugging/CMakeLists.txt

+    ${STACKTRACE_PUBLIC_LIBRARIES}
+)
+
+


Missing the absl_library for leak_check?

JonathanDCohen · 2017-10-27T15:17:51Z

absl/strings/CMakeLists.txt

+
+
+list(APPEND STRINGS_PUBLIC_HEADERS
+  "ascii_ctype.h"


This file wasn't meant to be released, and was leaked on accident. Please get rid of it here.

I have removed it

JonathanDCohen · 2017-10-27T16:08:40Z

absl/time/CMakeLists.txt

+#
+
+# test time_test
+list(APPEND TIME_TEST_SRC


Consistency in use $FOO_SRC and similar will be nice for us in the future when we generate cmake files from build files for consistency. Although if they're generated then re-spelling files isn't that bad either I suppose. It doesn't matter as long as it's consistent, build files are ideally for machines, not humans

JonathanDCohen · 2017-10-27T16:13:24Z

absl/types/CMakeLists.txt

+    ${BAD_OPTIONAL_ACCESS_PUBLIC_LIBRARIES}
+)
+
+


Please add any, optional, and span as their own libraries as well, as in the Bazel files

Added for the missing header only ones, and all exported

JonathanDCohen · 2017-10-27T16:15:54Z

absl/types/CMakeLists.txt

+
+
+
+absl_header_library(


Strings, time, etc have their own monolithic targets due to dependencies and bundling of similar types. The libraries here are all disparate, so let's not have a monolithic target here, again to match the Bazel file.

@patrikfors

- initial cmake support - downgrade cmake requirement to 2.8.12 - factorize cmake test flags / libs options - refactor test / library under helpers functions, follow bazel's style - Add fix for MSVC and Windows support ( thx @patrikfors ) - Switch to default "add_subdirectory()" usage mode - add CMake/README.md for instructions - add header-only cmake target generator - map absl target to absl:: namespace

JonathanDCohen · 2017-10-30T18:41:07Z

CMake/AbseilHelpers.cmake

+set(_ABSL_HELPERS_PATH "${CMAKE_CURRENT_LIST_DIR}")
+
+#
+# create a static library absl_based on the following variable


typo here? maybe "Create a static library absl_foo"

JonathanDCohen · 2017-10-31T16:19:36Z

Friendly ping here :) I'm working on trying to put my changes in to your PR but it's a litle slow going as I'm not as familiar with CMake

adevress · 2017-10-31T22:05:00Z

@JonathanDCohen Sorry for the slow progress, I had rather busy days recently.

All the modifications coming from your comments should be implemented now.

I also updated the README documentation appropriately.

One last thing I see for a real convenient usage would be to finalize the cctz CMake support (google/cctz#54) . Without it, it would imply that the user create its own cctz target himself.

JonathanDCohen · 2017-11-01T20:14:26Z

CMake/README.md

+    the targets  `gtest`, `gtest_main`, `gmock` and `cctz` need
+    to be declared in your project before including abseil with `add_subdirectory`.
+    You can find instructions on how to get and build these projects at these
+    URL :


URLs missing

- Update documentation - Remove type target - Remove typos - simplify target - improve 1-1 matchign with Bazel targets

Sarcasm · 2017-11-02T18:21:35Z

CMake/README.md

+  abseil directly and use the abseil targets in your project.
+
+    Note: Abseil requires CCTZ and the googletest framework. Consequently,
+    the targets  `gtest`, `gtest_main`, `gmock` and `cctz` need


In its current proposal, cctz would be better referred as cctz::cctz.

Since the cctz PR will likely undergo some change, I consider this non-blocking

Sarcasm · 2017-11-02T18:21:45Z

CMakeLists.txt

+#add_subdirectory(googletest)
+
+## check targets
+check_target(cctz)


In its current proposal, cctz would be better referred as cctz::cctz.

Since the cctz PR will likely undergo some change, I consider this non-blocking

Sarcasm · 2017-11-02T18:21:55Z

absl/time/CMakeLists.txt

+  ${TIME_PUBLIC_HEADERS}
+  ${TIME_INTERNAL_HEADERS}
+)
+set(TIME_PUBLIC_LIBRARIES absl::base absl::stacktrace absl::int128 cctz)


In its current proposal, cctz would be better referred as cctz::cctz.

Since the cctz PR will likely undergo some change, I consider this non-blocking

Sarcasm · 2017-11-02T18:22:52Z

absl/time/CMakeLists.txt

+  PUBLIC_LIBRARIES
+    ${TIME_PUBLIC_LIBRARIES}
+  PUBLIC_INCLUDE_DIRS
+    ${CCTZ_INCLUDE_DIRS}


In the current proposal of the cctz CMake support, this should not be needed.
The usage requirements for cctz::cctz should bring this automatically.

Since the cctz PR will likely undergo some change, I consider this non-blocking

JonathanDCohen · 2017-11-02T20:02:25Z

CMake/README.md

+      Here is a short CMakeLists.txt example of a possible project file
+      using abseil
+
+      cmake_minimum_required(VERSION 2.8.12)


Updated this after following the README myself, PTAL.

I'd like @Orphis to take one last look through the code since he is more knowledgeable about the ins and outs of CMake than me, but if everything goes well I suspect we'll merge this either tonight or tomorrow morning.

JonathanDCohen · 2017-11-03T15:43:38Z

Looks like this is ready to merge. It's definitely not perfect but it's undergone a lot of review and work, is reasonable as a start, and it works.

Thank you so much to EVERYBODY who has taken so much time on this. We were looking at this task with dread because none of us knew that much about CMake, and having this PR come so quickly was a wonderful surprise.

There were and are a lot of strong opinions here, and the polite, professional, courteous, and intelligent level of discourse on this PR made it a delight to work on. Thank you everybody, Abseil is lucky to have contributors like all of you :)

gennadiycivil · 2017-11-06T20:56:33Z

Thank you very much everyone!

- c3a608de577e0c278b50916ad4803549929f8f72 Merging #8 inte... by Gennadiy Civil <misterg@google.com> - d0b528cdf5843db871784c629cb4e7c5165af716 explicitly cast -1 for Span::npos by Jon Cohen <cohenjon@google.com> - 32066311a4379f1144f029aaa3740af59b1e364e Remove GUARDED_VAR and PT_GUARDED_VAR entirely. by Abseil Team <absl-team@google.com> - 3d3c69d97d15b5c6457906631054109094c083a6 Remove unneeded inline on constexpr definitions. by Alex Strelnikov <strel@google.com> - a9a8fe71f90d0b80de8e77375228a7185032636b Remove unneeded lint suppression. by Alex Strelnikov <strel@google.com> GitOrigin-RevId: c3a608de577e0c278b50916ad4803549929f8f72 Change-Id: I0897ce0b11e41f83fed8d88f18e079a15d086527

googlebot added the cla: yes label Sep 27, 2017

adevress force-pushed the master branch 2 times, most recently from c437574 to 8272265 Compare September 27, 2017 04:40

adevress force-pushed the master branch 3 times, most recently from dd90089 to 2121590 Compare September 27, 2017 20:12

adevress force-pushed the master branch 4 times, most recently from 78c3bfc to b3d0631 Compare September 28, 2017 05:45

adevress force-pushed the master branch 3 times, most recently from 02d7a3d to 3dfec79 Compare September 29, 2017 02:35

adevress force-pushed the master branch from 3dfec79 to 3a5e533 Compare September 29, 2017 19:37

KindDragon mentioned this pull request Sep 30, 2017

Port request: Abseil (Google's common library) microsoft/vcpkg#1883

Closed

solvingj referenced this pull request in microsoft/vcpkg Sep 30, 2017

[abseil] Initial commit. Highly experimental.

c957331

Orphis reviewed Oct 26, 2017

View reviewed changes

adevress force-pushed the master branch from 00617b3 to 22a025d Compare October 27, 2017 09:40

JonathanDCohen reviewed Oct 27, 2017

View reviewed changes

Orphis reviewed Oct 27, 2017

View reviewed changes

JonathanDCohen suggested changes Oct 27, 2017

View reviewed changes

adevress force-pushed the master branch from 22a025d to d5134a7 Compare October 27, 2017 16:46

Merge branch 'master' into master

200b5a7

JonathanDCohen reviewed Oct 30, 2017

View reviewed changes

adevress force-pushed the master branch from 5215f76 to 058fbb2 Compare October 31, 2017 21:47

JonathanDCohen reviewed Nov 1, 2017

View reviewed changes

Continue CMake support improvement

a3322e3

- Update documentation - Remove type target - Remove typos - simplify target - improve 1-1 matchign with Bazel targets

adevress force-pushed the master branch from 058fbb2 to a3322e3 Compare November 2, 2017 17:48

Sarcasm reviewed Nov 2, 2017

View reviewed changes

Update README.md

42a5f28

JonathanDCohen reviewed Nov 2, 2017

View reviewed changes

JonathanDCohen approved these changes Nov 3, 2017

View reviewed changes

gennadiycivil added the kokoro:run label Nov 3, 2017

kokoro-team removed the kokoro:run label Nov 3, 2017

Whitespace

2eedcf0

gennadiycivil merged commit 78e1abc into abseil:master Nov 6, 2017

		)


		add_library(${_NAME} SHARED ${__dummy_header_only_lib_file})




		absl_header_library(

Add CMake support #8

Add CMake support #8

Conversation

adevress commented Sep 27, 2017

kumagi commented Sep 27, 2017

Orphis commented Sep 27, 2017

tituswinters commented Sep 27, 2017

adevress commented Sep 27, 2017

Orphis commented Sep 27, 2017

adevress commented Sep 28, 2017

Orphis commented Sep 28, 2017

adevress commented Sep 28, 2017 • edited

Orphis commented Sep 28, 2017

adevress commented Sep 28, 2017

agauniyal commented Sep 29, 2017

tituswinters commented Sep 29, 2017

agauniyal commented Sep 29, 2017

tituswinters commented Sep 29, 2017

Orphis Oct 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanDCohen commented Oct 31, 2017

adevress commented Oct 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanDCohen commented Nov 3, 2017

gennadiycivil commented Nov 6, 2017

adevress commented Sep 28, 2017 •

edited

Orphis Oct 26, 2017 •

edited

adevress commented Oct 31, 2017 •

edited