-
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
bazel: Support maven_install #6553
Conversation
examples/WORKSPACE
Outdated
"com.google.api.grpc:proto-google-cloud-pubsub-v1:0.1.24", | ||
] + IO_GRPC_GRPC_JAVA_ARTIFACTS, | ||
generate_compat_repositories = True, | ||
maven_install_json = "//:maven_install.json", |
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.
Could this be too much for examples? It may add difficulty for users to copy the example and make changes. Even the examples of rules_jvm_external
itself are not always pinning the maven artifacts. https://github.com/bazelbuild/rules_jvm_external/tree/master/examples
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.
Hmm... Good point. I'd rather have the examples be as "real" as possible, and I expect most Bazel users will want to pin. I've added a comment to mention how to regenerate the pinned versions.
I'm fine with dropping this if it becomes a problem, but I think I'd prefer to "do it real" first and remove it if it becomes a problem.
Reminder: coordinate with #6574 |
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.
LGTM
So the Bazel failure has shown me that the version pinning here will actually be pretty annoying, because it includes all transitive dependencies and a |
@dapengzhang0, PTAL, just to confirm you are fine with me removing the pinning. I'm going to need to pull in the version change from #6572 as well. |
Using existing_rule() is now the preferred way of avoiding re-defining repositories. The function names were changed to match the name of the repository they add. Normally we would inline all the functions, but that's unnecessary churn since the repositories will mostly be replaced with maven_install() in the future.
http_repository is preferred by Bazel over git_repository.
maven_install is strongly superior to previous forms of grabbing dependencies from Maven as it computes the appropriate versions to use from the full transitive dependencies of all libraries used by an application. It also has much less boilerplate and includes dependencies with generated targets. In the future we will drop the jvm_maven_import_external usages and require maven_install, at which point we can swap to using the `@maven' repository and no longer depend on compat_repositories. Fixes grpc#5359
0055c0b
to
a7c38a2
Compare
maven_install is strongly superior to previous forms of grabbing dependencies
from Maven as it computes the appropriate versions to use from the full
transitive dependencies of all libraries used by an application. It also has
much less boilerplate and includes dependencies with generated targets.
In the future we will drop the jvm_maven_import_external usages and require
maven_install, at which point we can swap to using the `@maven' repository and
no longer depend on compat_repositories.
Fixes #5359
There are three commits. They are best reviewed separately. The http_repository change is non-essential, but I noticed it when working on this and didn't bother splitting it out to a separate PR.