-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Add support for bzlmod #11046
Add support for bzlmod #11046
Conversation
#9574 is merged, which causes havoc to this one. But I think we're gladly accepting such conflicts. I saw bazelbuild/bazel-central-registry#1699 before seeing this, and had actually noticed the unfortunate I don't understand how |
Another topic: is maven_install.json lock required? It causes me unease from a security standpoint. I mention something similar at grpc/grpc-proto#145 (comment) . maven_install.json isn't as bad a MODULE.bazel.lock in terms of complexity, but it still seems you could fabricate a dependency to inject your own code. To add the lock file we'd need the CI to re-generate it and verify the contents in git match. We do something similar for generated code already, so I think this could probably be done without much effort (re-generate the pin, then run git status) in the Bazel GithubActions CI. |
This reverts commit d7ec4fb.
either way we had to merge that there because of the circular dependency on googleapis, we can drop that patch for the next release if this gets merged
I wasn't sure either, discussed in this thread https://bazelbuild.slack.com/archives/CDCE4DFAP/p1711742750217869 sounds like it works how you want it to here where this repo's maven setup will contribute to the global repo named maven, so as long as downstream consumers use that name, your dependency versions will be taken into account
It sounds like checking it in, even for libraries, is the recommendation (same slack thread as above) so that resolution doesn't have to be redone ever. Added a task on CI to validate the pin |
@keith I'm not sure if it's an issue in this implementation or something else, but I'm getting an error trying to use this module in another project. Does the MODULE.bazel file need a declaration of the
|
I assume you're building for android? can you verify with what i just pushed? |
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.
This is shaping up. I think you could mark it as ready for review (since I have already!)
Hmm... rules_jvm_external 6.0 requires Java 11. The version used by java_grpc_library.bzl doesn't match MODULE.bzlmod for a few dependencies, including rules_jvm_external. Maybe we need rules_jvm_external to be different, but that makes it hard to notice when the versions are out-of-sync. |
how did you spot that? are you saying we should update rules_jvm_external itself in repositories.bzl? |
Looks like it is an issue in rules_jvm_external, in that it uses Java from PATH. |
@keith yes, and that commit worked for me. Thanks! |
grpc-java only got MODULE.bazel support as of this most recent version: - grpc/grpc-java#11046 - bazelbuild/bazel-central-registry#1699 This grpc-java version bump exposed two issues that are fixed in this commit: 1. The //java/com/engflow/notificationqueue:client target dependency on @maven//:io_netty_netty_handler broke. The original WORKSPACE import of io_grpc_grpc_java imported this dependency directly by passing IO_GRPC_GRPC_JAVA_ARTIFACTS directly to `maven_install`. The `maven.install` call from grpc/grpc-java's MODULE.bazel sets `strict_visibility = True`. Somehow the other dependencies registered by grpc-java's MODULE.bazel are accessible to notificationqueue:client, but netty-handler isn't. The solution was to add the `io.netty:netty-handler:4.1.100.Final` artifact to the `maven.install` call in this project's MODULE.bazel. It doesn't seem an optimal solution, but it works for now. 2. grpc/grpc-java removed `io.grpc.stub.MetadataUtils.attachHeaders()` in grpc/grpc-java#10443. This caused notificationqueue:client to fail to compile, but that PR revealed the replacement for the deprecated `attachHeaders` call. This commit applies that replacement.
* Enable bzlmod, update README, fix Swift build Rather than mess with the WORKSPACE rules, the shortest path to fixing `blaze build //swift:tests` appeared to be introducing MODULE.bazel. MODULE.bazel, a.k.a. bzlmod, appears to be the new hotness, so I'll also try updating the other builds to use it as well. - https://bazel.build/external/migration - https://bazel.build/external/overview#workspace-shortcomings - bazelbuild/bazel#18958 * Convert `bazel build //python/...` to MODULE.bazel * Convert `bazel build //csharp/...` to MODULE.bazel * Convert //go/..., gazelle to MODULE.bazel * Migrate protobuf, rules_proto to MODULE.bazel Also bumps to: - protobuf: 23.1 - rules_proto: 6.0.0-rc2 * Add rules_java, rules_jvm_external to MODULE.bazel rules_java was an implicit dependency before. Bumps: - rules_java: 7.4.0 => 7.5.0. - rules_jvm_external: 4.4.2 => 6.0 The rules_jvm_external docs describe using `maven.install` in MODULE.bazel as a replacement for `maven_install` in WORKSPACE: - https://github.com/bazelbuild/rules_jvm_external/blob/master/docs/bzlmod.md However, our current `maven_install` depends on the definition of IO_GRPC_GRPC_JAVA_ARTIFACTS from @io_grpc_grpc_java. I'll attempt that migration next. * MODULE.bazel: skylib, rules_proto_grpc, protobuf `bazel build //java/...` and `bazel test //java/...` both work with these changes. * Move grpc-java to MODULE.bazel, bump to 1.62.2 grpc-java only got MODULE.bazel support as of this most recent version: - grpc/grpc-java#11046 - bazelbuild/bazel-central-registry#1699 This grpc-java version bump exposed two issues that are fixed in this commit: 1. The //java/com/engflow/notificationqueue:client target dependency on @maven//:io_netty_netty_handler broke. The original WORKSPACE import of io_grpc_grpc_java imported this dependency directly by passing IO_GRPC_GRPC_JAVA_ARTIFACTS directly to `maven_install`. The `maven.install` call from grpc/grpc-java's MODULE.bazel sets `strict_visibility = True`. Somehow the other dependencies registered by grpc-java's MODULE.bazel are accessible to notificationqueue:client, but netty-handler isn't. The solution was to add the `io.netty:netty-handler:4.1.100.Final` artifact to the `maven.install` call in this project's MODULE.bazel. It doesn't seem an optimal solution, but it works for now. 2. grpc/grpc-java removed `io.grpc.stub.MetadataUtils.attachHeaders()` in grpc/grpc-java#10443. This caused notificationqueue:client to fail to compile, but that PR revealed the replacement for the deprecated `attachHeaders` call. This commit applies that replacement. * Move googleapis to MODULE.bazel This appears to be a fairly recent development, and isn't yet 100% officially supported in the googleapis/googleapis repo, but it works: - googleapis/googleapis#855 - bazelbuild/bazel-central-registry#1699 * Move rules_kotlin to MODULE.bazel, bump to v1.9.5 * Move rules_perl to MODULE.bazel, bump to 0.2.0 There's actually a 0.2.1 release, but it hasn't been pushed to https://registry.bazel.build/ yet. * Add rules_scala GitHub issue links rules_scala hasn't migrated to bzlmod yet, but discussion is underway. These links will help track its progress. * Migrate aspect_rules_ts to MODULE.bazel * Replace deps.bzl with go_deps This enables `bazel {build,test} //infra/...` to succeed using MODULE.bazel. See: - https://github.com/bazelbuild/rules_go/blob/master/docs/go/core/bzlmod.md#specifying-external-dependencies * Set test sizes to "small" as appropriate This eliminates warnings that these tests are sized too big, since the default is "medium". * Move http_{file,archive} calls to MODULE.bazel This moves the following http_file calls: - emacs - ubuntu_20.04_1.3GB and the following http_archive calls: - com_engflow_engflowapis - io_abseil_py * Update //platform rules, remove dotnet constraints The //dotnet/toolchain constraint was causing a Bazel error saying that the @@rules_dotnet//dontent/toolchain package didn't exist. Removing this constraint allowed remote execution to succeed anyway. * Update README, explain swift incompatibility Most of these changes are cosmetic, with the notable exception of the explantion behind the inability to build //swift remotely. Also added a `git` command to ignore python/requirements_lock.txt per: - https://stackoverflow.com/a/73720550
Add [email protected] Less patching is necessary than v1.62.2 since Bzlmod support was integrated upstream (grpc/grpc-java#11046).
Depends on bazelbuild/bazel-central-registry#1699 because of a circular dependency on googleapis
We probably want to do something like #9574 instead to support before and after bzlmod
Fixes: #9559