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.

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

Modern cmake #40

Merged
merged 30 commits into from
Nov 24, 2016
Merged

Modern cmake #40

merged 30 commits into from
Nov 24, 2016

Conversation

simogasp
Copy link
Member

@simogasp simogasp commented Nov 7, 2016

This PR upgrades the building system to a modern version of cmake. In particular:

  • Only a single library is built, libCCTag, whether CUDA is enabled or not
  • The targets have now the correct visibility wrt the dependencies (PUBLIC, PRIVATE, INTERFACE)
  • A CCTagConfig.cmake is generated for the installation allowing to better import the built library as third party in other cmake projects
  • A cctag_config.hpp file is generated containing the definitions used to compile the library. Note that said definition are automatically imported by CCTagConfig.cmake in the cmake host project.
    • for now it is not included in any of the source files as cmake add already the definitions.
  • In the same spirit, a version.hpp file is generate containing the definition for the code version
  • USE_CUDA has been replaced by WITH_CUDA as cmake option to compile with cuda (it simplifies the generation of the config file)

connected to #38

While at it, this PR also add CUDA to Continuous Integration on Travis (connected to #42)

check the error case first and fail accordingly
fix #38
* sorry not an atomic commit, too many changes involved
* Now only one library is generate, the same whether it is with or
without CUDA
* A CCTagConfig.cmake is generate to properly import the library
* The applications are now a separate project, thus it can be compiled
as a bundle or with CCTag as third party (so we can test)
* There is a duplicate FindTBB.cmake in order to do the previous point
Conflicts:
	CMakeLists.txt
#38
Now the behaviour is that if it cannot find cuda it errors instead of
falling back to CPU
@simogasp simogasp force-pushed the modernCmake branch 3 times, most recently from febc741 to 49bcd70 Compare November 7, 2016 19:54
@simogasp simogasp added this to the 2016_11 milestone Nov 7, 2016
@@ -4,7 +4,18 @@ cmake_minimum_required(VERSION 3.4)

include(ExternalProject)

project( CCTag )
project(CCTag VERSION 1.0.0 LANGUAGES C CXX)
Copy link
Member Author

Choose a reason for hiding this comment

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

@fabiencastan @zvrba @griwodz @lcalvet
Two things:

  • if you agree we can express the lib version with project(), this automatically sets all the variables to recover the major, minor, patch numbers (see https://cmake.org/cmake/help/v3.0/command/project.html)
  • More importantly I set here 1.0.0 as a first version of the library. It's arbitrary but it seemed to me a good number to start with :-). Anyway, just decide a number if you prefer another, the important thing is that what we decide here will be the minimal version supported in all the other libs, openMVG and other LABO libs.

Copy link
Member

Choose a reason for hiding this comment

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

It's important to have the version in the code, so users of the library can do some #if CCTAG_VERSION_MAJOR > 1.
Usually, I create a "version.hpp" and parse the values of major, minor, micro in cmake from this header.

Copy link
Member Author

@simogasp simogasp Nov 9, 2016

Choose a reason for hiding this comment

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

With this you can do the other way around, cmake can generate the version file with a template like:

[version.hpp.in]
#pragma once

#define CCTAG_VERSION_MAJOR @PROJECT_VERSION_MAJOR@
#define CCTAG_VERSION_MINOR @PROJECT_VERSION_MINOR@
#define CCTAG_VERSION_PATCH @PROJECT_VERSION_PATCH@

#define CCTAG_VERSION @PROJECT_VERSION@
//etc

which generates (and installs)

[version.hpp]
#pragma once

#define CCTAG_VERSION_MAJOR 1
#define CCTAG_VERSION_MINOR 0
#define CCTAG_VERSION_PATCH 0

#define CCTAG_VERSION 1.0.0

that can be included by the headers of the lib (if necessary) or by 3rd party code (the same way i did the config.hpp)

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually did it, i can push it

Copy link
Member Author

Choose a reason for hiding this comment

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

The only drawback of this approach (it seems to me) is that if for some reason, god forbid, one day we need to switch to a different building system it may be possible that we cannot replicate this and to have to add version.hpp to the source code.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the code is no more autonomous, just for that. So your IDE or static analysis tools will make an error on #include "cctag/version.hpp" as this file doesn't exist in your source tree.
I prefer to keep the important information in the code instead of cmake. But it's a detail, both works.

Copy link
Member Author

@simogasp simogasp Nov 9, 2016

Choose a reason for hiding this comment

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

So your IDE tools will make an error

Nitpicking, but true only for the IDEs that do not support cmake, the file is generated during the cmake configuration and it is enough to include it among the include directories of the targets with e.g. $<BUILD_INTERFACE:<the place where generated files are>> (so that it only works for in-tree build), at that point the IDE has no problem at retrieving it.

Copy link
Member Author

Choose a reason for hiding this comment

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

or we can use git tags to determine the version
http://brianmilco.blogspot.fr/2012/11/cmake-automatically-use-git-tags-as.html

:-D

```cmake
# Find the package from the CCTagConfig.cmake
# in <prefix>/lib/cmake/CCTag/. Under the namespace CCTag::
# it exposes the target popsift that allows you to compile
Copy link
Member

Choose a reason for hiding this comment

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

popsift?

Copy link
Member Author

Choose a reason for hiding this comment

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

oopsy

### Using CCTag as third party

When you install CCTag a file `CCTagConfig.cmake` is installed in `$CCTAG_INSTALL/lib/cmake/CCTag/` that allows you to import the library in your CMake project.
In your `CMakeLists.txt` file just add the dependency
Copy link
Member

Choose a reason for hiding this comment

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

add the dependency like that:

# set(BUILD_SHARED_LIBS ON)

if(NOT COMPILE_STATIC_CCTAG_LIBRARY)
message( FATAL_ERROR "We must link CCTag library static with CUDA" )
Copy link
Member

@fabiencastan fabiencastan Nov 21, 2016

Choose a reason for hiding this comment

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

To use CUDA, CCTag library needs to be build in static.
To disable CUDA and generate a dynamic library use: -DWITH_CUDA=OFF.

@fabiencastan fabiencastan changed the title [WIP] Modern cmake Modern cmake Nov 21, 2016
@fabiencastan
Copy link
Member

fabiencastan commented Nov 22, 2016

To be consistent with openMVG, but also ceres and opencv, it would be better to install config files in "share/CCTag/".

Both seems to be recognized by cmake as explained here:

CMake constructs a set of possible installation prefixes for the package. Under each prefix several directories are searched for a configuration file. The tables below show the directories searched. Each entry is meant for installation trees following Windows (W), UNIX (U), or Apple (A) conventions:

<prefix>/                                                       (W)
<prefix>/(cmake|CMake)/                                         (W)
<prefix>/<name>*/                                               (W)
<prefix>/<name>*/(cmake|CMake)/                                 (W)
<prefix>/(lib/<arch>|lib|share)/cmake/<name>*/                  (U) <<<<
<prefix>/(lib/<arch>|lib|share)/<name>*/                        (U)
<prefix>/(lib/<arch>|lib|share)/<name>*/(cmake|CMake)/          (U)
<prefix>/<name>*/(lib/<arch>|lib|share)/cmake/<name>*/          (W/U)
<prefix>/<name>*/(lib/<arch>|lib|share)/<name>*/                (W/U)
<prefix>/<name>*/(lib/<arch>|lib|share)/<name>*/(cmake|CMake)/  (W/U)

@simogasp
Copy link
Member Author

I just followed this on CMake tutorial https://cmake.org/cmake/help/v3.0/manual/cmake-packages.7.html#package-version-file
Alembic uses the same, for example.

@fabiencastan fabiencastan merged commit 9cee7d8 into develop Nov 24, 2016
@fabiencastan fabiencastan deleted the modernCmake branch November 24, 2016 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants