Allow CamelCase names for CMAKE_BUILD_TYPE ‘Debug’, ‘Release’ and empty ''

[bartlettroscoe]

Raw CMake accepts CamelCase names for 'Debug', 'Release', 'RelWithDebInfo' and 'MinSizeRel' as well as empty '' for CMAKE_BUILD_TYPE as described here:

https://cmake.org/cmake/help/v3.0/variable/CMAKE_BUILD_TYPE.html

However, raw CMake applies the build options specified by CMAKE_<LANG>_FLAGS_[DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL]. That seems inconsistent. Therefore, TriBITS many years ago adopted the convention to use upper case names for CMAKE_BUILD_TYPE of 'DEBUG', 'RELEASE', etc. and it defined the value 'NONE'. If the user passes in empty '', then the default of 'RELEASE' is selected.

While that seemed logical at the time, this is not consistent with raw CMake. A user who knows raw CMake will naturally try to configure at TriBITS-based project with -DCMAKE_BUILD_TYPE=Release or -DCMAKE_BUILD_TYPE=Debug or even -DCMAKE_BUILD_TYPE= (empty). Even though TriBITS does technically accept any case 'CMAKE_BUILD_TYPE' (it upper-cases it), this does not have the same behavior as with raw CMake.

Therefore, this story is to update TriBITS so that it behaves exactly like CMake w.r.t. standard values for CMAKE_BUILD_TYPE. The only exception is that if the user does not set 'CMAKE_BUILD_TYPE', then the project can change the default to whatever it wants.

The challenge will be fixing this behavior but not breaking backward compatibility or if backward compatibility will be broken, do so in a way that causes least harm and/or risk.

Tasks:

1. Pin down and document the exact behavior of raw CMake w.r.t. CMAKE_BUILD_TYPE.
2. Determine the desired behavior for CMAKE_BUILD_TYPE and if the upper case versions will be allowed or not (see options below).
3. Implement new behavior as part the plan decided on.
4. Add automated tests that define and protect the correct behavior.

[bartlettroscoe]

For an example of what needs fixed, if you configure a TriBITS project (e.g. Trilinos) with:

$ cmake -DCMAKE_BUILD_TYPE=Debug ../../../Trilinos

it does not set up for debug-mode checking by default:

$ grep ENABLE_DEBUG CMakeCache.txt 
...
Trilinos_ENABLE_DEBUG:BOOL=OFF
...

But when you use upper-case DEBUG, it does:

$ cmake -DCMAKE_BUILD_TYPE=DEBUG ../../../Trilinos

$ grep ENABLE_DEBUG CMakeCache.txt 
...
Trilinos_ENABLE_DEBUG:BOOL=ON
...

Also, if the user explicitly sets -DCMAKE_BUILD_TYPE="" it does not respect that and it goes ahead and sets a default build type:

cmake -DCMAKE_BUILD_TYPE= ../../../Trilinos 2>&1 | grep CMAKE_BUILD_TYPE
-- Setting CMAKE_BUILD_TYPE=RELEASE since it was not set ...
-- CMAKE_BUILD_TYPE=''

It could be hard to fix this without breaking backward compatibility.

[bartlettroscoe]

This issue was raised again in #325 (@gsjaardema). I fear that we are going to have to update TriBITS to recognize upper case or camel case for the CMAKE_BUILD_TYPE values. Otherwise, we will break a lot of CMake configure scripts for existing TriBITS projects.

@gsjaardema, how urgent would you say this is?

[gsjaardema]

I think it is fairly urgent. I had a couple people mention it to me and when I dug deeper I realized that many of my debug builds are not really being built as debug.

At a minimum, the suggested valid build type help string should say these must be all uppercase and give the examples as uppercase (Maybe that is provided by CMAke and unchangeable, in that case, add a comment that says that DEBUG must be DEBUG and not Debug...)

[bartlettroscoe]

At a minimum, the suggested valid build type help string should say these must be all uppercase and give the examples as uppercase

@gsjaardema, the upper case versions for this are documented here:

• https://tribits.org/doc/TribitsBuildReference.html#configuring-to-build-with-default-debug-or-release-compiler-flags

and throughout that document. (And there is a version of this document build for Trilinos at https://docs.trilinos.org/files/TrilinosBuildReference.html.)

But the upper case versions are no-standard CMake so this should be fixed to allow the Camel Case versions of 'Debug', 'Release', etc.

The question is, how? Almost anything we do here is going to break backward compatibility so the question is, what is the best solution for users? I can think of a few different options:

option-1: Allow both Camel Case and Upper Case versions (e.g. Debug and DEBUG) but print a deprecated warning if upper case version is set by a user. (This will make TriBITS code more complex and will require more automated testing to check that both versions are handled correctly. But this will also technically break backward compatibility because a working configuration that sets CMAKE_BUILD_TYPE=Debug may break when TriBITS start recognizing this as a debug build.)

option-2: Change TriBITS to only allow the Camel Case version (i.e. Debug, Release, RelWithDebInfo, MinSizeRel) and error out with a FATAL_ERROR if the upper case version is detected (i.e.. DEBUG, RELEASE, etc.). (This will break every existing configure script but it will do so in a very load and obvious way. And it ensures that all future configure scripts for TriBITS-based project will follow the CMake standard.

What do you think, option-1 or option-2?

[gsjaardema]

For someone who uses CMake, they are not going to read external documentation. They will go into the CMakeCache.txt file and change the CMAKE_BUILD_TYPE to "Debug" or "DEBUG". The comment in that file says the "Debug" is a valid entry so they will use that and not realize that they aren't getting debug code.

I like option-1. I think the backward compatabilty break in this case is potentially good in that it would be catching potential issues in the application that had been missed previously.

[bartlettroscoe]

I like option-1. I think the backward compatibility break in this case is potentially good in that it would be catching potential issues in the application that had been missed previously.

@gsjaardema, okay, I will work on option-1. Hopefully have something later this week.

[bartlettroscoe]

We discussed this at the Trilinos Developers meeting just now. The consensus was to:

• First implement option-1 to support CamelCase and allow UPPERCASE but issue a deprecated configure-time warning when UPPERCASE is being used.
• For the next major Trilinos release (i.e. 14.0.0), implement option-2 and it a FATAL_ERROR to use the UPPERCASE values.

It occurred to me that it would be useful to add a more reasoned approach for handling deprecation and breaks in backward compatibility. I could add a TriBITS option <Project>_CONFIGURE_DEPRECATED_NOTIFICATION with values IGNORE, WARNING, SEND_ERROR and FATAL_ERROR. The default default value would be WARNING but projects could override that by setting set(<Project>_CONFIGURE_DEPRECATED_NOTIFICATION_DEFAULT IGNORE), for example.