jdenny accepted this revision.
jdenny added a comment.

In D59168#1423587 <https://reviews.llvm.org/D59168#1423587>, @phosek wrote:

> I'm not super enthusiastic about the duplicated triple, but the only way to 
> eliminate it would be to move the Clang resource directory inside of 
> `lib/clang/x86_64-linux-gnu`, i.e. we'd have 
> `lib/clang/x86_64-linux-gnu/9.0.0/{include,lib}`.


`lib/x86_64-linux-gnu/clang/9.0.0/{include,lib}` would mean less duplication of 
the triple within system directories.  Is there a reason not to do that?  I'm 
not necessarily advocating for this.  I'm just trying to understand what you've 
chosen.

In D59168#1425701 <https://reviews.llvm.org/D59168#1425701>, @phosek wrote:

> In D59168#1424968 <https://reviews.llvm.org/D59168#1424968>, @smeenai wrote:
>
> > I don't think the duplicated triple is too huge of a deal. I think the 
> > layout where the resource directory is moved inside the triple directory is 
> > a bit nicer, but I also don't know how much work that change would be and 
> > if it's worth it.
>
>
> One downside is that we would be duplicating Clang resource headers for each 
> target which may not be too big of a deal but something worth mentioning.


In that case, could we symlink the target-specific include directories to a 
common directory?

Regardless of these points, your patch appears to strictly improve the 
situation, so maybe we should see how things go with it and then make further 
changes later if desired.

So, LGTM modulo the question I added inline.  Thanks.



================
Comment at: libcxx/lib/CMakeLists.txt:407
   install(TARGETS ${LIBCXX_INSTALL_TARGETS} ${filesystem_lib} 
${experimental_lib}
-    LIBRARY DESTINATION ${LIBCXX_INSTALL_PREFIX}lib${LIBCXX_LIBDIR_SUFFIX} 
COMPONENT cxx
-    ARCHIVE DESTINATION ${LIBCXX_INSTALL_PREFIX}lib${LIBCXX_LIBDIR_SUFFIX} 
COMPONENT cxx
+    LIBRARY DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} 
COMPONENT cxx
+    ARCHIVE DESTINATION ${LIBCXX_INSTALL_PREFIX}${LIBCXX_INSTALL_LIBRARY_DIR} 
COMPONENT cxx
----------------
Assume LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True.

Without your patch, it looks to me like specifying LIBCXX_INSTALL_PREFIX 
collapses the library destination from `$prefix/lib/clang/$version/$triple/lib` 
to `$prefix/lib`.

With your patch, it looks to me like the library destination is always 
`$prefix/lib/clang/$triple`, and specifying LIBCXX_INSTALL_PREFIX simply 
changes `$prefix`.

Is that correct?  If so, is that worthwhile to note in the summary?


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

https://reviews.llvm.org/D59168



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

Reply via email to