[PATCH] D124153: [CMake] Change default CMAKE_BUILD_TYPE to Release

Thu Apr 21 10:15:13 PDT 2022

aaronpuchert added subscribers: aaron.ballman, aaronpuchert.
aaronpuchert added a comment.

In D124153#3464356 <https://reviews.llvm.org/D124153#3464356>, @hans wrote:

>> - We can flip it to ON if you don't specify CMAKE_BUILD_TYPE and OFF if you pass Release and ON when passing debug. But this seems a bit ... convoluted? Is that really what end users expect?
>
> Actually I think that's exactly how it works today :-)
>
> I see your point about the logic getting convoluted, but I don't think it'd be too bad, especially since we're printing a message about it anyway. It could be "No build type selected, default to Release with asserts enabled".

Agreed. CMake's default compiler flags for `CMAKE_BUILD_TYPE=Release` are `-O3 -DNDEBUG`, so one would assume that this already disables assertions. If we want to default to `Release` but leave assertions on we're kind of subverting user expectations of how these build profiles behave.

In D124153#3464965 <https://reviews.llvm.org/D124153#3464965>, @mehdi_amini wrote:

> If we can't resolve the targeted audience, what about erroring out when `CMAKE_BUILD_TYPE` isn't defined?

Sounds reasonable to me. Or at least print a warning. But I don't think there is a good default, simply because we don't have good insight into why people compile LLVM and what they would want. And even if we had, this change shows that defaults can change.

Not sure if that could cause issues for multi-configuration users (if that's at all possible in LLVM), maybe @aaron.ballman can chime in (assuming that you use the Visual Studio generator)?

================
Comment at: llvm/CMakeLists.txt:67
 if (NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
-  message(STATUS "No build type selected, default to Debug")
-  set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "Build type (default Debug)" FORCE)
+  message(STATUS "No build type selected, default to Release with assertions enabled. Use -DCMAKE_BUILD_TYPE=Debug to build with debug info")
+  set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Build type (default Release)" FORCE)
----------------
Can we make this a warning? I don't think users should rely on the default being whatever it currently is.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124153/new/

https://reviews.llvm.org/D124153