tra added inline comments.
================
Comment at: clang/lib/Headers/CMakeLists.txt:516
COMPONENT cuda-resource-headers)
install(
----------------
qiongsiwu1 wrote:
> qiongsiwu1 wrote:
> > tra wrote:
> > > qiongsiwu1 wrote:
> > > > Do we need an install target for `${cuda_wrapper_bits_files}` for the
> > > > `cuda-resource-headers` component as well? It seems to be the case
> > > > because this patch is treating `${cuda_wrapper_bits_files}` as part of
> > > > `cuda-resource-headers`.
> > > >
> > > > ```
> > > > add_header_target("cuda-resource-headers"
> > > > "${cuda_files};${cuda_wrapper_files};${cuda_wrapper_bits_files}")
> > > > ```
> > > >
> > > >
> > > I'm not sure I understand the question. Are you saying that a separate
> > > `install()` for the 'bits' is not necessary and we could just install all
> > > headers with a single `install` above?
> > >
> > > If that's the case, then, AFAICT, the answer is that we do need a
> > > separate `install`.
> > > `install(FILES)` does not preserve the directory structure and dumps all
> > > files listed in `FILES`, regardless if they are in different directories
> > > into the same DESTINATION directory.
> > > That is exactly the problem this patch is intended to fix. We do need to
> > > place the file under `cuda_wrappers/bits/` directory and that's why we
> > > have separate `install(DESTINATION
> > > ${header_install_dir}/cuda_wrappers/bits)` here.
> > >
> > > `install(DIRECTORY)` would presumably preserve the source directory
> > > structure, but we lose per-file granularity. It may work for the files
> > > under cuda_wrappers for now, but I think there's some merit in explicitly
> > > controlling which headers we ship and where we put them. While we do have
> > > 1:1 mapping between the source tree and install tree, it may not always
> > > be the case.
> > >
> > >
> > >
> > Ah sorry for the confusion.
> >
> > > Are you saying that a separate install() for the 'bits' is not necessary
> > > and we could just install all headers with a single install above?
> >
> > No I am trying to say the opposite. I am suggesting we //add// the separate
> > install target as a component of `clang-resource-headers` //and// as a
> > component of `cuda-resource-headers`, as shown in the code change suggested
> > in the comment above. I am not suggesting any code form this patch to be
> > removed. The `cuda-resource-headers` can be used to install the cuda
> > related headers only, in the case when a user do not want to install all
> > the headers (e.g. if a user only want to install support for Intel and
> > Nvidia headers, but not the PowerPC headers, the user can select
> > `core-resource-headers`, `x86_files` and `cuda-resource-headers` during a
> > distribution build/install). I think without the code change suggested
> > above, if a user select to install `cuda-resource-headers` only without
> > specifying `clang-resource-headers`, we will miss the file
> > `cuda_wrappers/bits/shared_ptr_base.h`.
> Sorry I made a typo in the previous comment. I meant `x86-resource-headers`
> when I said `x86_files`.
I think understand now.
`cmake -DCOMPONENT=cuda-resource-headers -P ./cmake_install.cmake` indeed does
not install the bits component.
I've added the install with `COMPONENT clang-resource-headers` and verified
that the bits header is installed during individual component installation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151503/new/
https://reviews.llvm.org/D151503
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
