Ericson2314 added inline comments.
================ 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: > > 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? compile-rt is skipped in the latest version, and I have an independent https://reviews.llvm.org/D101497 laying the groundwork for a future `GNUInstallDirs`-adding patch to do the regular thing. 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
