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

Thu May 16 11:34:03 PDT 2019

xiaobai added a comment.

I noticed you have lots of comments that say "Release has different values for these variables". I think that you could instead guard the Release configuration behind a check. For example:

  if (CMAKE_BUILD_TYPE MATCHES Release)
    # Do the release configuration stuff here
  else()
    # Do the development configuration stuff here
  endif()

================
Comment at: lldb/cmake/caches/Apple-lldb-macOS.cmake:10
+set(CMAKE_INSTALL_PREFIX "${CMAKE_CURRENT_BINARY_DIR}/install/Developer/usr")
+set(LLDB_FRAMEWORK_INSTALL_DIR "${CMAKE_CURRENT_BINARY_DIR}/install/SharedFrameworks" CACHE STRING "")
+
----------------
sgraenitz wrote:
> labath wrote:
> > sgraenitz wrote:
> > > Follow-up from: https://reviews.llvm.org/D61956?id=199645#inline-550727
> > > 
> > > > I believe the expected usage of this variable is to make it point to the final resting place of the executables, ...
> > > 
> > > It's been a pragmatic decision. Maybe we can improve this. The rationale was, that the default configuration should give the user something that works without touching caches or overriding parameters. In a previous sketch I used a real-world destination like:
> > > ```
> > > set(CMAKE_INSTALL_PREFIX /Applications/Xcode.app/Contents/Developer/usr/)
> > > ```
> > > But then `ninja install` would fail by default due to lack of permissions in the install destination. Actual release configurations tend to be more complex anyway and vendors will have their own downstream repos / caches for it. Thus, choosing a good default for developers sounded like a good way forward. What do you think?
> > > 
> > > > Are you sure including {CMAKE_CURRENT_BINARY_DIR} here is a good idea? I think llvm tries to avoid that generally, ...
> > > 
> > > What exactly do you mean? Having absolute paths, or paths into the build-tree, or the `CMAKE_CURRENT_BINARY_DIR` specifically? I don't see problems with the two last points. Am I missing something?
> > > 
> > > For the first: choosing an absolute path was for consistency with `LLDB_FRAMEWORK_INSTALL_DIR`. In the current build logic, they can both be absolute paths. Otherwise:
> > > * if the install prefix is relative, it will be appended to the path of the root build directory
> > > * if the framework install dir is relative, it will be appended to the install prefix
> > > 
> > > > Then, if you want to copy the to-be-installed files into a staging area first, you're expected to do that with "DESTDIR=whatever ninja install".
> > > 
> > > [Clang cache scripts](https://github.com/llvm/llvm-project/blob/a8f88c38/clang/cmake/caches/Apple-stage1.cmake#L4) seem to accomplish it manually, which may look like this (but the default would again fail due to privileges):
> > > ```
> > > if($ENV{DESTDIR})
> > >   set(CMAKE_INSTALL_PREFIX $ENV{DESTDIR})
> > >   set(LLDB_FRAMEWORK_INSTALL_DIR "../../SharedFrameworks" CACHE STRING "")
> > > else()
> > >   set(CMAKE_INSTALL_PREFIX /Applications/Xcode.app/Contents/Developer/usr)
> > >   set(LLDB_FRAMEWORK_INSTALL_DIR /Applications/Xcode.app/Contents/SharedFrameworks CACHE STRING "")
> > > endif()
> > > ```
> > > 
> > > Would you (and other reviewers) prefer this solution?
> > > But then ninja install would fail by default due to lack of permissions in the install destination. Actual release configurations tend to be more complex anyway and vendors will have their own downstream repos / caches for it. Thus, choosing a good default for developers sounded like a good way forward. What do you think?
> > 
> > I don't think most developers actually run the "install" rule TBH. :) But if they do, I think we should encourage them to do the right thing, and run "DESTDIR=foo ninja install", which will work even with a real-world prefix. At least that is the right thing to do in the linuxy world -- on a mac I guess most people will not be able to install lldb to the system destination anyway, so I'm not sure what would be a sensible default.
> > 
> > > Would you (and other reviewers) prefer this solution?
> > 
> > I don't care that much about this TBH. I just wanted to explain the difference between the install prefix and destdir, because in my experience, a lot of people get those two mixed up. ccing  @mgorny, in case I'm saying something wrong, as he does a lot of building and installing..
> > I just wanted to explain the difference between the install prefix and destdir
> 
> Yes, that was a good point: DESTDIR is the location of the install-tree on the build machine, PREFIX is the install location on the enduser machine. Right? Sorry, I didn't come back to it. The Clang cache script I referred to, doesn't respect that.
The example you posted above using the clang cache file is quite sketchy imo. I recommend reading the page on DESTDIR, as I think it clears up some things: https://cmake.org/cmake/help/latest/envvar/DESTDIR.html

Taking the example above that you posted, if you set CMAKE_INSTALL_PREFIX to $ENV{DESTDIR}, you could end up with some strange results. For example, let DESTDIR="/Users/foo/". Installing something would mean that it would end up relative to the path "/Users/foo/Users/foo", which is probably not what you want. DESTDIR is a way of relocating the entire install tree to a non-default location. It in some sense "re-roots" your install tree.

As for using `CMAKE_CURRENT_BINARY_DIR` inside the `CMAKE_INSTALL_PREFIX` and `LLDB_FRAMEWORK_INSTALL_DIR`, I would instead rely on DESTDIR here and make `CMAKE_INSTALL_PREFIX` something like `Developer/usr` and `LLDB_FRAMEWORK_INSTALL_DIR` something like `SharedFrameworks`.

================
Comment at: lldb/cmake/caches/Apple-lldb-macOS.cmake:18
+set(CMAKE_BUILD_TYPE RelWithDebInfo)
+set(LLVM_ENABLE_MODULES ON CACHE BOOL "")
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61956