[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 |
rupprecht
left a comment
There was a problem hiding this comment.
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.
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.
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).
| # 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.
Not sure what this comment means, would you mind clarifying?
There was a problem hiding this comment.
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_archives 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. |
|
Any chance that some progress could be made on this? It seems to have stalled for quite a while |
The workspace build is now gated behind
--config=deprecated_workspace.See
utils/bazel/examplesfor 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/bazeldirectory.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_overlayextension like this: