[PATCH] D26579: [cmake] Allow CMAKE_BUILD_TYPE=Gentoo

[Michał Górny via llvm-commits]

mgorny added a comment.

In https://reviews.llvm.org/D26579#593712, @beanz wrote:

> The justification of controlling the compiler options is weak to me. At Apple we want to tightly control the flags that are added to the build as well. We do it by setting `CMAKE_BUILD_TYPE=RelWithDebInfo`, then overriding `CMAKE_<LANG>_FLAGS_RELWITHDEBINFO` to ensure CMake doesn't add anything strange. Then we use LLVM options to enable and disable the features that add flags to get the build the way we want.
>
> Why is this insufficient for Gentoo? I can understand that it may take time to craft, but it doesn't impose extra maintenance on the LLVM community, which is desirable. Making our CMake allow non-standard values for standard settings is confusing and I think should be avoided.


I think we are also trying hard to work-around upstreams doing weird stuff on CMAKE_BUILD_TYPE. Of course, that doesn't always work like expected. I'm going to look into changing this but as you can guess, this isn't going to happen overnight.

> Please keep in mind CMake's documentation explicitly says:
> 
>> Possible values are empty, Debug, Release, RelWithDebInfo and MinSizeRel.
> 
> (https://cmake.org/cmake/help/v3.4/variable/CMAKE_BUILD_TYPE.html)
> 
> In the LLVM build we disallowed empty by forcing it to Debug because it causes our build to behave incorrectly (which I could probably be convinced we should fix), but I really don't see a reason we should add a new type.

You should also note this bit following your quotation:

> This variable is only meaningful to single-configuration generators [...] as opposed to multi-configuration generators which offer selection of the build configuration within the generated build environment.

This is one reason not to do anything conditional to CMAKE_BUILD_TYPE. Whenever possible, you should really be using the per-build-type properties. That's tangential but if it could help get rid of the check, I could try preparing a patch for that.


https://reviews.llvm.org/D26579