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

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added inline comments.


================
Comment at: clang/lib/Headers/CMakeLists.txt:516
   COMPONENT cuda-resource-headers)
 
 install(
----------------
tra wrote:
> 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.
Thanks for the changes! One thing I realized after seeing the most recent update is that I mixed up the term "install target" with "install rules" in my previous comments. Apologies for the confusion. 

I think we will need the following new target code only if we intend to specify `cuda-resource-bits-headers` as a separate installation target (e.g. if we do `cmake -DCOMPONENT=cuda-resource-bits-headers -P ./cmake_install.cmake`). 

```
add_header_target("cuda-resource-bits-headers" "${cuda_wrapper_bits_files}")
add_dependencies("clang-resource-headers"
                  ... 
                  "cuda-resource-bits-headers"
                 ...
add_llvm_install_targets(install-cuda-resource-bits-headers
                         DEPENDS cuda-resource-bits-headers
                        COMPONENT cuda-resource-headers)
```
In other words, we do not need these three code changes above if we do not intend to install `cuda-resource-bits-headers` as a standalone target. If our intention is to always use it as a component of `cuda-resource-headers`, adding install rule
```
install(
   FILES ${cuda_wrapper_bits_files}
   DESTINATION ${header_install_dir}/cuda_wrappers/bits
   EXCLUDE_FROM_ALL
   COMPONENT cuda-resource-headers)
```
should be sufficient when installing `cuda-resource-headers` by itself. We still need the install rule for `clang-resource-headers` which was initially added and is still necessary. I tested the installation without the extra targets but with the rule and it seems to work as intended. 

My suggestion is that we can remove the new targets if we do not intend to install `cuda-resource-bits-headers` as a standalone component. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151503