Ericson2314 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)) {
----------------
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.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137337/new/

https://reviews.llvm.org/D137337

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to