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.

Sign up for GitHub

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

Jump to bottom

Modernizing CMake #152

Merged
merged 8 commits into from
Feb 18, 2020
Merged

Modernizing CMake #152

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5c1eb89
It works
favre49 Dec 25, 2019
1812812
Cut down a bit, and some more documentation
favre49 Dec 25, 2019
53ee9f5
Fixed version
favre49 Dec 25, 2019
46856b0
Fixed add_test
favre49 Dec 26, 2019
fc5ce2f
Added warning flags and exporting scripts.
favre49 Dec 27, 2019
8ef70e3
Added Conrad's suggestion. Must fix styles.
favre49 Dec 30, 2019
369046f
Changed case where necessary
favre49 Jan 2, 2020
bed3773
Removed ARCH_INDEPENDENT
favre49 Jan 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CMake/FindArmadillo.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,4 +366,10 @@ mark_as_advanced(
ARMADILLO_INCLUDE_DIR
ARMADILLO_LIBRARIES)

if (ARMADILLO_FOUND AND NOT TARGET Armadillo:Armadillo)
add_library(Armadillo::Armadillo INTERFACE IMPORTED)
set_target_properties(Armadillo::Armadillo PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${ARMADILLO_INCLUDE_DIR}"
INTERFACE_LINK_LIBRARIES "${ARMADILLO_LIBRARIES}")
endif()

#======================
73 changes: 28 additions & 45 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,61 +1,44 @@
# ensmallen CMake configuration. This project has no configurable options---it
# just installs the headers to the install location, and optionally builds the
# test program.
cmake_minimum_required(VERSION 2.8.10)
project(ensmallen C CXX)
cmake_minimum_required(VERSION 3.3.2)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly, Red Hat Enterprise Linux 7 (RHEL 7) has CMake 2.8.12 by default. RHEL 7 has gcc 4.8 by default, meaning it handles C++11 (using the -std=c++11 switch), and so it can compile ensmallen and mlpack.

RHEL 7 is used on a lot of computing clusters, suggesting that this CMake modernisation effort might end up reducing the uptake of ensmallen.

I generally don't see the point in bumping CMake requirements unless they bring in an important fix and/or functionality to the table.

On the Armadillo side I've made optional use of CMake 3.x features: https://gitlab.com/conradsnicta/armadillo-code/-/blob/9.850.x/CMakeLists.txt
Look for code with stuff like if(NOT (${CMAKE_MAJOR_VERSION} LESS 3))

Copy link
Member

@rcurtin rcurtin Feb 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This caused me some amount of consternation some time ago also, until I found the cmake3 package in EPEL: https://rpmfind.net/linux/rpm2html/search.php?query=cmake3, so I think that should be sufficient for users on CentOS/RHEL 7. I definitely agree that RHEL7 is a big target that we can't leave behind. 👍

Copy link
Contributor

@conradsnicta conradsnicta Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest expanding the README to inform folks on RHEL systems where the updated cmake package can be obtained. For example:

The installation of ensmallen requires cmake 3.3 or later.
For older systems, such as RHEL 7, an updated version of cmake can be obtained from https://cmake.org or via the EPEL repository.

Edit: see PR #169

project(ensmallen
VERSION 2.10.5
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to specify VERSION here? (Does it get us anything?) I can see this going out of date very quickly if we're not careful. In fact technically it's already out of date---2.11.0 is the newest version, but the git master branch should probably have a different version number. So if we can avoid adding this I think it might be better; but I am not familiar with the VERSION option, so maybe there is something I don't know about why it's needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably wasn't clear because I hadn't written the code for exporting the target, it was a misunderstanding on my part. The VERSION option helps us create the version config file, which is what another downstream project would use to check whether they are importing a compatible version, if they used find_package(ensmallen 2.10.0). You can see this in the wiki here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hardcoding the version number into CMakeLists.txt, it's possible to extract the version of ensmallen directly from the C++ source file. Example: https://gitlab.com/conradsnicta/armadillo-code/blob/9.800.x/CMakeLists.txt#L68-81

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 didn't know we had this file, thanks :)

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks for the clarification. 👍

LANGUAGES C CXX)

# Configurable options for CMake
option(USE_OPENMP "If available, use OpenMP for parallelization." ON)
option(BUILD_TESTS "Build tests." ON)

set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_SOURCE_DIR}/CMake")

# Ensure we have C++11 features. Since we support CMake < 3.1, this needs a
# little bit of special handling.
if ((${CMAKE_MAJOR_VERSION} LESS 3 OR
(${CMAKE_MAJOR_VERSION} EQUAL 3 AND ${CMAKE_MINOR_VERSION} LESS 1))
AND NOT FORCE_CXX11)
# Older versions of CMake do not support target_compile_features(), so we have
# to use something kind of hacky.
include(CMake/CXX11.cmake)
check_for_cxx11_compiler(HAS_CXX11)
if(NOT HAS_CXX11)
message(FATAL_ERROR "No C++11 compiler available!")
# Set required C++ standard to C++11
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
Copy link
Member

Choose a reason for hiding this comment

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

Can I still force the standard, in case I have a strange setup?

Copy link
Member

Choose a reason for hiding this comment

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

Does ensmallen work with anything older than C++11? (I'm not sure on that.)


# Create library target
add_library(ensmallen INTERFACE)
target_include_directories(ensmallen INTERFACE
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>)
rcurtin marked this conversation as resolved.
Show resolved Hide resolved

# Find OpenMP and link it.
if(USE_OPENMP)
if(NOT TARGET OpenMP::OpenMP_CXX)
find_package(Threads REQUIRED)
add_library(OpenMP::OpenMP_CXX IMPORTED INTERFACE)
set_property(TARGET OpenMP::OpenMP_CXX
PROPERTY INTERFACE_COMPILE_OPTIONS ${OpenMP_CXX_FLAGS})
# Only works if the same flag is passed to the linker; use CMake 3.9+ otherwise (Intel, AppleClang)
set_property(TARGET OpenMP::OpenMP_CXX
PROPERTY INTERFACE_LINK_LIBRARIES ${OpenMP_CXX_FLAGS} Threads::Threads)
endif()
enable_cxx11()
else()
# set required standard to c++11
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
endif ()

# Detect OpenMP support in a compiler. If the compiler supports OpenMP, flags
# to compile with OpenMP are returned and added for compilation.
if (USE_OPENMP)
find_package(OpenMP)
endif ()

if (OPENMP_FOUND)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${OpenMP_C_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OpenMP_CXX_FLAGS}")
else ()
# Disable warnings for all the unknown OpenMP pragmas.
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas")
endif ()

# Set the CFLAGS and CXXFLAGS depending on the options the user specified.
if(CMAKE_COMPILER_IS_GNUCC OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wpedantic -Wunused-parameter")
favre49 marked this conversation as resolved.
Show resolved Hide resolved
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Wpedantic -Wunused-parameter")
target_link_libraries(ensmallen INTERFACE OpenMP::OpenMP_CXX)
endif()

# The only dependency we need is Armadillo.
#
# We keep the minimum version in sync with mlpack, otherwise we could have
# irritating compatibility issues.
# Find Armadillo and link it.
find_package(Armadillo 8.400.0 REQUIRED)
include_directories(BEFORE "${ARMADILLO_INCLUDE_DIR}")
include_directories(BEFORE "${CMAKE_SOURCE_DIR}/include/")
target_link_libraries(ensmallen INTERFACE Armadillo::Armadillo)

# Install the headers to the correct location.
install(DIRECTORY "${CMAKE_SOURCE_DIR}/include/ensmallen_bits"
Expand Down
12 changes: 5 additions & 7 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
project(ensmallen_tests CXX)
birm marked this conversation as resolved.
Show resolved Hide resolved

# The tests that need to be compiled.
set(ENSMALLEN_TESTS_SOURCES
main.cpp
ada_bound_test.cpp
Expand Down Expand Up @@ -46,17 +45,16 @@ set(ENSMALLEN_TESTS_SOURCES
)

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR})
add_executable(${PROJECT_NAME} ${ENSMALLEN_TESTS_SOURCES})

target_link_libraries(${PROJECT_NAME} ${ARMADILLO_LIBRARIES})
add_executable(ensmallen_tests ${ENSMALLEN_TESTS_SOURCES})
target_link_libraries(ensmallen_tests PRIVATE ensmallen)

# Copy test data into place.
add_custom_command(TARGET ${PROJECT_NAME}
add_custom_command(TARGET ensmallen_tests
POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy_directory ${CMAKE_CURRENT_SOURCE_DIR}/data/
${CMAKE_BINARY_DIR}/data/
)

enable_testing()
add_test(NAME ${PROJECT_NAME} COMMAND ${PROJECT_NAME}
birm marked this conversation as resolved.
Show resolved Hide resolved
WORKING_DIRECTORY ${CMAKE_BINARY_DIR})
WORKING_DIRECTORY ${CMAKE_BINARY_DIR})