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

Move to modern CMake version and techniques. #15

Closed
FunMiles opened this issue Jul 16, 2020 · 5 comments
Closed

Move to modern CMake version and techniques. #15

FunMiles opened this issue Jul 16, 2020 · 5 comments

Comments

@FunMiles
Copy link

FunMiles commented Jul 16, 2020

CMake 2.6.3 is rather antiquated, uses non-scalable constructs and cannot find OpenMP on Mac OS on its own for example.
CMake >= 3.12 is capable of finding OpenMP on Mac OS and can support the modern way of handling dependencies.
For basic improvements to support finding OpenMP and using OpenMP the modern way, the lines below are a brief outline of changes needed:

cmake_minimum_required(VERSION 3.12)
project(libraw CXX)
[...]
add_library(raw)
# It is preferable to use target_sources rather than creating a variable, though one can still use
# the variable that exists with target_sources
target_sources(raw PUBLIC ${libraw_LIB_SRCS})

[...]

# This is the modern and clean way to get OpenMP
IF(ENABLE_OPENMP)
     find_package(OpenMP REQUIRED)
     # The target_link_libraries takes care of adding the proper compilation flags
     # and link flags. Much easier than the older way.
     target_link_libraries(raw PUBLIC OpenMP::OpenMP_CXX)
ENDIF(ENABLE_OPENMP)

[...]

Other things could benefit from this modern/cleaner approach.
In particular, the install should probably be modernized also so that the target_link_library where libraw is used links with the required OpenMP libraries.

@Dawars
Copy link
Contributor

Dawars commented Jul 30, 2020

I started working on this but no promises: https://github.com/Dawars/LibRaw-cmake/tree/modern_cmake

The samples build and tested raw-identify which works.

I'm planning to further update dependencies to use the modern way.
And most importantly my goal is for it to be able to be used via add_submodule()

@FunMiles
Copy link
Author

I do not know anything with submodules, so I cannot comment on that. I use git subrepo instead.
A quick comment however (didn't see how to put the comment on your repository directly) is that the version of cmake is a bit strange in the line: CMAKE_MINIMUM_REQUIRED(VERSION 3.1..11)

I believe it should be 3.12. Reading the release notes of 3.12, it mentions that 3.12 adds imported targets for JPEG. So that the JPEG::JPEG target requires 3.12 to be defined.
See https://cmake.org/cmake/help/v3.12/module/FindJPEG.html#module:FindJPEG vs https://cmake.org/cmake/help/v3.11/module/FindJPEG.html#module:FindJPEG

@Dawars
Copy link
Contributor

Dawars commented Jul 30, 2020

Thank you for your response.

By submodules I mean that you can add a lib as a git submodule which links it to the repo.
In cmake use add_subdirectory(libs/libraw) which will call the CMakeList within that directory and potentially export a library target. Which can be later linked to other libs/executables: https://github.com/Dawars/bundle_adjustment/blob/master/src/CMakeLists.txt#L29

The cmake version is not final, I'd need to test it (3.1..11 is supposed to be a backwards compatible range https://cliutils.gitlab.io/modern-cmake/chapters/basics.html)

Also it'd be a good idea to decide on the minimum version of cmake. Although it is "easy" to install the latest version, Ubuntu

  • 16.04 has 3.5.1
  • 18.04 has 3.10
  • 20.04 has 3.16.

I think these are relevant for most linux users. But if these are already too old then I would suggest to use the one that comes with the latest LTS.

Alternatively, it is possible to manually add add the exported targets for earlier versions like this: https://cliutils.gitlab.io/modern-cmake/chapters/packages/OpenMP.html

@FunMiles
Copy link
Author

Thanks for the info on the cmake version backward range. I had not seen it before. I admit that I tend to go for a fairly recent version of CMake, since, as you mention, it is easy to install it, even on a remote system where I do not have administrator privileges.
Of course, one can do the kind of manual creation of the exported target that you linked for OpenMP. The danger I see with that is that it adds a lot of special case CMake code. That kind of code eventually becomes very stale with passing time. So perhaps one needs to decide at some point to eventually deprecate the use of older version for some features.

@Dawars
Copy link
Contributor

Dawars commented Aug 7, 2020

The minimum supported cmake version is now 3.12.

I think everything mentioned in this issue has been implemented. The pull request is merged now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants