[PATCH] D151503: [CUDA] correctly install cuda_wrappers/bits/shared_ptr_base.h

[Artem Belevich via Phabricator via cfe-commits]

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