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 bzlmod support #4937

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add bzlmod support #4937

wants to merge 3 commits into from

Conversation

AlejoAsd
Copy link

@AlejoAsd AlejoAsd commented Nov 12, 2024

References to other Issues or PRs

Fixes #3759.

Have you read the Contributing Guidelines?

Yes.

Brief description of what is fixed or changed

As mentioned in #3759, Bazel has introduced bzlmod to better handle dependencies across projects. In order to enable bzlmod support, the existing Bazel WORKSPACE must be migrated to and temporarily co-exist with a MODULE.bazel file, which has some similar syntax but works differently.

This PR introduces a MODULE.bazel file, lock and non-module dependencies file. It allows building and importing the project as a Bazel module.

This work is heavily based on the great work of @SanjayVas, with some dependency versions updated to match the existing WORKSPACE configuration.

This was tested by building the entire repository with and without bzlmod with the following commands:

# bzlmod build
bazel run //:gazelle; bazel build //...

# WORKSPACE build
bazel run //:gazelle; bazel build //... --noenable_bzlmod

Other comments

  • The module version is currently hardcoded to v2.23.0 and will have to be updated manually. This can potentially be automated by Github actions when creating releases, but is considered future work.
  • This is a first step towards having the project be available in the Bazel registry as a module, but not enough on its own.
  • Even though this change does not register the project as a module in the registry, it can now be imported by defining a bazel_dep along with a suitable override in downstream projects using bzlmod.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this! It seems to be failing to run buildifier at the moment. I'm definitely interested in having this done and migrating away from WORKSPACE over time, but we need to be sure we understand the added maintenance burden.

###############################################################################
module(
name = "grpc_ecosystem_grpc_gateway",
version = "2.23.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for mentioning that this has to be maintained manually. When it comes to making a new release, should this be updated before or after the new release? We currently have a manual step in our CI already, so if this is just another post-release step, that's probably OK. If it needs to be updated before a release is tagged in git, that makes it a bit more annoying.

Copy link
Author

Choose a reason for hiding this comment

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

(Disclaimer: I am being super verbose about how Bazel works because I've read in some issues that you don't usually work with Bazel. Please let me know if I am being too verbose!)

Ideally it should be done with every new release. The truth is that this will not matter as much until the package is available through the Bazel registry, because right now you need to manually specify the git hash or download the archive to make this available to other projects.

As an example, in my downstream project, this is how I would end up using this:

# The `version` here has to match the `module` version in the project's MODULE.bazel.
bazel_dep(name = "grpc_ecosystem_grpc_gateway", version = "2.23.0")
# Tell Bazel to pull directly from the repo
git_override(
    module_name = "grpc_ecosystem_grpc_gateway",
    remote = "https://github.com/grpc-ecosystem/grpc-gateway.git",
    commit = "v2.23.0"  # This could be a any version, it's not limited to tags
)

As long as the version I specify in bazel_dep matches the version in the module declaration in MODULE.bazel this will work. It does become a little more strict when publishing to the Bazel registry though.

I believe that it would make sense to only update the module version when releasing, to reduce the manual burden and to better prepare the workflow for when (and if) the project is published to the Bazel registry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for clarifying, to be clear, it sounds like we'd need to update the module version before tagging a new release tag in git? Is there any way we could punt that requirement until we decide to support the bazel registry down the road? If users have to override with git anyway, is it still a requirement for us to set it accurately?

MODULE.bazel Show resolved Hide resolved
Comment on lines +22 to +31
go_deps.module(
path = "google.golang.org/grpc/cmd/protoc-gen-go-grpc",
sum = "h1:rNBFJjBCOgVr9pWD7rs/knKL4FRTKgpZmsRfV214zcA=",
version = "v1.3.0",
)
go_deps.module(
path = "github.com/golang/protobuf",
sum = "h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek=",
version = "v1.5.4",
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these versioned separately?

Copy link
Author

Choose a reason for hiding this comment

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

These are necessary because Gazelle only generates dependencies for Go files (since it generates dependencies by reading the go.mod file).

These two dependencies are introduced by .proto files and are not included in go.mod, so by adding these we tell Gazelle that those should be downloaded as well.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst Nov 13, 2024

Choose a reason for hiding this comment

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

I'm confused. If they are go deps, they should be in the go.mod file, since we check in our generated Go protobuf files and any dependencies they have will form part of the go.mod dependency closure at Go dependency resolution time. If these are pure protobuf dependencies (AFAIK, we don't have any proto dependencies other than googleapis), why are they part of go_deps?

MODULE.bazel Outdated Show resolved Hide resolved
non_module_deps.bzl Outdated Show resolved Hide resolved
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Looks like there's still a small generation error with the lock file - is there a risk that the contents of the lock file will change over time? I really like being able to re-run buildifier and have the output be deterministic barring changes to go.mod or (I guess) MODULE.bazel.

@johanbrandhorst
Copy link
Collaborator

Interesting, when I check this out and run bazel run gazelle locally, I see no diff, but for some reason there's a diff in CI?

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.

Bzlmod support
2 participants