Merge lp://staging/~thomas-voss/xorg-gtest/cmake into lp://staging/~oif-team/xorg-gtest/trunk

Proposed by Thomas Voß
Status: Rejected
Rejected by: Chase Douglas
Proposed branch: lp://staging/~thomas-voss/xorg-gtest/cmake
Merge into: lp://staging/~oif-team/xorg-gtest/trunk
Diff against target: 209 lines (+195/-0)
3 files modified
CMakeLists.txt (+152/-0)
doc/CMakeLists.txt (+25/-0)
examples/CMakeLists.txt (+18/-0)
To merge this branch: bzr merge lp://staging/~thomas-voss/xorg-gtest/cmake
Reviewer Review Type Date Requested Status
Chase Douglas (community) Needs Fixing
Thomas Voß (community) Needs Resubmitting
Review via email: mp+86201@code.staging.launchpad.net

Description of the change

Added a cmake setup basically replicating the autotools setup. Adds support for inclusion of xorg-gtest with the help of cmake's add_subdirectory in accordance with Google Test's recommendation of bundling the test framework with the actual framework.

To post a comment you must log in.
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

Looks fine. Just a few niggles.

I don't like CMake's default style of writing function names in UPPERCASE. It just looks like SHOUTING.

DUMMY_CONF_PATH is not, in my mind, a path since it leads all the way to a file. A path variable to me points to a directory (like the path variable in a shell). I would call this DUMMY_XORG_CONF_FILE to make it more self-documenting.

11. By Thomas Voß

Converted CMake directives to lower case and adjusted naming of cached-variable DUMMY_CONF_PATH -> DUMMY_XORG_CONF_FILE.

Revision history for this message
Thomas Voß (thomas-voss) wrote :

> Looks fine. Just a few niggles.
>

Thanks :-)

> I don't like CMake's default style of writing function names in UPPERCASE. It
> just looks like SHOUTING.
>

Done. Converted to lower case.

> DUMMY_CONF_PATH is not, in my mind, a path since it leads all the way to a
> file. A path variable to me points to a directory (like the path variable in a
> shell). I would call this DUMMY_XORG_CONF_FILE to make it more self-
> documenting.

Done.

review: Needs Resubmitting
Revision history for this message
Chase Douglas (chasedouglas) wrote :

After thinking about this some, and fixing stuff in the branch, I realized that this is a bad idea :). It will be a pain to keep the two updated. I am ok with merging in cmake and autotools version at the beginning if they are identical in build output and such. However, we need to decide on one by the time we make the initial release. If this is going to be an upstream xorg project, it will probably be autotools.

* All the location variables are unused and should be removed.

* The main library is named xorg-gtest-main when it should be xorg-gtest_main.

* The VERSION for the libraries should not be used. It does not work properly when mixed with SOVERSION, and we should use SOVERSION.

* The SOVERSION for the libraries uses the project version, which isn't quite correct. This needs to be corrected in the autotools version too. The SOVERSION should be 0:0:0 to start out with. See http://www.gnu.org/software/libtool/manual/libtool.html#Libtool-versioning for details.

* There are no executables, so we do not need to specify a RUNTIME install location.

* The autotools version builds both static and shared libraries, this should do the same.

* The autotools version does not install man pages. Either we need to change Makefile.am to do that, or we need to not install them here. I vote for installing them.

* The autotools version does not build docs through the default make target. Either we need to change doc/Makefile.am to do that, or we need to not do that here. I vote for not building them. This is consistent with current utouch practices.

* The autotools version should have a "doc" target to match the cmake version.

* The doc build needs to be done in the build directory. I believe I managed to make this work in utouch-qml. See there for an example

* xorg-gtest-example does not match the autotools version xorg_gtest_example. I prefer xorg-gtest-example, so we should change the autotools name.

* xorg-gtest-example links in gtest_main instead of xorg-gtest_main.

* make dist needs to include the CMakeList.txt files.

I pushed a bunch of changes to the above to lp:~chasedouglas/xorg-gtest/cmake. It is missing:

* autotools needs a "doc" target
* cmake fails to install if the "doc" target was not invoked
* The doc build is still in the source directory
* make dist needs to include the CMakeList.txt files

Please merge the branch in if you agree with the changes. I've run out of time in my day to propose them more formally :(.

Thanks!

review: Needs Fixing
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

The link you posted explains how Libtool's versioning works, not how Linux library versioning works. That one is explained e.g. here: http://tldp.org/HOWTO/Program-Library-HOWTO/shared-libraries.html#AEN135

I once had to fix my library versioning for an app. A Debian packager worked with me to do it right and what we came up with was the following. First you set up the versions:

set(CF_VERSION_MAJOR "1")
set(CF_VERSION_MINOR "1")
set(CF_VERSION_PATCH "0")
set(CF_VERSION "${CF_VERSION_MAJOR}.${CF_VERSION_MINOR}.${CF_VERSION_PATCH}")
# Increment this manually whenever breaking ABI.
set(ABI_VERSION 0)

And then apply it to libraries with

set_target_properties(${name} PROPERTIES VERSION ${CF_VERSION} SOVERSION ${ABI_VERSION})

For the whole code, see:

http://bazaar.launchpad.net/~jpakkane/cuneiform-linux/trunk/files

Revision history for this message
Chase Douglas (chasedouglas) wrote :

That doesn't work right. In the case of xorg-gtest, the version is 0.1.0, and the so version is 0.0.0. This leads to:

libxorg-gtest.so -> libxorg-gtest.so.0
libxorg-gtest.so.0 -> libxorg-gtest.so.0.1.0
libxorg-gtest.so.0.1.0

On the surface this looks reasonable, but it doesn't conform to the library so versioning. Let's say we bump the package version to 0.1.1 with a bug fix. The same files should be created because we haven't changed the API or ABI. However, it would produce libxorg-gtest.so.0.1.1. That is an invalid so version because it supports the current ABI (0), version (1), and age (1), which implies it also supports ABI (-1).

An example of how the three numbers should work is with utouch-frame. I didn't actually do this when bumping it to version 2, but I might go back now that I've read more and understand it. We went from ABI version 1.1 to 1.2 to 2.0. We should have had the following progression:

Current: 1, Revision: 0, Age: 0 # Should have started at Current = 0, but oh well.
Current: 1, Revision: 1, Age: 0 # Bug fix
Current: 2, Revision: 0, Age: 1 # Add ABI 1.2
Current: 2, Revision: 1, Age: 1 # Bug fix
Current: 2, Revision: 2, Age: 1 # Bug fix
Current: 3, Revision: 0, Age: 2 # Add ABI 2.0

Now, when we get rid of all the dependencies on the utouch-frame v1 API, we'll drop the v1 sources and bump this to:

Current: 3, Revision: 0, Age: 0 # Drop ABI 1.x

Revision history for this message
Stephen M. Webb (bregma) wrote :

> That is an invalid so version because it supports the current ABI (0), version (1), and age (1), which implies it also supports ABI (-1).

That's how libtool supports its SO versioning scheme. It is one particular way of versioning SO libraries.

The numbers after the .so in the shared object filename can have any meaning whatsoever. Often, they're left as 0.0.0 or left off entirely. The assumed meaning is major/minor/micro versions of the library. It would be foolish to depart too much from that convention, or the convention that the first number correspond to the SONAME of the library on an ELF system.

The file naming scheme used by libtool is a good one: it solves a lot of problems encountered in the wild when using alternative schemes and I would recommend it for non-libtool systems like cmake, if that's possible. If cmake needs to be adjusted to use libtool-style file name versioning on platforms that support symbolic links, please make sure it also properly sets the SONAME of the shared object on platforms with ELF binaries and dylib versions on platforms with MACH-O binaries.

Revision history for this message
Chase Douglas (chasedouglas) wrote :

We decided to keep this branch around for future reference, but leave xorg-test using autotools. It's too much of a hassle keeping them both up to date, and upstream X.org uses autotools for everything.

Unmerged revisions

11. By Thomas Voß

Converted CMake directives to lower case and adjusted naming of cached-variable DUMMY_CONF_PATH -> DUMMY_XORG_CONF_FILE.

10. By Thomas Voß

ADDED: Installation of conf/dummy.conf.

9. By Thomas Voß

Added a cmake setup to support add_subdirectory scenarios.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches