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.

Sign up for GitHub

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

Jump to bottom

Upgrade netty to 4.1.75 #15145

Closed
wants to merge 3 commits into from
Closed

Upgrade netty to 4.1.75 #15145

wants to merge 3 commits into from

Conversation

vpanta
Copy link
Contributor

@vpanta vpanta commented Mar 30, 2022 •
edited
Loading

In order to fix netty/netty#11815 for users of bazel, this upgrades the libraries under third_party/netty to netty/netty@4.1.75, along with third_party/netty_tcnative to netty/netty_tcnative@2.0.51.

This change needs to also include a grpc-java update to 1.45.1, to fix grpc/grpc-java#8981 which had been preventing netty/netty@4.1.75 from working properly.

This PR has 3 parts/commits, which if necessary can be split and merged separately:

  1. Split out a third_party/grpc-java from third_party/grpc
    • This makes a 1:1 mapping of GithHub repo to third_party directory.
    • Also makes it clearer in practice for these to be different versions (notable for commit 2).
  2. Upgrade to grpc/grpc-java@1.45.1
  3. Upgrade to netty/netty@4.1.75 and netty/netty_tcnative@2.0.51

@vpanta
Copy link
Contributor Author

vpanta commented Mar 30, 2022

Not as simple as I'd hoped, will look deeper tomorrow.

@vpanta vpanta force-pushed the netty75 branch 2 times, most recently from 8199c3e to 11e06b3 Compare March 31, 2022 05:21
@vpanta
Copy link
Contributor Author

vpanta commented Mar 31, 2022

This isn't ready for review unfortunately, I'm still trying to get gRPC updated in addition to netty (due to grpc/grpc-java#8981), but now I'm getting more incompatibilities. Currently, I either get stuck behind one of the following two errors:

ERROR: /private/var/tmp/_bazel_vasilios/a66b1d3a8126aa917c2980f966604cd1/external/com_github_grpc_grpc/BUILD:5264:23: error loading package '@com_google_googleapis//google/rpc': Unable to find package for @com_google_googleapis_imports//:imports.bzl: The repository '@com_google_googleapis_imports' could not be resolved: Repository '@com_google_googleapis_imports' is not defined. and referenced by '@com_github_grpc_grpc//:google_rpc_status_upb'

which is resolvable by removing some of the patched-out contents, but the next blocker is:

ERROR: /private/var/tmp/_bazel_vasilios/a66b1d3a8126aa917c2980f966604cd1/external/com_github_grpc_grpc/BUILD:5254:23: error loading package '@com_github_cncf_udpa//xds/data/orca/v3': at /private/var/tmp/_bazel_vasilios/a66b1d3a8126aa917c2980f966604cd1/external/com_github_cncf_udpa/bazel/api_build_system.bzl:1:6: at /private/var/tmp/_bazel_vasilios/a66b1d3a8126aa917c2980f966604cd1/external/com_envoyproxy_protoc_gen_validate/bazel/pgv_proto_library.bzl:1:6: at /private/var/tmp/_bazel_vasilios/a66b1d3a8126aa917c2980f966604cd1/external/io_bazel_rules_go/proto/compiler.bzl:25:5: at /private/var/tmp/_bazel_vasilios/a66b1d3a8126aa917c2980f966604cd1/external/io_bazel_rules_go/go/private/rules/transition.bzl:32:5: Unable to find package for @io_bazel_rules_go_name_hack//:def.bzl: The repository '@io_bazel_rules_go_name_hack' could not be resolved: Repository '@io_bazel_rules_go_name_hack' is not defined. and referenced by '@com_github_grpc_grpc//:xds_orca_upb'

which is its own bag of "I don't know how to untangle this"...

@vpanta vpanta requested review from a team as code owners April 1, 2022 16:36
@vpanta vpanta force-pushed the netty75 branch 2 times, most recently from 1fa0f44 to 00d6d60 Compare April 4, 2022 00:57
@vpanta
Copy link
Contributor Author

vpanta commented Apr 4, 2022

Ok, this is now ready for review. It's three distinct commits, each of which is probably viewed/reviewed on their own. I've updated the PR request above to go more into details about each commit and their purpose.

@vpanta vpanta mentioned this pull request Apr 4, 2022
Closed
coeuvre
coeuvre approved these changes Apr 5, 2022
View reviewed changes
Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

Thanks!

Can you rebase on master?

@coeuvre
Copy link
Member

coeuvre commented Apr 5, 2022

And unfortunately, we cannot import a PR that contains both third_party and non third_party changes. Can you split the PR into multiple small PRs that each of them works and only contains only third_party change or non third_party change?

@coeuvre
Copy link
Member

coeuvre commented Apr 5, 2022

I understand that moving grpc-java to third_party/grpc-java make it clearer but may introduce troubles when splitting PRs. I recommend first focus on only upgrading gRPC in one PR and clean it up in another one but it's up to you.

@vpanta
Copy link
Contributor Author

vpanta commented Apr 5, 2022

No worries, sounds good, I wasn't sure the right organization of the PRs, but I can rebase/re-structure it for sure. I'll revert this to a Draft for now, and re-open the PRs when they're ready.

@vpanta vpanta marked this pull request as draft April 5, 2022 13:19
@vpanta vpanta changed the base branch from release-5.1.0 to master April 5, 2022 13:52
@vpanta vpanta marked this pull request as ready for review April 5, 2022 14:25
@vpanta vpanta mentioned this pull request Apr 5, 2022
Closed
@vpanta
Copy link
Contributor Author

vpanta commented Apr 5, 2022

Well, that took a lot less than I'd feared. The only part which is not in third_party at the moment is the change to minimal_jdk_test.sh, so I just sent out #15182 to make that change first before this one can go in.

@sgowroji sgowroji added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Apr 6, 2022
bazel-io pushed a commit that referenced this pull request Apr 6, 2022
@vpanta @copybara-github
Increase allowed size for install base pre-netty@4.1.75 upgrade
afa02a8 
#15145 bumps the install base to just over 311MB, so we need to update the allowable size before that `third_party` change gets merged

Closes #15182.

PiperOrigin-RevId: 439782557
@vpanta
Split third_party/grpc into grpc and grpc-java [1/3]
799522d 
Moves grpc-java jars to separate dir but keep BUILD structure for non-third-party references
@vpanta
Update grpc-java to v1.45.1
a401868
@vpanta
Upgrade netty to 4.1.75
10b77a2
@vpanta
Copy link
Contributor Author

vpanta commented Apr 6, 2022

Updated with a master rebase now that #15182 is in via afa02a8

coeuvre
coeuvre approved these changes Apr 7, 2022
View reviewed changes
Copy link
Member

@coeuvre coeuvre left a comment

Choose a reason for hiding this comment

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

@meteorcloudy Can you help import this?

@bazel-io bazel-io closed this in 68277ec Apr 7, 2022
@vpanta vpanta mentioned this pull request Apr 7, 2022
Closed
bazel-io pushed a commit that referenced this pull request Apr 8, 2022
@vpanta @copybara-github
Split third_party/grpc into grpc and grpc-java [2/3]
5179d03 
Updates non-third_party code to use direct `grpc-java` targets

Follow-up to the split in 68277ec (#15145)

Closes #15193.

PiperOrigin-RevId: 440364565
@vpanta vpanta mentioned this pull request Apr 8, 2022
Closed
3405691582
3405691582 reviewed Jul 22, 2022
View reviewed changes
third_party/BUILD
"//src/conditions:linux_aarch64": ["netty_tcnative/netty-tcnative-boringssl-static-2.0.44.Final-linux-aarch_64.jar"],
"//src/conditions:linux_x86_64": ["netty_tcnative/netty-tcnative-boringssl-static-2.0.44.Final-linux-x86_64.jar"],
"//src/conditions:windows": ["netty_tcnative/netty-tcnative-boringssl-static-2.0.44.Final-windows-x86_64.jar"],
"//conditions:default": ["netty_tcnative/netty-tcnative-boringssl-static-2.0.44.Final.jar"],
Copy link
Contributor

Choose a reason for hiding this comment

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

This leaves the default case broken. Should this just be netty-tcnative-classes-2.0.51.Final.jar, or is something more elaborate required?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@3405691582 3405691582 3405691582 left review comments

@coeuvre coeuvre coeuvre approved these changes

Assignees
No one assigned
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Milestone
No milestone
4 participants
@vpanta @coeuvre @3405691582 @sgowroji