[Lldb-commits] [PATCH] D61956: [CMake] Add first CMake cache files

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

In D61956#1505144 <https://reviews.llvm.org/D61956#1505144>, @xiaobai wrote:

> I noticed you have lots of comments that say "Release has different values for these variables".


Yes, I was arguing: //In a previous sketch I had extra variants for development and release builds. I preferred to keep only one variant for now and make notes for release builds instead, because actual release configurations are likely more complex anyway.//

> I think that you could instead guard the Release configuration behind a check.

This would require setting `CMAKE_BUILD_TYPE` before loading the cache and it's not obvious when looking at the cache file.
I think we should avoid this, because the more useful command line order is: first load the cache, then override options on top of it. Let's not motivate people to mix the two. Thus, I'd keep it as is.

Thanks for all the input and discussions. I think this is something we have to iterate on. The current state looks like a good starting point to me. If there are no major concerns, I would land this some time soon.



================
Comment at: lldb/cmake/caches/Apple-lldb-macOS.cmake:18
+set(CMAKE_BUILD_TYPE RelWithDebInfo)
+set(LLVM_ENABLE_MODULES ON CACHE BOOL "")
----------------
xiaobai wrote:
> xiaobai wrote:
> > labath wrote:
> > > sgraenitz wrote:
> > > > compnerd wrote:
> > > > > sgraenitz wrote:
> > > > > > Can / Should we add this? Here?
> > > > > This is fine to add assuming that you are building on Darwin since that implies clang and that will work.  I would say that if you really want to be pedantically correct, it would be better to do:
> > > > > 
> > > > > ```
> > > > > if(CMAKE_CXX_COMPILER_ID MATCHES Clang)
> > > > >   set(LLVM_ENABLE_MODULES_ON CACHE BOOL "")
> > > > > endif()
> > > > > ```
> > > > > 
> > > > > Note that the `MATCHES` is required here to match both `Clang` and `AppleClang`.
> > > > That sounds reasonable. I would take your version and put it in the base cache?
> > > > 
> > > > The other question was, whether `RelWithDebInfo` is a good default. Personally, I use it far more often than other configurations. (Running the test suite with a debug Clang is just too slow.) Moving to the base cache too.
> > > Are you sure that `CMAKE_CXX_COMPILER_ID` is available this early in the initialization ?
> > I think RelWithDebInfo is an okay default. I personally don't like to set build types in caches because I think it's reasonable to expect the user to specify their build type, but if you mostly use that one build type then it's fine.
> I don't think `CMAKE_CXX_COMPILER_ID` is available at the cache processing stage of initialization, so this is probably not something you can do. Maybe you can guard with the condition that the OS is Darwin? Maybe that's not needed, since the cache is Apple-specific anyway.
Build type can still be set on the command line, it just feels like a better default then Debug.
Everything else: agreed. Will keep it as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61956