Don't restrict list of CMake build types

[rossburton]

For example, the llvm directory restricts the value of CMAKE_BUILD_TYPE:

if (CMAKE_BUILD_TYPE AND
    NOT uppercase_CMAKE_BUILD_TYPE MATCHES "^(DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL)$")
  message(FATAL_ERROR "Invalid value for CMAKE_BUILD_TYPE: ${CMAKE_BUILD_TYPE}")
endif()

https://github.com/llvm/llvm-project/blob/main/llvm/CMakeLists.txt#L389

That is the default set of build types, but it is not the exhaustive list of build types. Quoting from the CMake manual:

Typical values include Debug, Release, RelWithDebInfo and MinSizeRel, but custom build types can also be defined.

I suggest just deleting this chunk, as it doesn't serve any purpose: the person building llvm should be able to specify a custom build type if they wish.

[fsb4000]

I suggest just deleting this chunk, as it doesn't serve any purpose: the person building llvm should be able to specify a custom build type if they wish.

I found the commit with that changes: 55f097f

Apparently if you make a typo in the argument to CMAKE_BUILD_TYPE,
cmake silently accepts this but doesn't apply any particular build
type to your build. This means you get a build that doesn't really
make any sense - it's sort of a debug build with asserts disabled.

Error out instead.

I think that commit makes sense.

[fsb4000]

ping @bogner , what do you think?

Should we close this issue or maybe add more CMAKE_BUILD_TYPEs to the list?

[rossburton]

I disagree, mainly as the list of build types is not fixed. If someone means to type Release but types Releas they don’t get a broken build, it’s just missing some optimisation. They made the mistake.

The context here is that I’m defining a new build type as we’re trying to control the build flags, and this fatal error means that fails.

[bogner]

It’s probably fine to remove this check and let people do custom cmake build types. It’s definitely broken if someone makes a typo here, so I do like the safety net, but that’s not the only instance where cmake’s interface is user-hostile and it isn’t really realistic for us to catch all of the ways this can go wrong.

[whisperity]

Suggestion: message(FATAL_ERROR -> message(WARNING? That way we could have a blanket check that still catches the vast majority of accidents (as lots of people defining custom build targets seems to be unlikely) but doesn't immediately axe the build nevertheless?

[rossburton]

I'd certainly be happy with a warning that the build type is unknown. I'm defining a new one so I'm happy with that, but should help users who are new to cmake.

[fsb4000]

I've created: https://reviews.llvm.org/D144835

[pogo59]

If you're defining your own build type, you could modify the check to include your new keyword? I appreciate code that keeps me from wasting time with typos.

[rossburton]

Defining a build type is just a matter of naming it, if I pass -DCMAKE_BUILD_TYPE=FooBar to cmake then it looks for variables named CMAKE_CXX_FLAGS_FOOBAR. By default, cmake sets CMAKE_CXX_FLAGS_DEBUG and so on.

[boomanaiden154]

Just adding to this conversation that I just spent a non-insignificant amount of time tracking down an issue that turned out to be related to a typo I made in specifying CMAKE_BUILD_TYPE. It would've been very nice to have the FATAL_ERROR safety check still in place to catch this as I never ended up seeing the warning.

As @bogner mentions, CMake's design is fairly user-hostile here and doesn't have great support for doing this elegantly, but the current fix seems to optimize for a relatively niche use case at the expense of usability for everyone else. I think there are certainly alternative solutions that allow for this use case while still allowing for the message to set FATAL_ERROR. Maybe something like an addition LLVM_ADDITIONAL_BUILD_TYPES flag that can be set with additional build types that should be accepted? We could also just make the accepted types a cache variable and allow for it to be set as a flag.