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