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

Unprefixed cache variables in CMakeLists.txt #35

Open
firegurafiku opened this issue Apr 25, 2016 · 2 comments
Open

Unprefixed cache variables in CMakeLists.txt #35

firegurafiku opened this issue Apr 25, 2016 · 2 comments

Comments

@firegurafiku
Copy link
Contributor

firegurafiku commented Apr 25, 2016

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
Copy link
Contributor Author

@WardF

What do you think about the proposal above?

@WardF
Copy link
Member

WardF commented May 16, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants