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

Convert WORKSPACE to MODULE.bazel, update README #312

Merged
merged 18 commits into from
Apr 15, 2024
Merged

Convert WORKSPACE to MODULE.bazel, update README #312

merged 18 commits into from
Apr 15, 2024

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Apr 15, 2024

Inspired by https://github.com/EngFlow/engflow/pull/20913#discussion_r1559691958 by @jayconrod.

If this PR is too large, I'm happy to break it up. I also don't know yet how this will affect dependabot, so I'm happy to research that first if need be.

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 updated the other builds to use it as well.

The only one that isn't ready is //scala. rules_scala hasn't migrated to bzlmod yet, but discussion is underway.

Finally, //swift still won't build remotely, for the reasons cited here:

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
Also bumps to:

- protobuf: 23.1
- rules_proto: 6.0.0-rc2
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.
`bazel build //java/...` and `bazel test //java/...` both work with
these changes.
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.
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
There's actually a 0.2.1 release, but it hasn't been pushed to
https://registry.bazel.build/ yet.
rules_scala hasn't migrated to bzlmod yet, but discussion is underway.
These links will help track its progress.
This eliminates warnings that these tests are sized too big, since the
default is "medium".
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
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.
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
@mbland mbland requested a review from jayconrod April 15, 2024 13:43
@mbland mbland self-assigned this Apr 15, 2024
Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice. Thanks for doing this.

(I was initially a little shocked when I saw the size of the diff, before I saw it's all in MODULE.bazel.lock. There's so much in there. I wish they hadn't added that and kept version resolution fast and deterministic.)

@mbland mbland merged commit c9a30c2 into main Apr 15, 2024
4 checks passed
@mbland mbland deleted the bzlmod branch April 15, 2024 16:22
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.

2 participants