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

Merged
merged 6 commits into from
Nov 15, 2024
Merged

Add bzlmod support #4937

merged 6 commits into from
Nov 15, 2024

Conversation

AlejoAsd
Copy link
Contributor

@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

  • This is a first step towards having the project be available in the Bazel registry as a module, but not enough on its own.
  • The Bazel module version is set to 0.0.0 which will have to be changed if this module is to be published to the Bazel registry. Once this package is in the Bazel registry, the version will have to be updated every time a new release is created.
  • 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.bazel Outdated Show resolved Hide resolved
MODULE.bazel Show resolved Hide resolved
MODULE.bazel Show resolved Hide resolved
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?

@AlejoAsd
Copy link
Contributor Author

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?

Same here. Let me see if I can figure out why.

@AlejoAsd
Copy link
Contributor Author

AlejoAsd commented Nov 14, 2024

The @@rules_rust hash that is causing the job to fail is some transitive dependency that is not explicitly defined in the MODULE.bazel file.

The only thing I've found that could affect the difference in the lockfile is the .bazelrc config being used in CI pipelines. Specifically, the fact that we are using a cache.

It might be the case that some dependency in the cache is causing the lockfile to turn out slightly differently. Could you try running CI for this PR without cache?

If the cache turns out to be the problem, a simple solution is to change the command that is failing so that it does not diff the lockfile.

bazel run //:gazelle && git diff --exit-code -- !MODULE.bazel.lock

I don't know if this is something we want though.

@johanbrandhorst
Copy link
Collaborator

The @@rules_rust hash that is causing the job to fail is some transitive dependency that is not explicitly defined in the MODULE.bazel file.

The only thing I've found that could affect the difference in the lockfile is the .bazelrc config being used in CI pipelines. Specifically, the fact that we are using a cache.

It might be the case that some dependency in the cache is causing the lockfile to turn out slightly differently. Could you try running CI for this PR without cache?

Looking at the CI job, it doesn't look like it's actually able to match a cache in this case, so I don't think that's the reason.

If the cache turns out to be the problem, a simple solution is to change the command that is failing so that it does not diff the lockfile.

bazel run //:gazelle && git diff --exit-code -- !MODULE.bazel.lock

I don't know if this is something we want though.

Thanks for the suggestion, but yeah that's definitely not something we want. We'll have to figure out some way to make it deterministic, I never want to see a git diff after running the generation script. Can we specify the version to use for this transitive dependency to avoid it being resolved dynamically?

@AlejoAsd
Copy link
Contributor Author

Looks like that pinning the rules_rest version did it. I'll add a comment explaining why we added that dependency and we should be good to merge after that.

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.

Thank you so much for the help and the explanations! I wonder, do we need to add anything to our README regarding how to depend on the project as a bazel user? We don't need to do it as part of this PR.

@johanbrandhorst johanbrandhorst enabled auto-merge (squash) November 15, 2024 15:07
@AlejoAsd AlejoAsd disabled auto-merge November 15, 2024 15:09
@AlejoAsd
Copy link
Contributor Author

Excellent question. Let me take a look and update the README if necessary before merging.

@AlejoAsd
Copy link
Contributor Author

AlejoAsd commented Nov 15, 2024

Hmm, it looks like the only "real documentation" we have for setting up things with Bazel is this example repo shared in an issue comment.

Since we're not publishing to the Bazel Registry just yet, I wanted to add a short user guide on how to import this repo as a Bazel module, but I don't think there is a README or guide that goes into Bazel integration.

Let's follow up in another PR so that we can at least merge this. Thanks a ton for the thorough reviews Johan!

@johanbrandhorst johanbrandhorst merged commit 6ceae66 into grpc-ecosystem:main Nov 15, 2024
14 checks passed
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution! If you see any other QoL changes we can make for Bazel users, please don't hestitate to contribute :)

@AlejoAsd AlejoAsd deleted the bzlmod branch November 19, 2024 20:00
@ns-ggeorgiev
Copy link

I tried to adopt your repo through bzlmod but it complains about missing com_github_bazelbuild_buildtools

@AlejoAsd
Copy link
Contributor Author

Thanks @ns-ggeorgiev, I have opened #4974 to address this.

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
3 participants