-
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
Use the artifact
macro for loading maven deps
#9574
Use the artifact
macro for loading maven deps
#9574
Conversation
The recommended way to load dependencies from `rules_jvm_external` is to make use of the `@maven` workspace, and the most readable way of doing that is to use the `artifact` macro provides. This removes the need to generate the "compat" namespaces, which `rules_jvm_external` provided for backwards compatibility with older releases. This change also sets things up for supporting `bzlmod`: this requires all workspaces accessed by a library to be named "up front" in the `MODULE.bazel` file. This way, the only repo that needs to be exported is `@maven`, rather than the current huge list. Finally, this PR introduces a lock file for `rules_jvm_external` which improves local build times by avoiding the need to do a local resolution. In order to avoid the common failure case of "add a dep, forget to regenerate the lock file", the `fail_if_repin_required` attribute is set: builds will fail if the deps have been updated but the lock file hasn't been.
We've been purposefully avoiding using |
I'm +1 for this PR. Thank you for double-checking @ejona86! I'm one of the maintainers of Until |
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.
Must we have maven_install.json
? I looked into that earlier and it seems super-annoying; basically meaning any version bumps can't be backported without manual conflict resolution. And all dependency changes must be made serially. I know you lose some things without it, but it solves problems we don't have and creates problems we don't need.
TL;DR: it's fine not to use the lock file. There's no need to use it, but it makes builds more reproducible. Builds may also be faster, since they can skip the dependency resolution step: I'm not sure how much of a difference that makes for you, and Bazel should be caching the results until the bazel server shuts down. |
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.
1.33.0 removed the manual maven repos from repositories.bzl
, so it seems pretty safe to do this. The only risk I see is someone is using a tool other than maven_install to manage these deps, but I think it is worth a try to do this and see if any issues crop up.
Hmm... I don't know how #9559 will impact this (or everything, really) |
it would be great if we could merge this, I have a local change that supports bzlmod for this repo but it either depends on this or breaks compat w/ the non-bzlmod setup |
Turned back off compat repositories to swapping new targets to artifact() in a less-noisy commit. Conflicts: BUILD.bazel WORKSPACE api/BUILD.bazel core/BUILD.bazel netty/BUILD.bazel okhttp/BUILD.bazel services/BUILD.bazel testing/BUILD.bazel xds/BUILD.bazel
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.
awesome!
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.
@temawi, let me merge this once you approve, as I'll need to fix up the commit message.
The recommended way to load dependencies from
rules_jvm_external
is to make use of the@maven
workspace, and the most readable way of doing that is to use theartifact
macro provides.This removes the need to generate the "compat" namespaces, which
rules_jvm_external
provided for backwards compatibility with older releases. This change also sets things up for supportingbzlmod
: this requires all workspaces accessed by a library to be named "up front" in theMODULE.bazel
file. This way, the only repo that needs to be exported is@maven
, rather than the current huge list.Finally, this PR introduces a lock file for
rules_jvm_external
which improves local build times by avoiding the need to do a local resolution. In order to avoid the common failure case of "add a dep, forget to regenerate the lock file", thefail_if_repin_required
attribute is set: builds will fail if the deps have been updated but the lock file hasn't been.