[cmake] Allow CMAKE_BUILD_TYPE=Gentoo

[mgorny]

Summary

Allow the custom build type of 'Gentoo' that is used for Gentoo builds
of all CMake-based packages. The main goal of this build type is to
work-around some CMake defaults such as compiler flags.

[beanz]

This seems to me like a good use for a CMake cache file. Abusing the build type like this seems highly undesirable to me.

[mgorny]

In D26579#593694, @beanz wrote:

This seems to me like a good use for a CMake cache file. Abusing the build type like this seems highly undesirable to me.

I can't say I know the full rationale, since it was like that long before me. What I know, this is causing a major pain with the few projects that force CMAKE_BUILD_TYPE to be one of the upstream-predicted values. I'd really like to be able to build LLVM without necessity of custom patching, and I thought that allowing this specific build type (with its clear characteristic) is more likely to be accepted than removing the (unnecessary, after all) check completely.

The fact is, LLVM is also doing some changes to the build process depending on build type (e.g. appending flags). I know that's one of the things that Gentoo build type attempts to prevent.

[mehdi_amini]

There is a good reason Gento is doing it: to make sure they have full control over compiler flags in the Gentoo configuration. See here: https://github.com/coreos/portage-stable/blob/master/eclass/cmake-utils.eclass#L43

However accepting the value "Gentoo" here won't address the problem, it'll just mask it. We are expecting values from this list in multiple place.

For example in llvm/cmake/modules/HandleLLVMOptions.cmake:

# On non-Debug builds cmake automatically defines NDEBUG, so we
# explicitly undefine it:
if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )

And

if (NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" AND
    NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "RELWITHDEBINFO")
  add_flag_if_supported("-gline-tables-only" GLINE_TABLES_ONLY)
endif()

[mgorny]

In D26579#593702, @mehdi_amini wrote:

There is a good reason Gento is doing it: to make sure they have full control over compiler flags in the Gentoo configuration. See here: https://github.com/coreos/portage-stable/blob/master/eclass/cmake-utils.eclass#L43

However accepting the value "Gentoo" here won't address the problem, it'll just mask it. We are expecting values from this list in multiple place.

Yes. I can volunteer to make sure that Gentoo appears there whenever desired, if that's what you need.

For example in llvm/cmake/modules/HandleLLVMOptions.cmake:

# On non-Debug builds cmake automatically defines NDEBUG, so we
# explicitly undefine it:
if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )

Just to be clear, here 'Gentoo' is not expected since CMake doesn't control -DNDEBUG for this build type.

if (NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" AND
    NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "RELWITHDEBINFO")
  add_flag_if_supported("-gline-tables-only" GLINE_TABLES_ONLY)
endif()

…and here it's not expected since appending -g flags implicitly is against a Gentoo policy.

I was also wondering about disabling the -Wl,-O3 and friends but not sure about that yet.

[beanz]

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.

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.

[mehdi_amini]

In D26579#593711, @mgorny wrote:

In D26579#593702, @mehdi_amini wrote:

There is a good reason Gento is doing it: to make sure they have full control over compiler flags in the Gentoo configuration. See here: https://github.com/coreos/portage-stable/blob/master/eclass/cmake-utils.eclass#L43

However accepting the value "Gentoo" here won't address the problem, it'll just mask it. We are expecting values from this list in multiple place.

Yes. I can volunteer to make sure that Gentoo appears there whenever desired, if that's what you need.

That's not what *I* need, that's what *you* need.

For example in llvm/cmake/modules/HandleLLVMOptions.cmake:

# On non-Debug builds cmake automatically defines NDEBUG, so we
# explicitly undefine it:
if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )

Just to be clear, here 'Gentoo' is not expected since CMake doesn't control -DNDEBUG for this build type.

But here we're gonna add "-UNDEBUG" when the build type is Gentoo, is it OK?

if (NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" AND
    NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "RELWITHDEBINFO")
  add_flag_if_supported("-gline-tables-only" GLINE_TABLES_ONLY)
endif()

…and here it's not expected since appending -g flags implicitly is against a Gentoo policy.

Right, and with BUILD_TYPE=Gentoo this will add "-gline-tables-only".

I don't know what is the right thing for each of these cases, and this alone shows that you'll need to have a clear description and requirement for this "special" build type if you expect to have it supported upstream.

[mgorny]

In 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.

[mgorny]

In D26579#593739, @mehdi_amini wrote:

In D26579#593711, @mgorny wrote:

In D26579#593702, @mehdi_amini wrote:

There is a good reason Gento is doing it: to make sure they have full control over compiler flags in the Gentoo configuration. See here: https://github.com/coreos/portage-stable/blob/master/eclass/cmake-utils.eclass#L43

However accepting the value "Gentoo" here won't address the problem, it'll just mask it. We are expecting values from this list in multiple place.

Yes. I can volunteer to make sure that Gentoo appears there whenever desired, if that's what you need.

That's not what *I* need, that's what *you* need.

I need things to work somehow. Now I'm looking for a solution that would satisfy the other LLVM developers while making them work for us.

For example in llvm/cmake/modules/HandleLLVMOptions.cmake:

# On non-Debug builds cmake automatically defines NDEBUG, so we
# explicitly undefine it:
if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )

Just to be clear, here 'Gentoo' is not expected since CMake doesn't control -DNDEBUG for this build type.

But here we're gonna add "-UNDEBUG" when the build type is Gentoo, is it OK?

if (NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" AND
    NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "RELWITHDEBINFO")
  add_flag_if_supported("-gline-tables-only" GLINE_TABLES_ONLY)
endif()

…and here it's not expected since appending -g flags implicitly is against a Gentoo policy.

Right, and with BUILD_TYPE=Gentoo this will add "-gline-tables-only".

I don't know what is the right thing for each of these cases, and this alone shows that you'll need to have a clear description and requirement for this "special" build type if you expect to have it supported upstream.

You are right, this is quite confusing.

[mgorny]

I have discussed this with our CMake team further and it seems that we are also using CMAKE_BUILD_TYPE of Gentoo to alter behavior in standard CMake modules (e.g. FindPython*). Using another value will result in CMake behavior that is not appropriate for Gentoo packaging. There may be more hidden issues.

We'll be working on phasing that out but it'll likely take a few months before we can reliably deploy it on everyone. Especially that some of the potential issues are really hard to notice, so it will require a thorough testing.

[rnk]

I'd be surprised if a custom CMAKE_BUILD_TYPE value was really the best way to get distro CFLAGS through third party CMake files. My understanding is that each project defines its own custom build types, so that developers can share common configurations (think release+asserts, -gline-tables-only with logic to opt into gold).

I think if the kitware people come and say that the intention is for well-written CMake files to be configuration agnostic, then we could add support for config=Gentoo. I'd also want to know what the best practices are to avoid iterating over the standard config types. We currently do this kind of thing a lot:

# On non-Debug builds cmake automatically defines NDEBUG, so we
# explicitly undefine it:
if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
  add_definitions( -UNDEBUG )
  # Also remove /D NDEBUG to avoid MSVC warnings about conflicting defines.
  foreach (flags_var_to_scrub
      CMAKE_CXX_FLAGS_RELEASE
      CMAKE_CXX_FLAGS_RELWITHDEBINFO
      CMAKE_CXX_FLAGS_MINSIZEREL
      CMAKE_C_FLAGS_RELEASE
      CMAKE_C_FLAGS_RELWITHDEBINFO
      CMAKE_C_FLAGS_MINSIZEREL)
    string (REGEX REPLACE "(^| )[/-]D *NDEBUG($| )" " "
      "${flags_var_to_scrub}" "${${flags_var_to_scrub}}")
  endforeach()
endif()

[beanz]

Replacing the code we have where we check CMAKE_BUILD_TYPE with more robust and sane checks, is a great idea that I wholly support. Defining custom build types isn't.

From @rnk's example we should be able to translate the conditional:

if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )

Into something more like:

if(CMAKE_CXX_FLAGS_${uppercase_CMAKE_BUILD_TYPE} MATCHES "-DNDEBUG")

This check instead checks to see if the configuration-specific flags contain "-DNDEBUG", which is really what we care about. I had a conversation with @gottesmm about this a while back.

[rnk]

In D26579#594953, @beanz wrote:

if(CMAKE_CXX_FLAGS_${uppercase_CMAKE_BUILD_TYPE} MATCHES "-DNDEBUG")

This check instead checks to see if the configuration-specific flags contain "-DNDEBUG", which is really what we care about. I had a conversation with @gottesmm about this a while back.

This wouldn't support multi-configuration generators very well, though.

[beanz]

@rnk, we could handle multi-configuration generators explicitly, by checking for that case.

CMakeLists.txt

 ${tablegenned_files_on_include_dir}
 Please clean the source directory.")
   endif()
 endif()
 
 string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
 
 if (CMAKE_BUILD_TYPE AND
-    NOT uppercase_CMAKE_BUILD_TYPE MATCHES "^(DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL)$")
+    NOT uppercase_CMAKE_BUILD_TYPE MATCHES "^(DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL|GENTOO)$")
   message(FATAL_ERROR "Invalid value for CMAKE_BUILD_TYPE: ${CMAKE_BUILD_TYPE}")
 endif()
 
 set(LLVM_LIBDIR_SUFFIX "" CACHE STRING "Define suffix of library directory name (32/64)" )
 
 set(LLVM_TOOLS_INSTALL_DIR "bin" CACHE STRING "Path for binary subdirectory (defaults to 'bin')")
 mark_as_advanced(LLVM_TOOLS_INSTALL_DIR)