This is an archive of the discontinued LLVM Phabricator instance.

[bazel] Add missing C++ style clang headers and modulemap
ClosedPublic

Authored by aaronmondal on Oct 21 2022, 6:56 AM.

Details

Summary

These appear to have been overlooked.

Diff Detail

Event Timeline

aaronmondal created this revision.Oct 21 2022, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 6:56 AM
Herald added subscribers: mattd, yaxunl. · View Herald Transcript
aaronmondal requested review of this revision.Oct 21 2022, 6:56 AM
Herald added a project: Restricted Project. · View Herald Transcript

As I don't have commit access, It'd be nice if someone could commit on my behalf.
Name: Aaron Siddhartha Mondal
Email: aaron@eomii.org

aaronmondal retitled this revision from [bazel] Add missing clang CUDA wrappers to [bazel] Add missing C++ style clang headers.
aaronmondal edited the summary of this revision. (Show Details)

Well, of course we want to ADD these headers, not exclude them 😅

Move these headers into glob to clearly differentiate generated and non-generated files.

FYI, I intend that builtin_headers_gen may be available for the target for packaging.
Its contents shall be the same (or subset) of the installaed image by CMake.

utils/bazel/llvm-project-overlay/clang/BUILD.bazel
1478

Seems odd to pass non-wildcard files to glob.

Instead, could we reorganize the pattern? Like,

  • lib/Headers/*.h
  • lib/Headers/cuda_wrappers
  • lib/Headers/openmp_wrappers
  • lib/Headers/ppc_wrappers

(clang/lib/Headers/hlsl may be excluded here for now, since HLSL is not enabled in bazel build)

Address comments.

FYI, I intend that builtin_headers_gen may be available for the target for packaging.
Its contents shall be the same (or subset) of the installaed image by CMake.

Ah now it makes sense to me why we are excluding two headers.

Regarding the updated diff, it will work, but may go slightly against the guideline in https://bazel.build/reference/be/functions#glob, stating

In general, you should try to provide an appropriate extension (e.g. *.html) instead of using a bare '*' for a glob pattern. The more explicit name is both self documenting and ensures that you don't accidentally match backup files, or emacs/vi/... auto-save files.

I don't think this is an issue in this particular case though.

Add missing comma.

aaronmondal added inline comments.Oct 24 2022, 3:37 PM
utils/bazel/llvm-project-overlay/clang/BUILD.bazel
1478

There would also be the possibility to use (note the last line)

"lib/Headers/*.h",
"lib/Headers/cuda_wrappers/*",
"lib/Headers/openmp_wrappers/*",
"lib/Headers/ppc_wrappers/*.h",

but I think just using the asterisks in this case should be clear enough. Omitting the asterisks won't work.

Excuse me, I forgot trailer asterisks in my previous comment.

Regarding the updated diff, it will work, but may go slightly against the guideline in https://bazel.build/reference/be/functions#glob, stating

In general, you should try to provide an appropriate extension (e.g. *.html) instead of using a bare '*' for a glob pattern. The more explicit name is both self documenting and ensures that you don't accidentally match backup files, or emacs/vi/... auto-save files.

I don't think this is an issue in this particular case though.

I have rethought it should be an issue. We should not use glob for C++ suffix-less headers.
Your Diff 2 would be sufficient for now, I think.

Or shall we expand globbing?

@chapuni I think fully expanding the glob does not really gain us anything. It would only add ~300 lines of headers, which I would assume to not really impact builds anyways, except if a header is missing as in openmp_headers/time.h. Keeping those up to date would likely be unnecessary maintenance burden.

I'll revert to Diff2 and add the module.modulemap as well, which is also part of the core files exported by the CMake build.

aaronmondal retitled this revision from [bazel] Add missing C++ style clang headers to [bazel] Add missing C++ style clang headers and modulemap.

Address comments. Add module.modulemap as well, since it is always exported by the CMake build.

chapuni accepted this revision.Oct 26 2022, 6:45 AM

@aaronmondal Thank you for working. If you push your branch anywhere, I could fetch and commit it.

This revision is now accepted and ready to land.Oct 26 2022, 6:45 AM

I can't get arcanist or something else to work. As i don't have commit access, I pushed the changes to this git repo: https://github.com/aaronmondal/llvm-project/commit/652afbe8aff0dbf45d2d07ef5b74fdce137269e3

@chapuni maybe this here would be better? I can't get this to run, but maybe they work for you with

arc patch D136452
git commit --amend --author="Aaron Siddhartha Mondal <aaron@eomii.org>"
This revision was automatically updated to reflect the committed changes.