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

all: Update netty to 4.1.77.Final and netty_tcnative to 2.0.53.Final #9027

Merged
merged 7 commits into from
Jun 24, 2022

Conversation

temawi
Copy link
Contributor

@temawi temawi commented Mar 29, 2022

This is a more complicated Netty upgrade as netty-tcnative was split into:

  1. netty-tcnative-classes that has all the Java code
  2. netty-tcnative-boringssl-static that no longer has all of the native libraries, but instead declares dependencies to itself with a platform specific Maven classifiers (e.g. linux-x86_64)

This causes problems for:

  1. Gradle - it does not correctly resolve these classifiers, resulting in no native libraries being included in the build.
  2. Bazel - it thinks the dependencies with qualifiers are a cyclical dependency and fails the build.

This PR includes direct dependencies to netty-tcnative-boringssl-static with all the platform classifiers.

The latest Bazel release also has a problem with using platform classifiers. This PR changes to use a non-released version of rules_jvm_external that has a fix to the problem (bazel-contrib/rules_jvm_external#687).

@temawi temawi force-pushed the upgrade-netty branch 3 times, most recently from 263f128 to 4ffb3cb Compare March 30, 2022 17:31
@temawi temawi changed the title all: Update netty to 4.1.75.Final and netty_tcnative to 2.0.50.Final all: Update netty to 4.1.75.Final and netty_tcnative to 2.0.51.Final Mar 30, 2022
@temawi temawi force-pushed the upgrade-netty branch 3 times, most recently from 711238b to ce8b750 Compare June 14, 2022 02:16
@temawi temawi changed the title all: Update netty to 4.1.75.Final and netty_tcnative to 2.0.51.Final all: Update netty to 4.1.77.Final and netty_tcnative to 2.0.53.Final Jun 14, 2022
@temawi temawi force-pushed the upgrade-netty branch 2 times, most recently from e325ab0 to ba76107 Compare June 14, 2022 02:49
@temawi temawi requested a review from ejona86 June 23, 2022 19:10
@temawi temawi marked this pull request as ready for review June 23, 2022 19:10
SECURITY.md Outdated Show resolved Hide resolved
SECURITY.md Outdated Show resolved Hide resolved
@@ -57,6 +57,12 @@
<version>${netty.tcnative.version}</version>
<scope>runtime</scope>
</dependency>
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-tcnative-classes</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect this not to be necessary for Maven. Remove this dependency?

Although, actually, I see you didn't update the build.gradle for this example. It is using grpc-netty-shaded. We should swap to using grpc-netty-shaded here and then we don't need to define netty-tcnative at all. I've tested that works without issue.

@temawi temawi merged commit 7bd0797 into grpc:master Jun 24, 2022
@temawi temawi deleted the upgrade-netty branch June 24, 2022 17:47
larry-safran pushed a commit to larry-safran/grpc-java that referenced this pull request Jun 28, 2022
…rpc#9027)

all: Update netty to 4.1.77.Final and netty_tcnative to 2.0.53.Final

Also switches to a non-release version of rules_jvm_external to allow Bazel build to work with artifact classifiers.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2022
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.

2 participants