compnerd 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})
----------------
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.
================
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)
----------------
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.
================
Comment at: libcxx/CMakeLists.txt:32
+
+include(GNUInstallDirs)
----------------
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.
================
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
----------------
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.
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
