WORKSPACE is incompatible with bzlmod, so this allows us to pull these dependencies using bzlmod.
This should be a no-op change.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| utils/bazel/WORKSPACE | ||
|---|---|---|
| 18 | "Bazel Central Repository (BCR)" | |
Comment Actions
Blocking until I've tested this against rules_ll. It's a noop for the workspace build but might have implications for our already existing module build.
Did something change regarding maybe in bzlmod? I'm fairly certain that maybe won't be compatible with bzlmod (https://github.com/bazelbuild/bazel/issues/15377).
Potentially related, the bzlmod patch: (https://reviews.llvm.org/D137008)
Comment Actions
Blocking until I've tested this against rules_ll. It's a noop for the workspace build but might have implications for our already existing module build.
It won't have any implications, given that in your module build you won't actually import any of the things I added.
Did something change regarding maybe in bzlmod? I'm fairly certain that maybe won't be compatible with bzlmod (https://github.com/bazelbuild/bazel/issues/15377).
I believe what wyv meant when he said it "doesn't work with bzlmod" is that if you import the same repo rule twice, it won't do what it's supposed to and stop the second repo rule invocation. This is a non-issue for modules though, since as he mentioned, namespacing prevents this from ever happening, unless you do it twice in the same module (which you shouldn't, and we don't).
Potentially related, the bzlmod patch: (https://reviews.llvm.org/D137008)
Ah, I wasn't aware of this, thanks.
Comment Actions
Ok tested this, works. This needs some adjustment after https://github.com/llvm/llvm-project/commit/a268127736e4d703cef8c9f4841f9a8e8ac21ba7 (the zlib-ng archive needs to be a non-bcr-deps.bzl-dep, the old zlib variant is vulnerable and causes a gazillion warnings), but I like the way that this cleans up the bzlmod patch.
I'm actually stongly in favor of completely removing support for terminfo before we make this change though. Terminfo removal has been overdue for years and since we've now reworked all other external dependencies we might as well kick out all remaining legacy. That way we make this change a bit more compact and the bzlmod patch will also really nicely fall into place.
Comment Actions
@matts1 Changes to terminfo and zlib are now done. After rebasing this this should be fine to merge and then I'll update the bzlmod patch accordingly. Feel free to ping me if you need me to commit it for you 😊
| utils/bazel/WORKSPACE | ||
|---|---|---|
| 16 | This (and the other //repos) occurrances being relative might be risky since we move buildfiles around a lot when applying the configure logic. This keeps confusing me, so I might be wrong here, but I think we want this to be @llvm-raw//utils/bazel/repos:bcr_deps.bzl etc. to ensure that these files are found after applying the overlay. | |
| utils/bazel/repos/bcr_deps.bzl | ||
| 12 | We should probably update this dep (in a later patch). This is ancient. | |
| 24–34 | This is superceded by the llvm_zlib (zlib-ng) dependency in non_bcr_deps.bzl. | |
| utils/bazel/repos/llvm_repos.bzl | ||
| 13 | Ahh such simplicity :D It makes my heart hurt a bit that we still need the vulkan_sdk_setup, but that's on me. We can fix it later. | |
| utils/bazel/repos/non_bcr_deps.bzl | ||
| 12–22 | Oh I overlooked this dependency. The current implementation doesn't match the config_setting style that we now use for all other dependencies, but the current implementation (in vulkan_sdk.bzl) doesn't seem to interfere with bzlmod. I think this should be fine to fix in a later patch. | |
Comment Actions
I would like to see refactoring like I described in https://reviews.llvm.org/D143320 that makes it easy for users to configure the dependencies they need. The distinction between BCR and non-BCR is not really relevant for anyone using WORKSPACE files, so I don't think it helps. This actually makes it harder for people using WORKSPACE files because it adds an additional non-meaningful layer of indirection. As mentioned there, the examples in https://github.com/llvm/llvm-project/tree/main/utils/bazel/examples should be updated to demonstrate how to actually use this. It's a bit tricky, since a commit has to be checked in before it can be referenced, but we should check that it will work before checking this in (using the pending commit) and then actually update the examples to point at the commit once checked in. It would be helpful to add a bzlmod example there as well (and we should add a git_repository one as well).
Generally, I'm not really sold on bzlmod actually fixing the problems with WORKSPACE files without creating new ones, so I don't want this to make using WORKSPACE files any harder (it already has with the zstd and zlib changes, which we should fix).
Comment Actions
hi @matts1, FYI "bazel build" tag was created so that interested people will run bazel build as part of their pre-merge check, not in reviewing changes to bazel. It's also quite large.
Comment Actions
I don't disagree here, but I'll leave that to a future patch, since this is supposed to be a no-op refactor.
The distinction between BCR and non-BCR is not really relevant for anyone using WORKSPACE files, so I don't think it helps. This actually makes it harder for people using WORKSPACE files because it adds an additional non-meaningful layer of indirection. As mentioned there, the examples in https://github.com/llvm/llvm-project/tree/main/utils/bazel/examples should be updated to demonstrate how to actually use this. It's a bit tricky, since a commit has to be checked in before it can be referenced, but we should check that it will work before checking this in (using the pending commit) and then actually update the examples to point at the commit once checked in. It would be helpful to add a bzlmod example there as well (and we should add a git_repository one as well).
I hadn't considered this up until now. I was just assuming that users would still use the same method described in examples (which this change doesn't break - it still works). However, you raise a good point, so I've changed it to work the same way as most other repos (load the @<repo>, then call <repo>_deps to load the dependencies, then call <repo>_repos to execute our repo rules. This should make it much easier to load, but I'll leave that to a future CL.
Generally, I'm not really sold on bzlmod actually fixing the problems with WORKSPACE files without creating new ones, so I don't want this to make using WORKSPACE files any harder (it already has with the zstd and zlib changes, which we should fix).
Comment Actions
I think the examples were actually already broken by the previous patches in this area. You now have to manually configure zlib and zstd. They currently "work" because they're still pointing at an old version of the repo.
Comment Actions
Sorry for the late reply. LGTM.
I'm not a fan of the deps.bzl pattern in general. For instance, with a Protobuf workspace for multiple languages the import order of things can get very confusing when every ruleset imports their own variant of grpc/proto dependencies with potentially clashing names. This has always been an issue though and virtually every other WORKSPACE-based Bazel project uses the deps.bzl pattern. So I'd expect users to be used to this and "know where to look" when seeing an llvm_deps.bzl file. So for WORKSPACEs I'd say this is about as "standardized" as we could get.
Overriding dependencies can be done by adding overrides of the same name as the http_archives in llvm_deps.bzl before calling llvm_deps() in the WORKSPACE. I.e.
# WORKSPACE.bazel
load("@llvm-raw//utils/bazel/repos:llvm_deps.bzl", "llvm_deps")
http_archive(
name = "myoverriddendep",
...
)
llvm_deps()Whether the overriden dep is actually enabled or not in the build is then a configuration flag in .bazelrc. E.g.
# .bazelrc build --@llvm-project//clang-tools-extra/clang-tidy:enable_static_analyzer=false build --@llvm_zstd//:llvm_enable_zstd=false
This should be fairly CI friendly since it allows easily setting different build configurations within a single .bazelrc via --config options and no longer requires custom environment variables. It also decouples customizations from the BUILD files.
I'm not sure whether the examples currently work, but they'll definitely be broken after this patch. I don't have a strong opinion on whether they should be updated as part of this patch or in a later patch. I'd actually lean towards "later" and then do it properly with a more explicit README etc. There is also the option to remove the examples entirely and add all of that to the main README. Considering that using the overlay is roughly just
http_archive(
name = "llvm-raw",
...
)
load("@llvm-raw//utils/bazel/repos:llvm_deps.bzl", "llvm_deps")
# Optionally, override dependencies here.
# http_archive(name = "myoverride", ...)
# Some components of LLVM are configurable by adding the corresponding
# config options in your `.bazelrc`. See third-party-deps for details.
llvm_deps()
load("@llvm-raw//utils/bazel/repos:llvm_repos.bzl", "llvm_repos")
llvm_repos(name = "llvm-project")(+- skylib/foreign_cc), I think this would also be fine.
Comment Actions
Ah I guess the llvm_configure still kind of works. So this doesn't seem to break the examples (more than they might already be). I believe we should ultimately aim for a setup like
http_archive(name = "llvm...", ...) # load(...deps.bzl, "llvm_dependencies") # Or load(...repositories.bzl, "llvm_dependencies") llvm_dependencies()
as this is what most projects look like, e.g. rules_go, rules_proto, and rules_rust.
This (and the other //repos) occurrances being relative might be risky since we move buildfiles around a lot when applying the configure logic.
This keeps confusing me, so I might be wrong here, but I think we want this to be @llvm-raw//utils/bazel/repos:bcr_deps.bzl etc. to ensure that these files are found after applying the overlay.