phosek added inline comments.
================
Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:66
install(FILES "${LIBCXX_BINARY_INCLUDE_DIR}/${fpath}"
- DESTINATION ${LIBCXX_INSTALL_HEADER_PREFIX}include/c++/v1/${dstdir}
+ DESTINATION
${LIBCXX_INSTALL_HEADER_PREFIX}${CMAKE_INSTALL_INCLUDEDIR}/c++/v1/${dstdir}
COMPONENT cxx-headers
----------------
phosek wrote:
> ldionne wrote:
> > Ericson2314 wrote:
> > > ldionne wrote:
> > > > compnerd wrote:
> > > > > Ericson2314 wrote:
> > > > > > compnerd wrote:
> > > > > > > @ldionne - how is the `LIBCXX_INSTALL_HEADER_PREFIX` used? Can
> > > > > > > altering the value of `CMAKE_INSTALL_INCLUDEDIR` serve the
> > > > > > > purpose?
> > > > > > It is sometimes modified to be per target when multiple targets are
> > > > > > being used at once. All things `CMAKE_INSTALL_*` are globally
> > > > > > scoped so in general the combination builds are quite awkward.
> > > > > >
> > > > > > (Having worked on Meson, I am really missing
> > > > > > https://mesonbuild.com/Subprojects.html which is exactly what's
> > > > > > needed to do this without these bespoke idioms that never work well
> > > > > > enough . Alas...)
> > > > > I don't think that bringing up other build systems is particularly
> > > > > helpful.
> > > > >
> > > > > I do expect it to be modified, and I suspect that this is used
> > > > > specifically for builds that @ldionne supports.
> > > > Actually, I've never used it myself, but @phosek seems to be using it
> > > > for the Runtimes build to output one set of headers for each target, as
> > > > mentioned above.
> > > >
> > > > It seems to me that tweaking `CMAKE_INSTALL_PREFIX` globally when
> > > > driving the libc++ build from the runtimes build would be more in-line
> > > > with the CMake way of doing things (one configuration == one build),
> > > > but I'm curious to hear what @phosek has to say about that.
> > > > It seems to me that tweaking CMAKE_INSTALL_PREFIX globally when driving
> > > > the libc++ build from the runtimes build would be more in-line with the
> > > > CMake way of doing things (one configuration == one buid)
> > >
> > > You mean trying to mutate it during the libc++ CMake eval and then set it
> > > back after? That would keep the intended meaning of
> > > `CMAKE_INSTALL_PREFIX` being the actual prefix, but that feels awfully
> > > fragile to me. Or do you mean something else?
> > I keep forgetting that the runtimes build uses `add_subdirectory` to
> > include each sub-project instead of driving a proper CMake build for each
> > of those.
> >
> > Basically, what I'm saying is that whatever place we decide to build for N
> > multiple targets, we should perform N different CMake builds setting the
> > appropriate `CMAKE_INSTALL_PREFIX`, one for each target. But that
> > discussion should happen elsewhere, not on this review.
> >
> > As far as this review is concerned, I do believe you want instead:
> >
> > ```
> > ${CMAKE_INSTALL_INCLUDEDIR}${LIBCXX_INSTALL_HEADER_PREFIX}
> > ```
> >
> > (reversed the order of variables)
> >
> > We should have @phosek confirm.
> @ldionne `CMAKE_INSTALL_PREFIX` cannot be used for this purpose. When using
> the multiarch layout with the runtimes build, we want to place build
> artifacts in `${BUILD_DIR}/{include,lib}/<target>/...` and install them to
> `${CMAKE_INSTALL_PREFIX}/{include,lib}/<target>/...`. There are two issues:
> 1. `CMAKE_INSTALL_PREFIX` only comes into play during install step, but we
> want the build layout to match the installation layout so we need a different
> variable to control the output directory.
> 2. We already use `CMAKE_INSTALL_PREFIX` for the toolchain itself and there's
> no way to use it hierarchically, that is you cannot do something like
> `${SUPER_CMAKE_INSTALL_PREFIX}/{include,lib}/${CMAKE_INSTALL_PREFIX}/...`
> which is why we need a separate variable to control the installation
> directory.
>
> Regarding `LIBCXX_INSTALL_HEADER_PREFIX`, I haven't found any current uses of
> that variable. I introduced it in D59168 which was an earlier attempt at
> having per-target headers, but D89013 is strictly better for the reasons I
> described in https://reviews.llvm.org/D89013#2662578 (no duplicate headers)
> so I think we can just remove this variable.
I have sent D99697 for review which removes these variables.
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
