These appear to have been overlooked.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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
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,
(clang/lib/Headers/hlsl may be excluded here for now, since HLSL is not enabled in bazel build) |
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.
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.
Address comments. Add module.modulemap as well, since it is always exported by the CMake build.
@aaronmondal Thank you for working. If you push your branch anywhere, I could fetch and commit it.
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>"
Seems odd to pass non-wildcard files to glob.
Instead, could we reorganize the pattern? Like,
(clang/lib/Headers/hlsl may be excluded here for now, since HLSL is not enabled in bazel build)