compnerd added inline comments.
================ Comment at: bolt/lib/RuntimeLibs/RuntimeLibrary.cpp:32 SmallString<128> LibPath = llvm::sys::path::parent_path(Dir); - llvm::sys::path::append(LibPath, "lib" LLVM_LIBDIR_SUFFIX); + llvm::sys::path::append(LibPath, CMAKE_INSTALL_LIBDIR); if (!llvm::sys::fs::exists(LibPath)) { ---------------- Ericson2314 wrote: > compnerd wrote: > > serge-sans-paille wrote: > > > Ericson2314 wrote: > > > > mgorny wrote: > > > > > Well, one thing I immediately notice is that technically > > > > > `CMAKE_INSTALL_LIBDIR` can be an absolute path, while in many > > > > > locations you are assuming it will always be relative. Not saying > > > > > that's a blocker but I think you should add checks for that into > > > > > `CMakeLists.txt`. > > > > Yes I was working on this, and I intend to use `CMAKE_INSTALL_LIBDIR` > > > > with an absolute path. > > > > > > > > OK if this isn't supported initially but we should ovoid regressing > > > > where possible. > > > Turns out LLVM_LIBDIR_SUFFIX is used in several situations: (a) for the > > > library install dir or (b) for the location where build artifact as > > > stored in CMAKE_BINARY_DIR. (a) could accept a path but not (b), unless > > > we derive it from (a) but I can see a lot of complication there for the > > > testing step. Would that be ok to choke on CMAKE_INSTALL_LIBDIR being a > > > path and not a directory component? > > > > > I think that is absolutely reasonable. There is the > > `CMAKE_INSTALL_FULL_LIBDIR` which should be the relatively absolute path > > (it is relative to `DESTDIR`). The `CMAKE_INSTALL_LIBDIR` should be the > > relative component which is added to `CMAKE_INSTALL_PREFIX`. > Please do not do this. In NixOS we uses absolute paths for these which are > *not* within `CMAKE_INSTALL_PREFIX` and I would very much like that to > continue to work, and have put a lot of effort into it. > > (I am sorry I have been a bit AWAL on LLVM things in general but hopefully > will have more time as we head into the new year.) Why can NixOS not use `CMAKE_INSTALL_FULL_LIBDIR`? That is computed to `${CMAKE_INSTALL_PREFIX}${CMAKE_INSTALL_LIBDIR}` only if it is not already defined to an absolute path. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137337/new/ https://reviews.llvm.org/D137337 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits