netcdf-cxx4
netcdf-cxx4 copied to clipboard
Unprefixed cache variables in CMakeLists.txt
Hi all!
While I was making changes for #34, I've noticed that the project's CMakeLists.txt
defines a lot of cache variables with simple, unprefixed names, and I think they should be fixed. Please, don't treat this message as a critic attack. Of course, I could rename those variables myself, but I cannot PR such a breaking change without a prior discussion.
set(PACKAGE "netcdf-cxx4" CACHE STRING "")
SET(BUILDNAME_PREFIX "" CACHE STRING "")
SET(BUILDNAME_SUFFIX "" CACHE STRING "")
SET(BUILDNAME "${TMP_BUILDNAME}" CACHE STRING "Build name variable for CDash")
set(TMP_BUILDNAME "${osname}-${osrel}-${cpu}" CACHE STRING "Build name variable for CDash")
I don't exactly know how CDash operates, but does it really require us putting such names into CMake cache? May they be turned into, say, NCXX_DASH_PACKAGE
and NCXX_DASH_BUILDNAME
? Maybe it's also worth adding NCXX_ENABLE_DASH
option in order not to pollute end user's cache unless they asked?
SET(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules/"
CACHE INTERNAL "Location of our custom CMake modules.")
Does it really have to be a cache variable? Also, isn't it better to write `list(APPEND CMAKE_MODULE_PATH ...)" in order to have previous search path lost?
# Set the build type.
IF(NOT CMAKE_BUILD_TYPE)
SET(CMAKE_BUILD_TYPE DEBUG CACHE STRING
"Choose the type of build, options are: None, Debug, Release."
FORCE)
ENDIF()
AFAIK, CMAKE_BUILD_TYPE
is always in cache. It may be empty, though, but it's not worse than "None" option. Also, why to enforce debug value?
SET(CTEST_MEMORYCHECK_COMMAND valgrind CACHE STRING "")
What about just find_program(NCXX_VALGRIND_COMMAND valgrind REQUIRED)
which would create corresponding cache item automatically?
SET(HAVE_DOT YES CACHE STRING "")
SET(SITE "${NCXX_CTEST_SITE}" CACHE STRING "")
SET(BUILD_PARALLEL ${NC_IS_PARALLEL} CACHE STRING "")
Some more names which require prefixing.
Note that I'm thinking about the situation when NetCDF-C++ is a part of large software project linking lots of libraries which are sharing single global cache.
@WardF
What do you think about the proposal above?
Hello @firegurafiku sorry for the delayed response; it's been very busy dealing with some changes related to the recent hdf5 1.10.0 release!
I'm perfectly fine prefixing the variable names; it makes perfect sense. Regarding which ones are cached vs. which ones aren't, there is definitely room for refinement; the current cmake configuration file was roughed-in to provide cmake/visual studio support. I don't want to make valgrind
required, unless we fencepost it for Windows platforms.
So, I guess the bottom line is I think the proposal is fine, and we can tweak it if/where needed. And, don't worry, it wasn't received as a critical suggestion; I'm always open to improvements that can be made :).