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

Use the artifact macro for loading maven deps #9574

Merged
merged 7 commits into from Mar 28, 2024

Conversation

AutomatedTester
Copy link
Contributor

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.

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.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 30, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@ejona86
Copy link
Member

ejona86 commented Oct 1, 2022

We've been purposefully avoiding using @maven directly since using it would require our users to use maven_install and they needed time to migrate. It's probably been long enough. Let me double-check the state of things.

@shs96c
Copy link

shs96c commented Oct 1, 2022

I'm +1 for this PR. Thank you for double-checking @ejona86!

I'm one of the maintainers of rules_jvm_external, and did the work to make it play nicely with bzlmod. I've not implemented the backwards compat libraries there because the user experience with them is so poor --- essentially, someone using them will need to list the transformed name of every single first-order dependency in their MODULE.bazel. For grpc-java, that's a pretty long list, which will need to be maintained in lock-step with any new imports added. The alternative (of using @maven) is significantly less work and (more importantly) a lot less error-prone.

Until grpc-java is modularised (#9559) anyone who transitively depends on grpc-java in their modularised builds would need to maintain this list themselves, and it would require support in rules_jvm_external that I'm not keen on putting there.

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.

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.

@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 Oct 6, 2022
@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 Oct 6, 2022
@shs96c
Copy link

shs96c commented Oct 6, 2022

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.

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.

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.

WORKSPACE Outdated Show resolved Hide resolved
@ejona86
Copy link
Member

ejona86 commented Jan 10, 2023

Hmm... I don't know how #9559 will impact this (or everything, really)

@keith
Copy link
Contributor

keith commented Mar 26, 2024

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

@keith keith mentioned this pull request Mar 27, 2024
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
Copy link
Contributor

@keith keith left a comment

Choose a reason for hiding this comment

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

awesome!

@ejona86 ejona86 requested a review from temawi March 28, 2024 20:55
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.

@temawi, let me merge this once you approve, as I'll need to fix up the commit message.

@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 Mar 28, 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 Mar 28, 2024
@ejona86 ejona86 merged commit 0064991 into grpc:master Mar 28, 2024
14 checks passed
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.

None yet

6 participants