-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[Bazel] Migrate to Bzlmod #88927
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[Bazel] Migrate to Bzlmod #88927
Conversation
cc @keith Still under construction, but at least this works with |
think you'll get back to this sometime? |
I believe this is still blocked by these:
I just tried building with |
Seems like the two issues are resolved now. Any chance you want to try the transition again? |
Will retry this weekend 👍 I'll need to figure out a more elegant solution for the patching and I think I figured out a better way for the external dependencies via label-flags so that we no longer need to maintain the custom nonhermeticity logic. |
ebec54f
to
c16968e
Compare
@keith @rupprecht Do you think you could maybe review the PR soonish? It is quite the nice QoL improvement |
FYI I've pulled out the zlib/zstd changes in a cleaner way in #126729 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not yet very familiar w/ bzlmod, so I'm not a great reviewer. But if it works, it works. Thanks for taking care of this!
# We unconditionally depend on the custom LLVM zlib wrapper. This will | ||
# be an empty library unless zlib is enabled, in which case it will | ||
# both provide the necessary dependencies and configuration defines. | ||
"@llvm_zlib//:zlib", | ||
# We unconditionally depend on the custom LLVM zstd wrapper. This will | ||
# be an empty library unless zstd is enabled, in which case it will | ||
# both provide the necessary dependencies and configuration defines. | ||
"@llvm_zstd//:zstd", | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like having this pattern, as it makes the build files less cluttered (fewer selects). Could we re-introduce a wrapper around it, similar to the maybe_pfm target in this file? i.e. something like:
cc_library(
name = "maybe_zlib",
defines = select({
":zlib_enabled": ["LLVM_ENABLE_ZLIB=1"],
"//conditions:default": [],
}),
deps = select({
":zlib_enabled": ["@zlib-ng"],
"//conditions:default": [],
}),
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to #126729
I did a lot of back-and-forth with the "maybe pattern". The cleaner looking callsites ("depsites" I guess 😅) are appealing but I ultimately came to the conclusion that it seems like a bad idea because it abuses implementation details that "just happen" to work in the cc_library case. That is, this isn't a generally applicable pattern that could be used for arbitrary rules because deps
might not always be implemented transitively.
It also introduces additonal complexity that makes the build graph less efficient (an edge to maybe_zlib
even if it's unused) and harder to reason about (can no longer easily see which config option influences the dep - need to go to the maybe
target to check).
However, we can still make this less verbose. #126729 moves the LLVM_ENABLE_ZLIB
define to the config rather than a defines
attribute. This not only saves a lot off command-line length but also reduces the overall number of selects. Now the 4 occurances of the zlib dep in the overlay add just 3 additional selects over the maybe approach (as opposed to the previous 6) and just 1 additional select in the zstd case (as opposed to the previous 2).
utils/bazel/extensions.bzl
Outdated
# TODO: Make `NB_BUILD=1` and `NB_SHARED=1` configurable in the BCR variant | ||
# and import this via `MODULE.bazel`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this comment means, would you mind clarifying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically we should treat the http_archive
calls as "migration helpers" with the goal of removing all of them and instead importing them via bazel_dep
in the MODULE.bazel
file. For the deps that are already in the Bazel Central Registry (BCR) I did that. The http_archive
s are ones that currently don't have a corresponding BCR module.
The nanobind was an exception where a module exists in the BCR, but that variant doesn't work for our usecase at the moment.
I've clarified the comment.
fb44deb
to
8bdeb5f
Compare
This change implements the following flags and provides the infrastructure for future additions: bazel query 'kind(".*_flag", @llvm-project//config:all)' @llvm-project//config:LLVM_ENABLE_BACKTRACES @llvm-project//config:LLVM_ENABLE_CRASH_DUMPS @llvm-project//config:LLVM_ENABLE_CRASH_OVERRIDES @llvm-project//config:LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING @llvm-project//config:LLVM_ENABLE_PLUGINS @llvm-project//config:LLVM_ENABLE_ZLIB @llvm-project//config:LLVM_ENABLE_ZSTD @llvm-project//config:LLVM_HAS_LOGF128 @llvm-project//config:LLVM_WINDOWS_PREFER_FORWARD_SLASH For instance: bazel build ... \ --@llvm-project//config:LLVM_ENABLE_BACKTRACES=false The following flags have been moved to mirror CMake and simplify migration to bzlmod: @llvm_zlib//:llvm_enable_zlib -> @llvm-project//config:LLVM_ENABLE_ZLIB @llvm_zstd//:llvm_enable_zstd -> @llvm-project//config:LLVM_ENABLE_ZSTD The overlay now has platform definitions at `@llvm-project//platform`. You can use these to cross compile the config headers to see how they'd look like on other systems. For instance: bazel build @llvm-project//llvm:config_h \ --platforms=@llvm-project//platform:aarch64-unknown-linux-gnu bazel build @llvm-project//llvm:llvm-config_h \ --platforms=@llvm-project//platform:x86_64-pc-win32 The new implementation uses the original CMake templates as source of truth. This makes it easier to detect and prevent drift as unhandled substitutions tend to turn into compiler errors. Saves 16 defines on windows and ~35 defines on other platforms which previously leaked into the command lines of all targets that depended transitively on the llvm configuration. For a full build of the overlay this amounts to ~380k fewer defines.
The workspace build is now gated behind `--config=deprecated_workspace`. See `utils/bazel/examples` for the new setup instructions. While not necessarily the most intuitive implementation, this seems to be the only way retain the flexibility of all potential usecases as of Bazel 8. The module build supports: - The in-tree build from within the `utils/bazel` directory. - The llvm-project cloned into an arbitrary directory (e.g. for local development, submodule setups and and Nix users). - The llvm-project fetched from github via an archive. Since module overrides are invisible to third-party dependents, every project that has the llvm project "somewhere" in its dependency tree needs to create an explicit override in their root module. It's possible to avoid this inconvenience by creating custom bazel registries, but only makes sense to do for the next stable tag that contains this commit. If you need to link against a nonhermetic system variant of an external dependency you can override the repositories from the `llvm_project_overlay` extension like this: ```python '''MODULE.bazel''' new_local_repository = use_repo_rule( "@bazel_tools//tools/build_defs/repo:local.bzl", "new_local_repository", ) new_local_repository( name = "zlib-ng", path = "xxxx", build_file_content = "xxxx", ) override_repo(llvm_project_overlay, "zlib-ng") ```
8bdeb5f
to
17937a1
Compare
Note: This is now rebased onto (and dependent on) #126729 to separate flag config changes from the bzlmod changes. |
The workspace build is now gated behind
--config=deprecated_workspace
.See
utils/bazel/examples
for the new setup instructions.While not necessarily the most intuitive implementation, this seems to
be the only way retain the flexibility of all potential usecases as of
Bazel 8. The module build supports:
utils/bazel
directory.development, submodule setups and and Nix users).
Since module overrides are invisible to third-party dependents, every
project that has the llvm project "somewhere" in its dependency tree
needs to create an explicit override in their root module. It's possible
to avoid this inconvenience by creating custom bazel registries, but
only makes sense to do for the next stable tag that contains this commit.
If you need to link against a nonhermetic system variant of an external
dependency you can override the repositories from the
llvm_project_overlay
extension like this: