Provide an explicit default CMAKE_BUILD_TYPE #3131
Conversation
|
I assume this will not apply to Trilinos since its inside the not having set Project Name clause? |
Yes, that was the intention. I think it's not our business to do anything if |
CMakeLists.txt
Outdated
| @@ -83,6 +83,13 @@ IF(NOT KOKKOS_HAS_TRILINOS) | |||
| ENDif() | |||
| IF(NOT DEFINED ${PROJECT_NAME}) | |||
There was a problem hiding this comment.
Shouldn't this be
IF (NOT DEFINED PROJECT_NAME)
?
There was a problem hiding this comment.
I realize this line already exists, but I think this is wrong. This line is 3 years old and somehow never got caught.
There was a problem hiding this comment.
This is obviously unrelated to the PR, per se, but should we fix it now?
| @@ -83,6 +83,13 @@ IF(NOT KOKKOS_HAS_TRILINOS) | |||
| ENDif() | |||
| IF(NOT DEFINED ${PROJECT_NAME}) | |||
| PROJECT(Kokkos CXX) | |||
| IF (NOT CMAKE_BUILD_TYPE) | |||
There was a problem hiding this comment.
Would this better to check DEFINED CMAKE_BUILD_TYPE? There are uses for this being empty (i.e. CMake adds none of its own flags).
There was a problem hiding this comment.
The purpose of this pull request was to default to something reasonable when no build type is provided when calling cmake.
If I check
IF(NOT DEFINED CMAKE_BUILD_TYPE)
I still get an empty build type in that case. In other words, CMAKE_BUILD_TYPE is already defined (as empty) at this point.
There was a problem hiding this comment.
Yep. I see the problem. See comment below.
| SET(DEFAULT_BUILD_TYPE "Release") | ||
| MESSAGE(STATUS "Setting build type to '${DEFAULT_BUILD_TYPE}' as none was specified.") | ||
| SET(CMAKE_BUILD_TYPE "${DEFAULT_BUILD_TYPE}" CACHE STRING | ||
| "Choose the type of build, options are: Debug, Release, RelWithDebInfo and MinSizeRel." |
There was a problem hiding this comment.
Same question. If we only go here if build type has not been defined, we can just use a regular variable and don't need a cache variable.
There was a problem hiding this comment.
This needs to be a CACHE variable to have any effect.
There was a problem hiding this comment.
Touché. This does get automatically defined in the CACHE even if not given on the command line. My bad.
CMakeLists.txt
Outdated
| PROJECT(Kokkos CXX) | ||
| IF (NOT CMAKE_BUILD_TYPE) | ||
| SET(DEFAULT_BUILD_TYPE "Release") |
Without this, we don't set any debug or optimization flags by default.
The actual default type is debatable but I think
Releaseis an appropriate choice.