CMake Update #292

Mizux · 2018-01-31T13:14:33Z

Few patch to:

Ease the use of glog as subproject (i.e. adding alias glog::glog)
Better manage gflags dependencies
Rework VERSION management using project() provided variables
cf https://cmake.org/cmake/help/v3.0/command/project.html

googlebot · 2018-01-31T13:14:39Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Mizux · 2018-01-31T13:17:34Z

I signed it! (since I'm a Google Temp (red card))

andyleejordan · 2018-01-31T19:08:53Z

CMakeLists.txt

+    if (TARGET gflags::gflags)
+      set(GFLAGS_TARGET gflags::gflags)
+    elseif(TARGET gflags)
+      set(GFLAGS_TARGET gflags)


Just curious, what is this logic doing? Why is the target sometimes gflags::gflags and other times just gflags, is this a side effect of version differences for the glags package maybe? I'm not too familiar with it.

I guess, this is a workaround for gflags not providing an alias target when used as a subproject.

@Mizux Wouldn't it make sense to let the gflags folks add this alias instead of making this work using ugly hacks?

Hi,

Curently gflags-[target|config].cmake provide gflags imported target (and gflags alias if using as subproject).

There is no official findgflags.cmake module AFAIK

I'm currently sending a PR247 in gflags.git to use gflags::gflags instead (and providing alias as well when using as subproject)

gRPC seems to already to look for gflags::gflags

We have glog::glog protobuf::libprotobuf gRPC::grpc ...

CMake preconise the use of namespace

When providing imported targets, these should be namespaced (hence the Foo:: prefix);
CMake will recognize that values passed to target_link_libraries() that contain :: in their name are supposed to be imported targets (rather than just library names), and will produce appropriate diagnostic messages if that target does not exist (see policy CMP0028).

src: https://cmake.org/cmake/help/latest/manual/cmake-developer.7.html#a-sample-find-module (at ~bottom, search for namespaced)

So all in all we have gflags currently but hope to have gflags::gflags soon -> here it is a workaround to be able to work during the transition since find_package don't provide a way to deal with this otherwise...

I would prefer not adding the alias hack and wait until the gflags::gflags alias is added. No need for temporary workarounds.

LGTM remove the commit for this PR

andyleejordan · 2018-01-31T19:14:02Z

CMakeLists.txt

@@ -8,23 +31,14 @@ if (POLICY CMP0063)
  cmake_policy (SET CMP0063 NEW)
 endif (POLICY CMP0063)

-project (glog)


I am 👍 on this, and checked that our minimum CMake version supports this syntax in project (I think it must have been older 2.x versions that didn't).

~~I'm not. The policies must be set before the project call, not after it. Please revert.~~ Nevermind. project is in the correct place.

project(VERSION) came in CMake 3.0 (i.e. not available in 2.8.12)
Also Trusty cmake package is 2.8.12 BUT ubuntu provides 3.5.1 as cmake3 package for trusty (and travis has trusty with 3.8 or 3.9 BTW).
So it should be reasonable to think everyone have at least CMake 3.0 with VERSION support...

And we're using cmake_minimum_required (VERSION 3.0), so it'll be a very explicit error if 2.x is used. It is not worth attempting to support CMake 2, it's possibly worse than than the Python 2 and 3 split IMHO.

sergiud · 2018-01-31T22:31:11Z

CMakeLists.txt

-set_target_properties (glog PROPERTIES VERSION ${GLOG_MAJOR_VERSION})
-set_target_properties (glog PROPERTIES SOVERSION ${GLOG_VERSION})
+set_target_properties (glog PROPERTIES VERSION ${PROJECT_VERSION})
+set_target_properties (glog PROPERTIES SOVERSION ${PROJECT_VERSION_MAJOR})


I'm not sure this is correct. Does this mean there will be only a libglog.so.0 but not a libglog.so.0.3.5?

No there will be 3 files: (-> means symbolic link aka ln -s ...)
libglog.so -> libglog.so.0 (since SO_VERSION which is the API compatibility is only major)
libglog.so.0 -> libglog.so.0.3.5 (so the "API" point on your build version)
libglog.so.0.3.5 (which is the "real" library)

And it seems that target_link_library will point on the soname aka lbglog.so.0
(so you should be able to install different major version on linux system etc..)

As I said in the commit message, there is also an inversion between VERSION and SOVERSION usage
(easy to spot with make install DESTDIR=install && ls -l install/use/local/lib)
doc: https://cmake.org/cmake/help/latest/prop_tgt/VERSION.html#prop_tgt:VERSION

Hope it's clear know 😅

side note: if you wrap your code in a python "setup.py" bdist will resolve the symlink so you just have to depend on $<TARGET_SONAME_FILE:glog>
cf https://github.com/google/or-tools/blob/mizux/dev/cmake/python.cmake#L110

sergiud · 2018-01-31T22:33:36Z

CMakeLists.txt

@@ -8,23 +31,14 @@ if (POLICY CMP0063)
  cmake_policy (SET CMP0063 NEW)
 endif (POLICY CMP0063)

-project (glog)


~~I'm not. The policies must be set before the project call, not after it. Please revert.~~ Nevermind. project is in the correct place.

Mizux · 2018-02-01T19:09:46Z

I just remove the gflags::gflags / gflags management.
keep it on a dev branch on my fork when needed...

Mizux · 2018-02-02T16:21:23Z

@googlebot rescan

googlebot · 2018-02-02T16:21:26Z

CLAs look good, thanks!

sergiud · 2018-02-02T20:56:12Z

CMakeLists.txt

+##   add_subdirectory(glog)
+##
+##   add_executable(foo src/foo.cc)
+##   target_link_libraries(foo glog::glog)


I don't believe this is a good place to document such specific stuff. I would suggest moving this part either into the wiki or cmake/INSTALL.md.

Ok, I'll move it...

ps: I just put it here as gflags CMakelists.txt did

Useful when using glog as subproject. Add an explanation at top like in gflags.git CMakeLists.txt

CTest module already provides it

- Use of Project version properties instead of custom variables - fix missmatch between VERSION (build version) and SOVERSION (API version) src: https://cmake.org/cmake/help/latest/prop_tgt/VERSION.html#prop_tgt:VERSION

Mizux · 2018-02-06T09:40:15Z

Ok I moved the doc to INSTALL.md and incorparate this patch with the glog::glog alias commit.
You can see the rendering there https://github.com/Mizux/glog/blob/master/cmake/INSTALL.md

Don't hesitate to comment / modify / whatever...

sergiud · 2018-02-10T13:34:28Z

LGTM.

CMake Update

googlebot added the cla: no label Jan 31, 2018

andyleejordan reviewed Jan 31, 2018

View reviewed changes

sergiud reviewed Jan 31, 2018

View reviewed changes

sergiud requested changes Jan 31, 2018

View reviewed changes

Mizux force-pushed the master branch from 2b9f8a5 to cbdeeea Compare February 1, 2018 19:07

googlebot added cla: yes and removed cla: no labels Feb 2, 2018

sergiud requested changes Feb 2, 2018

View reviewed changes

Mizux added 3 commits February 6, 2018 10:37

Provide glog::glog ALIAs

cbfd86a

Useful when using glog as subproject. Add an explanation at top like in gflags.git CMakeLists.txt

remove uneeded enable_testing()

4ef88d5

CTest module already provides it

Rework CMake glog VERSION management.

6b6e38a

- Use of Project version properties instead of custom variables - fix missmatch between VERSION (build version) and SOVERSION (API version) src: https://cmake.org/cmake/help/latest/prop_tgt/VERSION.html#prop_tgt:VERSION

Mizux force-pushed the master branch from cbdeeea to 6b6e38a Compare February 6, 2018 09:37

sergiud merged commit 4c4631c into google:master Feb 10, 2018

durswd pushed a commit to durswd/glog that referenced this pull request Sep 2, 2019

Merge pull request google#292 from Mizux/master

e17185d

CMake Update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake Update #292

CMake Update #292

Mizux commented Jan 31, 2018

googlebot commented Jan 31, 2018

Mizux commented Jan 31, 2018

andyleejordan Jan 31, 2018

sergiud Jan 31, 2018

Mizux Feb 1, 2018 •

edited

sergiud Feb 1, 2018

Mizux Feb 1, 2018

andyleejordan Jan 31, 2018

sergiud Jan 31, 2018 •

edited

Mizux Feb 1, 2018

andyleejordan Feb 1, 2018

sergiud Jan 31, 2018

Mizux Feb 1, 2018 •

edited

sergiud Jan 31, 2018 •

edited

Mizux commented Feb 1, 2018

Mizux commented Feb 2, 2018

googlebot commented Feb 2, 2018

sergiud Feb 2, 2018 •

edited

Mizux Feb 5, 2018

Mizux commented Feb 6, 2018 •

edited

sergiud commented Feb 10, 2018

CMake Update #292

CMake Update #292

Conversation

Mizux commented Jan 31, 2018

googlebot commented Jan 31, 2018

Mizux commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mizux Feb 1, 2018 • 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

sergiud Jan 31, 2018 • 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

Mizux Feb 1, 2018 • edited

Choose a reason for hiding this comment

sergiud Jan 31, 2018 • edited

Choose a reason for hiding this comment

Mizux commented Feb 1, 2018

Mizux commented Feb 2, 2018

googlebot commented Feb 2, 2018

sergiud Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mizux commented Feb 6, 2018 • edited

sergiud commented Feb 10, 2018

Mizux Feb 1, 2018 •

edited

sergiud Jan 31, 2018 •

edited

Mizux Feb 1, 2018 •

edited

sergiud Jan 31, 2018 •

edited

sergiud Feb 2, 2018 •

edited

Mizux commented Feb 6, 2018 •

edited