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

Allow root module's override tags to take precedence over the overridees from transitive deps. #1278

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jin
Copy link
Collaborator

@jin jin commented Nov 5, 2024

Occasionally, a bazel_dep can introduce a maven.overrides tag that aliases an artifact to another label. This may not be what the root module desires, and so we should allow the root module's overrides to take precedence over any transitive overrides, but at the caution of the root module owner.

Concretely, this is a problem encountered by the Bazel project itself, where a grpc-java transitive dep overrides @maven//:com_google_protobuf_protobuf_java as an alias to @@protobuf+//:protobuf_java, which was not desirable.

This PR is validated by a test that contains a root override from okhttp to javapoet, which takes precedence over a dependency's override from okhttp to a poison pill label.

@jin jin force-pushed the root-overrides branch 3 times, most recently from 75df6c3 to 2a88bc1 Compare November 5, 2024 13:44
@meteorcloudy
Copy link
Contributor

Thanks! Is it possible for the root module to just remove the override instead of adding a new one? Not sure how to do it properly.

/cc @shs96c

@jin
Copy link
Collaborator Author

jin commented Nov 5, 2024

It's possible, but removing it instead of aliasing it to something else might cause the transitive bazel_dep to break?

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

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

Makes sense

@meteorcloudy
Copy link
Contributor

It's possible, but removing it instead of aliasing it to something else might cause the transitive bazel_dep to break?

So I guess, if the root module wants to revert it back to the jar from maven, they can still do that with an override, right?

@jin
Copy link
Collaborator Author

jin commented Nov 5, 2024

It's possible, but removing it instead of aliasing it to something else might cause the transitive bazel_dep to break?

So I guess, if the root module wants to revert it back to the jar from maven, they can still do that with an override, right?

Yep! That's the idea. The root owner can override it back to the original (or any other target) if they are sure that the transitive dep is ok with it. But outright removing it will probably break the transitive dep.

Copy link
Contributor

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thanks!

artifacts = ["com.squareup:javapoet:1.11.1"],
)

bazel_dep(name = "transitive_module_can_override", version = "0.0.0")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be marked as a dev_dependency so that consumers of us don't try and look up transitive_module_can_override in the BCR.

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

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

The bzlmod example test is failing because the new dep is missing in the BCR.

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.

3 participants