Modernize CMake #1524

phcerdan · 2020-10-15T13:22:45Z

Avoid global include_directories, link_libraries etc. Use
instead the target_ commands.
Create DGtal library in the top directory, allowing to be targeted in
the other CMake files.
Use lower case for CMake commands, i.e. SET to set etc.
Remove naming the condition in else, endif, and endforeach: if(x), else(x), endif(x)
Add function DGtal_add_test(test_file) to avoid repetition when
configuring tests. It accepts a dummy optional parameter, for example
ONLY_ADD_EXECUTABLE that will create the executable, link to DGtal,
but avoid the add_test command. Used mainly when benchmarking is ON.

Checklist

Unit-test of your feature with Catch.
Doxygen documentation of the code completed (classes, methods, types, members...)
Documentation module page added or updated.
New entry in the ChangeLog.md added.
No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
All continuous integration tests pass (Travis & appveyor)

dcoeurjo · 2020-11-09T08:17:35Z

I am now a great fan of "Fact_content" (or any higher level facade like https://github.com/TheLartians/CPM.cmake).
What do think to have the following deps "fetched" at cmake by default:

Eigen
GMP/GMPXX
CGAL

(with flag to allow to use the installed deps)

@JacquesOlivierLachaud ? @rolanddenis

rolanddenis · 2020-11-09T08:37:21Z

Since CGAL is now header only, it is a good idea to simplify DGtal installation! If we replace boost/program_options by CL11 in DGtal, I suppose Boost should also be header only?

JacquesOlivierLachaud · 2020-11-09T08:44:50Z

I am not a great expert on configuration tools. Everything that makes the life of future developers/users easier is good for me.
If we have to fetch by default some packages, the three you list is a good option (Eigen very useful but pulls a lot of other packages, gmp/gmpxx is a small dependency, CGAL is sometimes useful and header only).

dcoeurjo · 2020-11-09T09:04:16Z

In DGtal, boost is already header only (only libs have been considered in DGtalTools but not true anymore I guess). It could also be "fetched" at cmake step but my only concern would be the size of the working copy (minor issue, though...).

Maybe the "make install" step is also something that could introduce bugs..

phcerdan · 2020-11-09T09:06:48Z

FetchContent uses add_subdirectory, so you end owning the third-party library in your project, i.e gmp.so would be at the same level than DGtal.so. If the CMake files of the third-party are not pretty modern, you might face global changes on CMAKE_CXX_FLAGS, or if missing alias targets, you might find incompatibilities of library names between find_package and add_subdirectory.

An alternative that from my experience works great is to have a dependencies subproject inside DGtal (but completely independent), that uses FetchContent to feed all the dependencies (with options). It will be built in its own folder, for example build-dgtal-dependencies.

With this approach, DGtal won't be changed, it will still expect Eigen_DIR, CGAL_DIR and use find_package. Then, a wrapper can be created, such as: DEPENDENCIES_DIR=/path/build-dgtal-dependencies to where the dependencies subproject was built, that provide all the dependencies _DIR with just one option.

The dependencies approach is like the super-build approach, but you don't have to config all the dependencies every time.

phcerdan · 2020-11-09T09:10:02Z

Since CGAL is now header only, it is a good idea to simplify DGtal installation! If we replace boost/program_options by CL11 in DGtal, I suppose Boost should also be header only?

I think boost-graph can enjoy being linked (reduces compile time).

phcerdan · 2020-11-09T12:25:33Z

In any case, this modernization is about removing global commands for target specific commands. Remove upper case, and modernize DGtalConfig.cmake file accordingly.

dcoeurjo · 2020-11-12T09:15:50Z

Additional things that should be in the Fetch_content stuff:

Catch2 (for the tests)
CLI11 (maybe DGtalTools only)
LibBoard? (though the main project seems to be dead)

dcoeurjo · 2020-12-01T12:46:14Z

I've just tried a first Fetch_content with Catch2...

cmake/FetchExternalDeps.cmake

- Avoid global `include_directories`, `link_libraries` etc. Use instead the `target_` commands. - Create DGtal library in the top directory, allowing to be targeted in the other CMake files. - Use lower case for CMake commands, i.e. `SET` to `set` etc. - Remove naming the condition in `else`, `endif`, and `endforeach`: `if(x), else(x), endif(x)` - Add function `DGtal_add_test(test_file)` to avoid repetition when configuring tests. It accepts a dummy optional parameter, for example `ONLY_ADD_EXECUTABLE` that will create the executable, link to DGtal, but avoid the `add_test` command. Used mainly when benchmarking is ON.

No need to prepend CMAKE_INSTALL_PREFIX

Instead of `INSTALL_X_DIR`. Where X is one of LIB, BIN, INCLUDE, DATA. `INSTALL_X_DIR` will be set based on the RELATIVE ones. This allow to have access to the relative paths to make DGtal re-allocatable. Also allow to change the docs with INSTALL_DOC_DIR_RELATIVE This also solves the existing problem where the docs installation folder does not change when changing `CMAKE_INSTALL_PREFIX` after the first configuration (because it was a CACHE variable)

Use find_dependency (find_package) for all enabled dependencies. This forces consumers of DGtal to provide `FOO_DIR` where FOO is a DGtal dependency. To ease this transition the option HINTS is provided with the current value of `FOO_DIR` computed when building DGtal. This will ease the transition in development environments, for example along DGtalTools. If DGtal is deployed, i.e. re-allocated, the HINTS are meaningless, but hopefully harmless, they just won't help in this case.

Use target_compile_definitions instead.

Provide HINTS to DGtalConfig.cmake. This allow third-parties (i.e DGtalTools) to not having to provide FOO_DIR, where FOO is a DGtal dependency, to their project, but to reuse the FOO_DIR provided in DGtal. This option should be disabled for deployment or any other re-allocation situation.

…s.cmake To avoid using add_definitions, we need DGtal target to exists. So these options are moved to a CMake file included after DGtal target is created.

Also fix OPENMP to OpenMP

In the build tree into: `PROJECT_BINARY_DIR/Modules` In install tree into `DGTAL_CMAKE_DIR_INSTALL/Modules` Also change find_dependency to find_package for the FindFoo.cmake packages. It seems that find_dependency prioritizes Config files and ignore Find modules.

phcerdan · 2020-12-08T11:05:49Z

@dcoeurjo I will rebase on top of master instead of: Merge branch master into this branch.

git checkout master; git pull;
git checkout modernize_cmake;
git rebase master

Are you working on something on this branch? The head of this branch will be invalidated

dcoeurjo · 2020-12-08T11:10:53Z

please go ahead, I'll resest my working copy if I need.

No need to have two `add_library` commands based on `BUILD_SHARED_LIBS`. `SHARED` will be added automatically if `BUILD_SHARED_LIBS` is `ON` and `STATIC` if `OFF`.

dcoeurjo · 2020-12-27T12:59:16Z

Can this branch be merged ? @phcerdan anything that is still WIP here?
We may consider other FecthContent in the future but maybe in a separate PR..

phcerdan · 2020-12-27T13:11:04Z

Can this branch be merged ?

I think yes.

We may consider other FecthContent in the future but maybe in a separate PR..

Sounds good!

dcoeurjo

Excellent, I just have a minor question.

cmake/DGtalConfigGenerator.cmake

cmake/FindCOIN3D.cmake

dcoeurjo · 2020-12-27T13:43:24Z

Ok SoQT and COin3D can be removed.. I'll do it

dcoeurjo

Changelog entry?

dcoeurjo

All good... thx

phcerdan force-pushed the modernize_cmake branch 7 times, most recently from ae581dd to 757a5b9 Compare October 19, 2020 10:32

phcerdan commented Dec 1, 2020

View reviewed changes

cmake/FetchExternalDeps.cmake Show resolved Hide resolved

phcerdan and others added 13 commits December 8, 2020 11:59

Fix removal of -frounding-math (clang) when CMAKE_CXX_FLAGS is empty

17bc025

Fix INSTALL_INCLUDE_DIR is already absolute

7f37733

No need to prepend CMAKE_INSTALL_PREFIX

Change add_definitions still present in Common.cmake

196ce2c

Use target_compile_definitions instead.

Move debug and verbose options from Common.cmake to DevelopmentOption…

dc620a8

…s.cmake To avoid using add_definitions, we need DGtal target to exists. So these options are moved to a CMake file included after DGtal target is created.

Fix check of Foo_DIR for HINTS

a416483

Also fix OPENMP to OpenMP

Adding Catch2 as external deps using Fetch_Content

59303e5

Adding Catch2 as external deps using Fetch_Content (missing file)

74ea5f6

Adding Catch2 as external deps using Fetch_Content (missing file)

da59a04

dcoeurjo added 10 commits December 8, 2020 11:59

Adding Catch2 as external deps using Fetch_Content (missing file)

07e3abc

Comment added

85e08e7

Download Catch2 only when testing is on

242c032

Download Catch2 only when testing is on (typo)

8ca64f5

Bumping mandatory cmake version to 3.11

66c4af5

FetchContent_MakeAvailable for cmake >3.11 but <3.14+

d4f52a1

FetchContent_MakeAvailable for cmake >3.11 but <3.14+ (typo)

32ab055

lower case in cmake fetch_content

3836f5d

OSX image with xcode10.1

a201cbb

Changelog entry for Catch2

c86b448

CMake: Simplify add_library(DGtal) into one command

309b81b

No need to have two `add_library` commands based on `BUILD_SHARED_LIBS`. `SHARED` will be added automatically if `BUILD_SHARED_LIBS` is `ON` and `STATIC` if `OFF`.

phcerdan force-pushed the modernize_cmake branch from 2753977 to 309b81b Compare December 8, 2020 11:16

Merge branch 'master' into modernize_cmake

d84ba9d

phcerdan mentioned this pull request Dec 27, 2020

Add python wrappings #1528

Merged

27 tasks

dcoeurjo approved these changes Dec 27, 2020

View reviewed changes

cmake/DGtalConfigGenerator.cmake Show resolved Hide resolved

cmake/FindCOIN3D.cmake Show resolved Hide resolved

dcoeurjo added 2 commits December 27, 2020 14:45

Delete FindCOIN3D.cmake

1e4cba0

Delete FindSOQT.cmake

9febcfb

dcoeurjo requested changes Dec 27, 2020

View reviewed changes

Update ChangeLog for CMake branch

d376074

phcerdan requested a review from dcoeurjo December 27, 2020 18:31

dcoeurjo approved these changes Dec 27, 2020

View reviewed changes

dcoeurjo approved these changes Dec 28, 2020

View reviewed changes

dcoeurjo merged commit f36dbce into DGtal-team:master Dec 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize CMake #1524

Modernize CMake #1524

phcerdan commented Oct 15, 2020 •

edited

Loading

dcoeurjo commented Nov 9, 2020

rolanddenis commented Nov 9, 2020

JacquesOlivierLachaud commented Nov 9, 2020

dcoeurjo commented Nov 9, 2020

phcerdan commented Nov 9, 2020

phcerdan commented Nov 9, 2020

phcerdan commented Nov 9, 2020

dcoeurjo commented Nov 12, 2020

dcoeurjo commented Dec 1, 2020

phcerdan commented Dec 8, 2020 •

edited

Loading

dcoeurjo commented Dec 8, 2020

dcoeurjo commented Dec 27, 2020

phcerdan commented Dec 27, 2020

dcoeurjo left a comment

dcoeurjo commented Dec 27, 2020

dcoeurjo left a comment

dcoeurjo left a comment

Modernize CMake #1524

Modernize CMake #1524

Conversation

phcerdan commented Oct 15, 2020 • edited Loading

Checklist

dcoeurjo commented Nov 9, 2020

rolanddenis commented Nov 9, 2020

JacquesOlivierLachaud commented Nov 9, 2020

dcoeurjo commented Nov 9, 2020

phcerdan commented Nov 9, 2020

phcerdan commented Nov 9, 2020

phcerdan commented Nov 9, 2020

dcoeurjo commented Nov 12, 2020

dcoeurjo commented Dec 1, 2020

phcerdan commented Dec 8, 2020 • edited Loading

dcoeurjo commented Dec 8, 2020

dcoeurjo commented Dec 27, 2020

phcerdan commented Dec 27, 2020

dcoeurjo left a comment

Choose a reason for hiding this comment

dcoeurjo commented Dec 27, 2020

dcoeurjo left a comment

Choose a reason for hiding this comment

dcoeurjo left a comment

Choose a reason for hiding this comment

phcerdan commented Oct 15, 2020 •

edited

Loading

phcerdan commented Dec 8, 2020 •

edited

Loading