Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[bazel] Add filegroup for builtin_headers#67757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
dzbarsky wants to merge1 commit intollvm:main
base:main
Choose a base branch
Loading
fromdzbarsky:main

Conversation

dzbarsky
Copy link

I'd like to package these files into a distribution tar as part ofhttps://github.com/dzbarsky/static-clang. I'm currently applying patches to the llvm repo but figured this bit could be upstreamed.

(Also open to ideas what to do about theconfig.bzl change inhttps://github.com/dzbarsky/static-clang/blob/master/llvm.patch - it's needed to link with musl libc)

@dzbarsky
Copy link
Author

@jathu I saw you were touching the same area

@jathu
Copy link
Contributor

Why can't you directly use the outs of the genrule?

@dzbarsky
Copy link
Author

I could. It would require the following change on my side:

 pkg_files(     name = "builtin_headers_pkg_files",     srcs = [-        "@llvm-project//clang:builtin_headers_files",+        "@llvm-project//clang:builtin_headers_gen",     ],-    strip_prefix = "lib/Headers",+    strip_prefix = strip_prefix.from_pkg() + "staging/include/",     prefix = "lib/clang/%s/include" % LLVM_VERSION_MAJOR,

This seems more brittle, it will break with your PR. Also that genrule takes 17 seconds in my setup to copy the files around, which is a waste since I'm just going to package them up anyway. I'd probably stick with my local patch in that case.

Any reason not to add this?

@dzbarsky
Copy link
Author

Oh I guess I misread and it won't change with your PR, I would probably need to update to use the const though. Either way, I think the point is the filegroup is simpler and faster.

@jathu
Copy link
Contributor

Do you think you will still need this once we land#67626? The clang-tidy binary will have the header files as runfiles, which you can just package directly

Also that genrule takes 17 seconds in my setup to copy the files around, which is a waste since I'm just going to package them up anyway. I'd probably stick with my local patch in that case. Any reason not to add this?

No, I don't see any reason not to add this. I assumed the filegroup was redundant since the genrule would cache the subsequent uses anyways

@dzbarsky
Copy link
Author

Do you think you will still need this once we land#67626? The clang-tidy binary will have the header files as runfiles, which you can just package directly

I'm building the entire tar package within Bazel and I'm not packaging clang-tidy (you don't need it for a minimal toolchain) so I don't think it helps.

Also that genrule takes 17 seconds in my setup to copy the files around, which is a waste since I'm just going to package them up anyway. I'd probably stick with my local patch in that case. Any reason not to add this?

No, I don't see any reason not to add this. I assumed the filegroup was redundant since the genrule would cache the subsequent uses anyways

I think something about my compilation setup is non-hermetic, because when executing from a github actions runner (with RBE/remote caching) I am seeing tons of remote misses. When running from a minimal docker image on my laptop (also against RBE) I am seeing everything cached, so I suspect we are somehow leaking the repo path, which is probably randomized on github. Anyway, this is a total aside, but the caching doesn't work as well as I'd like :)

jathu reacted with thumbs up emoji

@EndilllEndilll added the bazel"Peripheral" support tier build system: utils/bazel labelJul 14, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bazel"Peripheral" support tier build system: utils/bazel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@dzbarsky@jathu@Endilll

[8]ページ先頭

©2009-2025 Movatter.jp