Skip to content
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

Merged
merged 19 commits into from
Apr 4, 2024
Merged

Add support for bzlmod #11046

merged 19 commits into from
Apr 4, 2024

Conversation

keith
Copy link
Contributor

@keith keith commented Mar 27, 2024

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

@ejona86
Copy link
Member

ejona86 commented Mar 28, 2024

#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 update_labels.patch that you needed.

I don't understand how maven_install is supposed to work, because defining a custom grpc_java_maven doesn't seem appropriate. We need the entire build to use the same set of versions resolved by maven_install. But I don't understand bzlmod enough to understand how diamond dependencies are supposed to work in general, much less through maven_install.

@ejona86
Copy link
Member

ejona86 commented Mar 28, 2024

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.

MODULE.bazel Outdated Show resolved Hide resolved
@keith
Copy link
Contributor Author

keith commented Mar 29, 2024

I saw bazelbuild/bazel-central-registry#1699 before seeing this, and had actually noticed the unfortunate update_labels.patch that you needed.

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 don't understand how maven_install is supposed to work, because defining a custom grpc_java_maven doesn't seem appropriate. We need the entire build to use the same set of versions resolved by maven_install. But I don't understand bzlmod enough to understand how diamond dependencies are supposed to work in general, much less through maven_install.

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

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.

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

@pswaminathan
Copy link

@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 com_google_protobuf_javalite repo?

ERROR: /private/var/tmp/_bazel_p/07e53bbbb53e1b58583f1109f79a3661/external/rules_jvm_external~~maven~maven/BUILD:11002:11: @@rules_jvm_external~~maven~maven//:com_google_firebase_firebase_perf: invalid label '@@[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]//:protobuf_javalite' in element 15 of attribute 'deps' in 'aar_import' rule: invalid repository name '[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.' and '~' and must not start with '~'
ERROR: /private/var/tmp/_bazel_p/07e53bbbb53e1b58583f1109f79a3661/external/rules_jvm_external~~maven~maven/BUILD:11149:11: @@rules_jvm_external~~maven~maven//:com_google_firebase_protolite_well_known_types: invalid label '@@[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]//:protobuf_javalite' in element 0 of attribute 'deps' in 'aar_import' rule: invalid repository name '[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.' and '~' and must not start with '~'
ERROR: /private/var/tmp/_bazel_p/07e53bbbb53e1b58583f1109f79a3661/external/rules_jvm_external~~maven~maven/BUILD:11310:6: @@rules_jvm_external~~maven~maven//:com_google_protobuf_protobuf_javalite: invalid label '@@[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]//:protobuf_javalite' in attribute 'actual' in 'alias' rule: invalid repository name '[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.' and '~' and must not start with '~'
ERROR: /private/var/tmp/_bazel_p/07e53bbbb53e1b58583f1109f79a3661/external/rules_jvm_external~~maven~maven/BUILD:11310:6: @@rules_jvm_external~~maven~maven//:com_google_protobuf_protobuf_javalite: missing value for mandatory attribute 'actual' in 'alias' rule
ERROR: /private/var/tmp/_bazel_p/07e53bbbb53e1b58583f1109f79a3661/external/rules_jvm_external~~maven~maven/BUILD:11346:11: @@rules_jvm_external~~maven~maven//:com_google_protobuf_protobuf_kotlin_lite: invalid label '@@[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]//:protobuf_javalite' in element 0 of attribute 'deps' in 'jvm_import' rule: invalid repository name '[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.' and '~' and must not start with '~'
ERROR: /private/var/tmp/_bazel_p/07e53bbbb53e1b58583f1109f79a3661/external/rules_jvm_external~~maven~maven/BUILD:13596:11: @@rules_jvm_external~~maven~maven//:original_io_grpc_grpc_protobuf_lite: invalid label '@@[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]//:protobuf_javalite' in element 2 of attribute 'deps' in 'jvm_import' rule: invalid repository name '[unknown repo 'com_google_protobuf_javalite' requested from @@grpc-java~]': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.' and '~' and must not start with '~'

@keith
Copy link
Contributor Author

keith commented Apr 1, 2024

I assume you're building for android? can you verify with what i just pushed?

Copy link
Member

@ejona86 ejona86 left a 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!)

MODULE.bazel Outdated Show resolved Hide resolved
MODULE.bazel Show resolved Hide resolved
@keith keith marked this pull request as ready for review April 1, 2024 20:56
@keith keith requested a review from ejona86 April 4, 2024 00:38
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 4, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 4, 2024
@ejona86 ejona86 requested a review from temawi April 4, 2024 14:26
@ejona86 ejona86 merged commit d1890c0 into grpc:master Apr 4, 2024
15 checks passed
@ejona86
Copy link
Member

ejona86 commented Apr 4, 2024

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.

@keith keith deleted the ks/add-bzlmod branch April 4, 2024 16:04
@keith
Copy link
Contributor Author

keith commented Apr 4, 2024

how did you spot that? are you saying we should update rules_jvm_external itself in repositories.bzl?

@ejona86
Copy link
Member

ejona86 commented Apr 4, 2024

Looks like it is an issue in rules_jvm_external, in that it uses Java from PATH. bazel info java-runtime on my machine shows JDK 21. But version 6.0 started requiring Java 11, so now it is noticeable. That should be okay for us, but dunno about downstream. I guess WORKSPACE doesn't prevent downgrades the least bit, so it might not really matter.

@pswaminathan
Copy link

I assume you're building for android? can you verify with what i just pushed?

@keith yes, and that commit worked for me. Thanks!

mbland added a commit to EngFlow/example that referenced this pull request Apr 13, 2024
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.
mbland added a commit to EngFlow/example that referenced this pull request Apr 15, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a MODULE.bazel in order to support for bzlmod
5 participants