search

smanders / externpro

build external projects with cmake
MIT License
13 stars 12 forks source link

update cmake version beyond 3.12 #268

Closed smanders closed 4 years ago

smanders commented 4 years ago

We build cmake on linux (as part of externpro) and use a downloaded version for Windows. We build on linux so we can match the version we use on Windows, because it's generally very tricky to get an exact version on linux with different distributions, etc.

I know of a few cmake warnings when using a newer version of cmake (greater than our current 3.12) generated by externpro cmake code... given a little time I'm sure I could fix these warnings while also producing a linux build of the version of cmake we decide on.

Once we have an installer for our supported platforms, one hurdle has always been to get it installed in all the locations where we need to build. We will need system admin support to get this done.

Here are some closed externpro issues where we tracked updating to cmake 3.12 (and discussed the possibility of moving to 3.12.2) in quarter 3 of 2018. There were a few bumps along the way, so it's not always a slam dunk. https://github.com/smanders/externpro/issues/203 https://github.com/smanders/externpro/issues/217

From: Blake Nelson Sent: Friday, April 10, 2020 8:14 AM To: Scott M Anderson; Pete Mace Cc: Andres Imperial; Aaron May; Kevin Glenn Subject: CMake 3.13

Scott/Peter,

Is there any possibility that the build systems for the <undisclosed> plugin could use CMake 3.13 or later?

There is bug in CMake 3.12 (https://gitlab.kitware.com/cmake/cmake/issues/18008) relating to CUDA compilation that has prevented us from writing our CUDA code as optimally as we'd like. Up to this point we have been able to find workarounds that were not too bad in terms of implementation time or CUDA performance.

Our most recent CUDA updates have very poor performance. The straightforward optimizations expose the CMake 3.12 bug so I've spent much of this past week trying to implement the workarounds. These workarounds are taking much longer than they usually do and are starting to adversely impact code quality.

Our preference from both a scheduling and code quality point of view would be to use CMake 3.13 if at all possible.

Thanks.

Blake

smanders commented 4 years ago

3.13 Release Notes https://cmake.org/cmake/help/v3.13/release/3.13.html 3.14 Release Notes https://cmake.org/cmake/help/v3.14/release/3.14.html 3.15 Release Notes https://cmake.org/cmake/help/v3.15/release/3.15.html 3.16 Release Notes https://cmake.org/cmake/help/v3.16/release/3.16.html 3.17 Release Notes https://cmake.org/cmake/help/v3.17/release/3.17.html

to get a feel for cmake releases https://github.com/Kitware/CMake/releases here are some dates

I would vote we move to at least 3.16.6 (the release notes don't currently show what was fixed since 3.16.5) or the very latest 3.17.1 -- I'll look closer at the release notes to see if there are compelling features in 3.17, but very often the bleeding edge has given us issues on past releases

smanders commented 4 years ago

https://github.com/Kitware/CMake/compare/v3.12.0...smanders:xp3.12.0 https://github.com/Kitware/CMake/compare/v3.17.2...smanders:xp3.17.2

https://github.com/Kitware/CMake/releases/tag/v3.17.2

smanders commented 4 years ago

using cmake 3.17 on Vantage (a project that uses externpro) generates a bunch of cmake warnings (one for every external project from externpro, internpro, and webpro) -- similar to the following

CMake Warning (dev) at /usr/share/cmake-3.17/Modules/FindPackageHandleStandardArgs.cmake:272 (message):
  The package name passed to `find_package_handle_standard_args` (eigen) does
  not match the name of the calling package (usexp-eigen).  This can lead to
  problems in calling code that expects `find_package` result variables
  (e.g., `_FOUND`) to follow a certain pattern.
Call Stack (most recent call first):
  /opt/extern/externpro-20.02.1-gcc750-64-Linux/share/cmake/usexp-eigen-config.cmake:24 (find_package_handle_standard_args)
  /opt/extern/externpro-20.02.1-gcc750-64-Linux/share/cmake/xpfunmac.cmake:1173 (find_package)
  Shared/make/toplevel.cmake:11 (xpFindPkg)
  CMakeLists.txt:5 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

looking at the CMake documentation for find_package_handle_standard_args() https://cmake.org/cmake/help/v3.17/module/FindPackageHandleStandardArgs.html I thought the most straightforward change to make would be to add NAME_MISMATCHED to every find_package_handle_standard_args() call

diff --git a/projects/use/usexp-eigen-config.cmake b/projects/use/usexp-eigen-config.cmake
index 2fe5247..b42d033 100644
--- a/projects/use/usexp-eigen-config.cmake
+++ b/projects/use/usexp-eigen-config.cmake
@@ -21,5 +21,5 @@ if(NOT TARGET xpro::eigen)
 endif()
 set(reqVars ${PRJ}_VER ${PRJ}_INCLUDE_DIR)
 include(FindPackageHandleStandardArgs)
-find_package_handle_standard_args(${prj} REQUIRED_VARS ${reqVars})
+find_package_handle_standard_args(${prj} REQUIRED_VARS ${reqVars} NAME_MISMATCHED)
 mark_as_advanced(${reqVars})

this would need to be done for every usexp-<project>-config.cmake ("use script") in externpro, internpro, and webpro

this solution seemed painful because it would require simultaneous releases of externpro, internpro, and webpro that aren't backwards compatible to versions of cmake (for example, the current 3.12) that don't understand NAME_MISMATCHED

even with all of these changes, I was still experiencing a cmake warning generated by protobuf and I thought I would need to patch protobuf to get past the warning generated by these lines https://github.com/protocolbuffers/protobuf/blob/3.0.0-beta-3/cmake/protobuf-module.cmake.in#L134-L135

FIND_PACKAGE_HANDLE_STANDARD_ARGS(PROTOBUF DEFAULT_MSG
    PROTOBUF_LIBRARY PROTOBUF_INCLUDE_DIR)

but looking closer I realized that this is the "simple" or "old" signature of find_package_handle_standard_args() mentioned in the docs https://cmake.org/cmake/help/v3.17/module/FindPackageHandleStandardArgs.html and reading closer led me to a mention of setting the FPHSA_NAME_MISMATCHED variable to bypass the warning if using the old signature

and so I found that I wouldn't need to patch protobuf, but could do this in the protobuf use script

diff --git a/projects/use/usexp-protobuf-config.cmake b/projects/use/usexp-protobuf-config.cmake
index 0565e32..6bddcd0 100644
--- a/projects/use/usexp-protobuf-config.cmake
+++ b/projects/use/usexp-protobuf-config.cmake
@@ -11,8 +11,10 @@ get_filename_component(XP_ROOTDIR ${XP_ROOTDIR} ABSOLUTE) # remove relative part
 string(TOUPPER ${prj} PRJ)
 set(${PRJ}_VER "@VER@ [@PROJECT_NAME@]")
 set(nsPrefix xpro::) # TRICKY: nsPrefix also used in protobuf-module.cmake
+set(FPHSA_NAME_MISMATCHED ON) # protobuf-module.cmake FIND_PACKAGE_HANDLE_STANDARD_ARGS old signature
 # protobuf installs a config file which includes -targets.cmake and -module.cmake
 include(${XP_ROOTDIR}/lib/cmake/protobuf_@VER@/${prj}-config.cmake)
+unset(FPHSA_NAME_MISMATCHED)
 set(${PRJ}_LIBRARIES ${nsPrefix}libprotobuf)
 set(${PRJ}_PROTOC_EXECUTABLE ${nsPrefix}protoc) # TRICKY: must be named to match what's used in -module.cmake
 get_target_property(PROTOBUF_INCLUDE_DIR ${nsPrefix}libprotobuf INTERFACE_INCLUDE_DIRECTORIES)

I found I had to unset(FPHSA_NAME_MISMATCHED) because cmake wasn't happy if I was using both FPHSA_NAME_MISMATCHED and NAME_MISMATCHED

and then the light bulb turned on... and I realized I could avoid using NAME_MISMATCHED in every single use script across externpro, internpro, and webpro if I simply used FPHSA_NAME_MISMATCHED globally for anything that utilizes externpro! so where should that call be made?

with cmake 3.17, running cmake on internpro (patch step) generates the cmake warning about the mismatched name (because it calls the patch project use script), which means putting

set(FPHSA_NAME_MISMATCHED TRUE)

in Findexternpro.cmake ("externpro find script") so that it's set for everything that uses externpro

and so that's the story behind the "resolve CMake Warnings: find_package_handle_standard_args" commit

smanders commented 4 years ago

and with these commits (to resolve cmake warnings) we can release externpro independently of internpro and webpro and the changes are backwards compatible, too... so these changes don't require us to switch to cmake 3.17 (for these changes)

smanders commented 4 years ago

https://github.com/Kitware/CMake/compare/v3.17.3...smanders:xp3.17.3 https://github.com/Kitware/CMake/releases/tag/v3.17.3 https://cmake.org/download/

smanders commented 4 years ago

cmake 3.17.3 is the version externpro will release with... completed with commits to dev branch referenced above