qiongsiwu1 added inline comments.
================
Comment at: clang/lib/Headers/CMakeLists.txt:516
COMPONENT cuda-resource-headers)
install(
----------------
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`.
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
