Ericson2314 added inline comments.
================
Comment at: clang/cmake/modules/AddClang.cmake:127
+ LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+ ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}
+ RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
----------------
compnerd wrote:
> Ericson2314 wrote:
> > compnerd wrote:
> > > For the initial change, Id leave this off. `CMAKE_INSTALL_LIBDIR`
> > > sometimes already deals with the bit suffix, so you can end up with two
> > > instances of `32` or `64`. I think that cleaning that up separately,
> > > while focusing on the details of cleaning up how to handle
> > > `LLVM_LIBDIR_SUFFIX` is the right thing to do. The same applies to the
> > > rest of the patch.
> > https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/GNUInstallDirs.cmake#L257
> > Hmm I see what you mean. So you are saying `s/${
> > CMAKE_INSTALL_LIBDIR}${LLVM_LIBDIR_SUFFIX}/${ CMAKE_INSTALL_LIBDIR}/`
> > everywhere?
> Yes, that is what I was referring to. I'm suggesting that you do *not* make
> that change instead. That needs a much more involved change to clean up the
> use of `${LLVM_LIBDIR_SUFFIX}`. I think that this change is already
> extremely large as is, and folding more into it is not going to help.
So you are saying II should back all of these out into
`lib${LLVM_LIBDIR_SUFFIX}` as they were before, for now?
Yes I don't want to make this bigger either, and would rather be on the hook
for follow-up work than have this one be too massive to get over the finish
line.
================
Comment at: compiler-rt/cmake/Modules/CompilerRTUtils.cmake:389
get_compiler_rt_target(${arch} target)
- set(${install_dir} ${COMPILER_RT_INSTALL_PATH}/lib/${target} PARENT_SCOPE)
+ set(${install_dir}
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_LIBDIR}/${target} PARENT_SCOPE)
else()
----------------
ldionne wrote:
> lebedev.ri wrote:
> > This looks suspect
> Yeah, I don't understand why this isn't just `CMAKE_INSTALL_LIBDIR` like
> elsewhere.
See the non-line comment I responded to @lebidev.ri with. In sort if
```
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_LIBDIR}/${target}
```
is a relative path, then we end up with
```
${CMAKE_INSTALL_PREFIX}/${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_LIBDIR}/${target}
```
instead of
```
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/${target}
```
as we do with the other per-package prefixes. Also if `CMAKE_INSTALL_LIBDIR` is
already an absolute path, then
```
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_LIBDIR}/${target}
```
is the same thing, and closer to the second than the first.
================
Comment at: compiler-rt/cmake/base-config-ix.cmake:72
+ set(COMPILER_RT_INSTALL_PATH "" CACHE PATH
+ "Prefix where built compiler-rt artifacts should be installed, comes
before CMAKE_INSTALL_PREFIX.")
option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit
tests." OFF)
----------------
compnerd wrote:
> Ericson2314 wrote:
> > compnerd wrote:
> > > Please don't change the descriptions of the options as part of the
> > > `GNUInstallDirs` handling. The change to `COMPILER_RT_INSTALL_PATH`
> > > looks incorrect. Can you explain this change please?
> > I tried explain this https://reviews.llvm.org/D99484#2655885 and the
> > original description about prefixes. The GNU install dir variables are
> > allowed to be absolute paths (and not necessary with the installation
> > prefix) (and I needed that for the NixOS patch), so always prepending
> > `COMPILER_RT_INSTALL_PATH` as is done doesn't work if that is
> > `CMAKE_INSTALL_PREFIX` by default.
> >
> > If you do `git grep '_PREFIX ""'` and also `git grep GRPC_INSTALL_PATH` you
> > will see that many similar variables also were already empty by default. I
> > agree this thing is a bit ugly, but by that argument I am not doing a new
> > hack for `GNUInstallDIrs` but rather applying an existing ideom
> > consistently in all packages.
> Sure, but the *descriptions* of the options are needed to changed for
> changing the value.
>
> That said, I'm not convinced that this change is okay. It changes the way
> that compiler-rt can be built and used when building a toolchain image. The
> handling of the install prefix in compiler-rt is significantly different from
> the other parts of LLVM, and so, I think that it may need to be done as a
> separate larger effort.
So just skip everything compile-rt related for now?
================
Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
----------------
compnerd wrote:
> Ericson2314 wrote:
> > compnerd wrote:
> > > Does this need to come here? Why not push this to after the if block
> > > completes? The same applies through out the rest of the patch.
> > It might be fine here. But I was worried that in some of these cases code
> > included in those blocks might refer to the `GNUInstallDirs` variables.
> >
> > Originally I had `GNUInstallDirs` only included in the conditional block
> > after `project(...)`, but then the combined build test failed because
> > nothing including `GnuInstallDirs` in that case. If there is an
> > "entrypoint" CMakeLists boilerplate that combined builds should always use,
> > I think the best thing would be to change it back and then additionally put
> > `GNUInstallDirs` there.
> Unified builds always use `llvm/CMakeLists.txt`. However, both should
> continue to work, and so you will need to add this in the subprojects as well.
Huh. That one always had the unconditional `include(GNUInstalDirs)`, so not
sure why the CI build was failing before.
================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:66
install(FILES "${LIBCXX_BINARY_INCLUDE_DIR}/${fpath}"
- DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1/${dstdir}
+ DESTINATION
${LIBCXX_INSTALL_HEADER_PREFIX}${CMAKE_INSTALL_INCLUDEDIR}/c++/v1/${dstdir}
COMPONENT cxx-headers
----------------
ldionne wrote:
> compnerd wrote:
> > Ericson2314 wrote:
> > > compnerd wrote:
> > > > @ldionne - how is the `LIBCXX_INSTALL_HEADER_PREFIX` used? Can
> > > > altering the value of `CMAKE_INSTALL_INCLUDEDIR` serve the purpose?
> > > It is sometimes modified to be per target when multiple targets are being
> > > used at once. All things `CMAKE_INSTALL_*` are globally scoped so in
> > > general the combination builds are quite awkward.
> > >
> > > (Having worked on Meson, I am really missing
> > > https://mesonbuild.com/Subprojects.html which is exactly what's needed to
> > > do this without these bespoke idioms that never work well enough .
> > > Alas...)
> > I don't think that bringing up other build systems is particularly helpful.
> >
> > I do expect it to be modified, and I suspect that this is used specifically
> > for builds that @ldionne supports.
> Actually, I've never used it myself, but @phosek seems to be using it for the
> Runtimes build to output one set of headers for each target, as mentioned
> above.
>
> It seems to me that tweaking `CMAKE_INSTALL_PREFIX` globally when driving the
> libc++ build from the runtimes build would be more in-line with the CMake way
> of doing things (one configuration == one build), but I'm curious to hear
> what @phosek has to say about that.
> It seems to me that tweaking CMAKE_INSTALL_PREFIX globally when driving the
> libc++ build from the runtimes build would be more in-line with the CMake way
> of doing things (one configuration == one buid)
You mean trying to mutate it during the libc++ CMake eval and then set it back
after? That would keep the intended meaning of `CMAKE_INSTALL_PREFIX` being
the actual prefix, but that feels awfully fragile to me. Or do you mean
something else?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99484/new/
https://reviews.llvm.org/D99484
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
