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