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

bazel: Export deps from maven stand-in targets #9780

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

ejona86
Copy link
Member

@ejona86 ejona86 commented Jan 3, 2023

If an artifact on Maven Central exposes a type from gRPC on its API surface, then consumers of that artifact need that gRPC API in the compile classpath. Bazel handles this by making hjars for transitive dependencies, but if the dependencies are runtime_deps then Bazel won't generate hjars containing the needed symbols.

We don't export netty-shaded because the classes already don't match Maven Central. If an artifact on Maven Central is exposing a netty-shaded class on its API surface, it wouldn't work anyway since the class simply doesn't exist for the Bazel build.

Fixes #9772

If an artifact on Maven Central exposes a type from gRPC on its API
surface, then consumers of that artifact need that gRPC API in the
compile classpath. Bazel handles this by making hjars for transitive
dependencies, but if the dependencies are runtime_deps then Bazel won't
generate hjars containing the needed symbols.

We don't export netty-shaded because the classes already don't match
Maven Central. If an artifact on Maven Central is exposing a
netty-shaded class on its API surface, it wouldn't work anyway since the
class simply doesn't exist for the Bazel build.

Fixes grpc#9772
@ejona86 ejona86 requested a review from sanjaypujare January 3, 2023 18:13
@ejona86 ejona86 added the TODO:backport PR needs to be backported. Removed after backport complete label Jan 3, 2023
@ejona86
Copy link
Member Author

ejona86 commented Jan 3, 2023

CC @larry-safran

"@io_netty_netty_handler//jar",
"@io_netty_netty_handler_proxy//jar",
"@io_netty_netty_resolver//jar",
"@io_netty_netty_transport//jar",
"@io_netty_netty_transport_native_unix_common//jar",
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this from line 23 - is that necessary? What does it achieve?

Copy link
Contributor

Choose a reason for hiding this comment

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

It puts them in alphabetical order which makes maintenance easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Buildifier did it because it was out-of-order. Somehow it had been missed before.

Copy link
Contributor

@sanjaypujare sanjaypujare left a comment

Choose a reason for hiding this comment

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

LGTM: left one comment about what might be an unneeded change

@ejona86 ejona86 merged commit f0614e5 into grpc:master Jan 3, 2023
@ejona86 ejona86 deleted the bazel-exports-maven branch January 3, 2023 21:36
@ejona86 ejona86 removed the TODO:backport PR needs to be backported. Removed after backport complete label Jan 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

//services:services_maven should declare a compile-time dependency on //services:reflection
3 participants