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

Modernizing CMake #152

Merged
merged 8 commits into from Feb 18, 2020
Merged

Modernizing CMake #152

merged 8 commits into from Feb 18, 2020

Conversation

@favre49
Copy link
Member

favre49 commented Dec 25, 2019

I tried my hand at "modernizing" ensmallen's Cmake. This is the first time I've worked with a build system (or rather a build system generator) so I might have made silly mistakes or misunderstood some things, so bear with me :).

I mostly just reconfigured the code to work at a target level instead of a directory level. I had to change the FindArmadillo.cmake file a bit to export a target instead of using it's parameters (just like here). The CMake to import OpenMP was taken from here.

favre49 added 2 commits Dec 25, 2019
@favre49

This comment has been minimized.

Copy link
Member Author

favre49 commented Dec 26, 2019

Not sure what that Travis error is, shouldn't it be looking for an ensmallen_test executable?

tests/CMakeLists.txt Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
@birm

This comment has been minimized.

Copy link
Member

birm commented Dec 26, 2019

I think this looks good. There are certainly other ways to organize the tests, but this seems consistent with the previous organization.

@birm
birm approved these changes Dec 26, 2019
CMakeLists.txt Show resolved Hide resolved
Copy link
Member

rcurtin left a comment

Hey @favre49, neat to see how modern CMake looks, and starting with ensmallen is nice because it's pretty easy. :) I have some questions, which I left as comments. Can you look into those when you have a chance please? :)

A couple more questions:

  • How will this work for people who were to make a CMake project that depends on ensmallen?
  • What OSes/setups have you tested this on? We should make sure that we aren't inadvertently breaking things for some users or we will get bug reports quickly. :)

(marked 'request changes' so that mlpack-bot doesn't auto-approve quite yet)

project(ensmallen C CXX)
cmake_minimum_required(VERSION 3.3.2)
project(ensmallen
VERSION 2.10.5

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 27, 2019

Member

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.

This comment has been minimized.

Copy link
@favre49

favre49 Dec 27, 2019

Author Member

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.

This comment has been minimized.

Copy link
@conradsnicta

conradsnicta Dec 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@favre49

favre49 Dec 30, 2019

Author Member

I didn't know we had this file, thanks :)

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 31, 2019

Member

Got it, thanks for the clarification. 👍

CMakeLists.txt Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
@favre49

This comment has been minimized.

Copy link
Member Author

favre49 commented Dec 27, 2019

@rcurtin As a reply to your other comments:

  • It shouldn't affect people's projects (I tested this with mlpack, it was able to detect the library), since the headers are still installed in the same place.

  • This is something I was planning to bring up too. I have access to only one setup right now, and that's my own, which is Arch Linux with CMake version 3.16.2. I'll downgrade to test with lower versions when I get the time, and I'll also try testing whether or not another library can import ensmallen for use. We'll have to test on other setups too (especially MacOS and Windows, which I know next to nothing about). Not sure how to be going about that though.

@jeffin143

This comment has been minimized.

Copy link
Contributor

jeffin143 commented Dec 29, 2019

@rcurtin As a reply to your other comments:

  • It shouldn't affect people's projects (I tested this with mlpack, it was able to detect the library), since the headers are still installed in the same place.
  • This is something I was planning to bring up too. I have access to only one setup right now, and that's my own, which is Arch Linux with CMake version 3.16.2. I'll downgrade to test with lower versions when I get the time, and I'll also try testing whether or not another library can import ensmallen for use. We'll have to test on other setups too (especially MacOS and Windows, which I know next to nothing about). Not sure how to be going about that though.

I will help , I will setup some VMS and also test it on Mac os, will pull your branch up and then test it during first week of January :)

string(REGEX REPLACE ".*#define ENS_VERSION_MINOR ([0-9]+).*" "\\1" ENSMALLEN_VERSION_MINOR "${ENSMALLEN_VERSION_FILE_CONTENTS}")
string(REGEX REPLACE ".*#define ENS_VERSION_PATCH ([0-9]+).*" "\\1" ENSMALLEN_VERSION_PATCH "${ENSMALLEN_VERSION_FILE_CONTENTS}")

message(STATUS "Configuring Ensmallen ${ENSMALLEN_VERSION_MAJOR}.${ENSMALLEN_VERSION_MINOR}.${ENSMALLEN_VERSION_PATCH}")

This comment has been minimized.

Copy link
@zoq

zoq Dec 30, 2019

Member

I would prefer if we don't start with a capital e here, we usually use small caps for ensmallen.

enable_cxx11()
# Set required C++ standard to C++11.
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

This comment has been minimized.

Copy link
@zoq

zoq Dec 30, 2019

Member

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

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 31, 2019

Member

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

"Installation directory for cmake files, relative to ${CMAKE_INSTALL_PREFIX}.")
set(version_config "${PROJECT_BINARY_DIR}/ensmallen-config-version.cmake")
set(project_config "${PROJECT_BINARY_DIR}/ensmallen-config.cmake")
set(targets_export_name ensmallen-targets)

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 31, 2019

Member

I typically use capital letters to represent variable names, but I guess it is not a big deal either way. Still, maybe good to change it for the sake of consistency?

Copy link
Member

rcurtin left a comment

Just a couple more questions from my end. I think I understand everything else though. Awesome work putting this together! I suppose, a good test to work out any bugs might be setting up mlpack's CMake configuration to handle this new ensmallen configuration and ensure that everything works. :)

(Although, I guess for that, we have to handle reverse compatibility, so it could be just a little tricky, since the user might have an older version installed.)

# Generate the version, config and target files into the build directory.
write_basic_package_version_file(${version_config}
VERSION ${VERSION}
COMPATIBILITY AnyNewerVersion)

This comment has been minimized.

This comment has been minimized.

Copy link
@favre49

favre49 Jan 2, 2020

Author Member

This isn't an available option for CMake 3.3.2, I believe it has been added in one of the latest versions

This comment has been minimized.

Copy link
@rcurtin

rcurtin Jan 9, 2020

Member

Ah, got it, thanks for the clarification. Looks like ARCH_INDEPENDENT was added in 3.14. Do you think we should do something like this?

if (${CMAKE_VERSION} VERSION_LESS "3.14.0")
  write_basic_package_version_file( ... )
else ()
  write_basic_package_version_file( ... ARCH_INDEPENDENT ... )
endif ()

I'm not sure how important the ARCH_INDEPENDENT really is.


# Set helper variables for creating the version, config and target files.
include(CMakePackageConfigHelpers)
set(ENSMALLEN_CMAKE_DIR "lib/cmake/ensmallen" CACHE STRING

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 31, 2019

Member

Should this be share/cmake/ensmallen? On all systems I've used, CMake scripts install to /usr/share/cmake/... or something like that. It seems like the example here uses lib/Foo/cmake though. I don't know what's needed to make it so that after installing ensmallen, find_package(ensmallen) will "just work" though.

This comment has been minimized.

Copy link
@favre49

favre49 Jan 2, 2020

Author Member

I did this reading another library's CMakeLists, I'll test it by making a library that used ensmallen and see if find_package works

This comment has been minimized.

Copy link
@rcurtin

rcurtin Jan 9, 2020

Member

Sounds good, so long as it works with find_package() I guess it doesn't matter so much where it gets put. :)

favre49 added 2 commits Jan 2, 2020
@rcurtin
rcurtin approved these changes Jan 9, 2020
Copy link
Member

rcurtin left a comment

Nothing else from my end---how you want to handle the ARCH_INDEPENDENT is up to you. It might be worth opening an issue for if you think it's worth fixing later or investigating more. 👍

Awesome work! 🚀

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Jan 9, 2020

(@favre49 you should go ahead and merge this one when you're ready---I'm not sure if you're done with it, so I'm not going to click the "merge" button. :))

@PACKAGE_INIT@

include(${CMAKE_CURRENT_LIST_DIR}/@targets_export_name@.cmake)
check_required_components(ensmallen)

This comment has been minimized.

Copy link
@kumaran-14

kumaran-14 Jan 18, 2020

Newline at the end?

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2020

Member

Thanks, handled during merge with 67a94b2. 👍

@mlpack-bot

This comment has been minimized.

Copy link

mlpack-bot bot commented Feb 17, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

@mlpack-bot mlpack-bot bot added the s: stale label Feb 17, 2020
@favre49 favre49 removed the s: stale label Feb 17, 2020
@favre49

This comment has been minimized.

Copy link
Member Author

favre49 commented Feb 17, 2020

Sorry for the lack of activity, college has kept me busy with other projects. I think we could go ahead and merge this, but I'm also worried this could break something. Maybe someone interested in the GSoC project on CMake modules could test this more? I don't see myself getting time to test it myself any time soon.

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Feb 17, 2020

@favre49 no worries, I think it's mergeable too. Let's see if this passes the Windows build once #158 is merged (hopefully in just a few minutes), and if it does, then I think we should merge this too. 👍

@rcurtin rcurtin closed this Feb 17, 2020
@rcurtin rcurtin reopened this Feb 17, 2020
@@ -1,72 +1,101 @@
# 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)

This comment has been minimized.

Copy link
@conradsnicta

conradsnicta Feb 18, 2020

Contributor

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))

This comment has been minimized.

Copy link
@rcurtin

rcurtin Feb 18, 2020

Member

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. 👍

This comment has been minimized.

Copy link
@conradsnicta

conradsnicta Feb 19, 2020

Contributor

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

@rcurtin rcurtin merged commit bed3773 into mlpack:master Feb 18, 2020
3 checks passed
3 checks passed
Static Code Analysis Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Feb 18, 2020

Thanks @favre49 and everyone else who helped with this! 🎉

@rcurtin rcurtin mentioned this pull request Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.