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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
