Ericson2314 marked 10 inline comments as done. Ericson2314 added a comment.
The approval on this patch is quite old, but nothing much interesting has happened in it since then --- if anything, it has gotten more trivial as I created patches "under" it and landed them which cleared the way for this. The build failure in the libc++ "modular build" I also see in other CI runs, e.g. https://buildkite.com/llvm-project/libcxx-ci/builds/7855#b40f8ad6-dc6a-4589-81d7-a459dae8b7e7, so I it appears unrelated. I double checked and old lingering conversations (pre approval) can be marked resolved. ================ Comment at: clang-tools-extra/clang-doc/tool/CMakeLists.txt:26 install(FILES ../assets/index.js - DESTINATION share/clang + DESTINATION "${CMAKE_INSTALL_DATADIR}/clang" COMPONENT clang-doc) ---------------- Ericson2314 wrote: > Ericson2314 wrote: > > compnerd wrote: > > > Why are these quoted but other uses not? > > I confess I have no clue when quoting is required or advisable with CMake. > > I started out converting things by hand and then did some auto-conversions, > > this must have been one of the by-hand ones. > > > > Happy to normalize either way, just tell me which one. > I looked it up, > https://stackoverflow.com/questions/35847655/when-should-i-quote-cmake-variables > says I'm slightly better off quoting all of these so I did. Since then I landed D115566, which made the quoting consistent prior to this. This adds quotes defensively around variable expansions, and just needs to where the path expression didn't involve variables before. ================ 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: > > > 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. > Yes. `CMAKE_INSTALL_LIBDIR` is no longer introduced in this patch; this has been the case since before the approval. ================ Comment at: libcxx/CMakeLists.txt:32 + +include(GNUInstallDirs) ---------------- Ericson2314 wrote: > 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. `include(GNUInstallDirs)` is no longer in funny locations like this. ================ Comment at: polly/cmake/CMakeLists.txt:82-96 +set(POLLY_INSTALL_PREFIX "") set(POLLY_CONFIG_LLVM_CMAKE_DIR "${LLVM_BINARY_DIR}/${LLVM_INSTALL_PACKAGE_DIR}") -set(POLLY_CONFIG_CMAKE_DIR "${POLLY_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}") -set(POLLY_CONFIG_LIBRARY_DIRS "${POLLY_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}") +set(POLLY_CONFIG_CMAKE_DIR "${POLLY_INSTALL_PREFIX}${CMAKE_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}") +set(POLLY_CONFIG_LIBRARY_DIRS "${POLLY_INSTALL_PREFIX}${CMAKE_INSTALL_FULL_LIBDIR}${LLVM_LIBDIR_SUFFIX}") if (POLLY_BUNDLED_ISL) set(POLLY_CONFIG_INCLUDE_DIRS + "${POLLY_INSTALL_PREFIX}${CMAKE_INSTALL_FULL_LIBDIR}" ---------------- Meinersbur wrote: > Ericson2314 wrote: > > `POLLY_INSTALL_PREFIX`, like `COMPILER_RT_INSTALL_PATH`, I changed to be > > empty by default. If the `COMPILER_RT_INSTALL_PATH` can be removed, maybe > > `POLLY_INSTALL_PREFIX` can also be removed? > I don't have an overview atm on Polly's different paths, but could look into > it if needed. Originally, this was derived from how Clang did it. If it works > for COMPILER_RT_INSTALL_PATH, it should work for Polly as well. Polly was made like the others with D116555. There is no issue here anymore. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99484/new/ https://reviews.llvm.org/D99484 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
