[libcxx-commits] [PATCH] D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/clang/ and include/

[Joel E. Denny via Phabricator via libcxx-commits]

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