Unidata / netcdf-cxx4

Official GitHub repository for netCDF-C++ libraries and utilities.
Other
124 stars 49 forks source link

Unprefixed cache variables in CMakeLists.txt #35

Open firegurafiku opened 8 years ago

firegurafiku commented 8 years ago

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.

firegurafiku commented 8 years ago

@WardF

What do you think about the proposal above?

WardF commented 8 years ago

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