This patch provides a draft overlay to support compilation of llvm libc with Bazel.
Tested on linux x86-64 with
cd git/llvm-project/utils/bazel bazelisk-linux-amd64 build --sandbox_base=/dev/shm --config=generic_clang @llvm-project//libc:all
Details
Details
- Reviewers
sivachandra michaelrj GMNGeoffrey - Commits
- rG8ed70d0189ba: [libc] Bazel overlay for libc
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Overall LGTM but one question: Will the file libc/BUILD.bazel interfere with downstream BUILD files?
Comment Actions
I assume you're talking about Google's monorepo. No, it won't.
| utils/bazel/llvm-project-overlay/libc/fenv_targets.bzl | ||
|---|---|---|
| 9 ↗ | (On Diff #390374) | I'm confused why this would be pulled out into a macro rather than just defined as build rules directly in the BUILD.bazel file. This sort of indirection defeats tooling aimed at automated build file maintenance and makes it harder to inspect build files or understand where some target is defined. These macros also aren't really general purpose and require the other macros to have defined specific targets in order to function. I'm guessing the goal is to avoid a massive build file and organize things for readability. I think some of the typical reasons for factoring things are less applicable in build rules. As another idea, could you instead place a build file in src/fenv? It seems like all these rules use sources in src/fenv. This would be a much more natural way to factor things (similar for the other such macros). The only reason the LLVM and MLIR build files are one giant file is that the lib/ and include/ split make it difficult to factor things since Bazel defines packages by directories. See, for reference https://docs.bazel.build/versions/main/skylark/bzl-style.html#macros |
| utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl | ||
| 12 | Please use **kwargs, which is used in all other macros in the project. | |
| 27–30 | I think it would make more sense to make this a named kwarg in the function signature | |
| 36 | Normally, you'd use a _ prefix for implementation-detail rules (as recommended by the style guide) | |
| 50 | Is this function used anywhere? | |
| utils/bazel/llvm-project-overlay/libc/math_targets.bzl | ||
| 26 ↗ | (On Diff #390374) | This macro will make understanding what srcs and deps a rule has more complicated. It may be worth the tradeoff, just consider https://docs.bazel.build/versions/main/skylark/bzl-style.html#macros when making this decision please |
| 33–39 ↗ | (On Diff #390374) | I think this is equivalent, unless you want to use this opportunity to add error checking if a specialization is none of those things |
Comment Actions
Nice! Thanks for iterating on this. It looks like the Bazel build has been broken since yesterday evening. The error looks unrelated to libc, but would still be good to have a clean build. Maybe wait till @dblaikie or @mtrofin get a chance to fix it or rebase on the last passing commit (https://github.com/llvm/llvm-project/commit/a556ec8861) so we can make sure this is all working properly. I don't see the libc targets showing up in the build logs, but I think that's just because it's a bazel test invocation so the final summary just prints tests (even though it does also build the targets specified). Have you double checked that these targets show up in bazel query @llvm-project//... as expected (I can check, if you'd like).
| utils/bazel/llvm-project-overlay/libc/BUILD.bazel | ||
|---|---|---|
| 37 | Nit: there's no official line length limit in the style guide, but it is recommended to wrap at 80 (79?) where possible, so I think it makes sense to have the line fills and explicit wraps be that wide. https://docs.bazel.build/versions/main/skylark/bzl-style.html#line-length | |
Comment Actions
I've cherry picked this change on top of the commit you mentioned, the build is green (I've also checked that llvm libc targets do appear in bazel query)
I also fixed the line limit.
I'll wait for your final approval and submit.
Comment Actions
Thanks, LGTM 🙂 (The previous accept was intended to indicate "please fix these nits then gtg")
Nit: there's no official line length limit in the style guide, but it is recommended to wrap at 80 (79?) where possible, so I think it makes sense to have the line fills and explicit wraps be that wide. https://docs.bazel.build/versions/main/skylark/bzl-style.html#line-length