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

Add grpc v1.56.3 #1246

Merged
merged 3 commits into from
Jan 23, 2024
Merged

Add grpc v1.56.3 #1246

merged 3 commits into from
Jan 23, 2024

Conversation

mering
Copy link
Contributor

@mering mering commented Jan 2, 2024 •
edited
Loading

@fmeum fmeum added the skip-url-stability-check Skip the URL stability check for the PR label Jan 2, 2024
@mering mering force-pushed the grpc-v1.56.3 branch 3 times, most recently from d7f0440 to 8eba527 Compare January 2, 2024 16:47
@katre
Copy link
Member

katre commented Jan 4, 2024

The latest grpc release appears to be 1.60.0. Is there any reason you're adding 1.56.3 instead of 1.60.0? (I need 1.60.0 in order to get rid of the upb dependency).

@mering
Copy link
Contributor Author

mering commented Jan 4, 2024 •
edited
Loading

Yes, because this is the version we are currently using in our WORKSPACE and we are currently in the migration process to Bzlmod. I think there is no problem in packaging more/all versions to BCR, so I would suggest to go ahead and also package 1.60.0.

@katre
Copy link
Member

katre commented Jan 4, 2024

Thanks!

@mering mering force-pushed the grpc-v1.56.3 branch 3 times, most recently from e41f161 to f568d80 Compare January 15, 2024 16:57
@mering
Copy link
Contributor Author

mering commented Jan 15, 2024

It seems that the CI compiler has been upgraded since opening this PR resulting in additional warnings which are treated as errors by the upb dependency. What is the beset way to fix this? Do I need to add a patch to the upb dependency in this PR as well now?

@mering mering force-pushed the grpc-v1.56.3 branch 4 times, most recently from 7dae02f to aa9e38e Compare January 18, 2024 23:20
@mering
Copy link
Contributor Author

mering commented Jan 18, 2024

@Wyverald could you please trigger the CI here as well?

@mering mering force-pushed the grpc-v1.56.3 branch 2 times, most recently from c94407a to 64fcbb8 Compare January 18, 2024 23:40
@Wyverald Wyverald added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label Jan 18, 2024
@mering mering force-pushed the grpc-v1.56.3 branch 5 times, most recently from 633a218 to 2b7a7ed Compare January 19, 2024 15:17
meteorcloudy
meteorcloudy previously requested changes Jan 19, 2024
View reviewed changes
modules/grpc/1.56.3/MODULE.bazel Outdated Show resolved Hide resolved
fmeum
fmeum reviewed Jan 19, 2024
View reviewed changes
modules/grpc/1.56.3/MODULE.bazel Outdated Show resolved Hide resolved
modules/grpc/1.56.3/MODULE.bazel Outdated Show resolved Hide resolved
@mering mering marked this pull request as draft January 20, 2024 10:21
@mering
Copy link
Contributor Author

mering commented Jan 20, 2024 •
edited
Loading

The other issue seems to be an upstream one in rules_python. @rickeylev pointed me to bazelbuild/rules_python#1696 (comment) where he discovered and fixed this bug. Marking as "Draft" until there is a rules_python release with the fix included.

@mering mering force-pushed the grpc-v1.56.3 branch 14 times, most recently from b79cc8d to e0c6087 Compare January 23, 2024 13:02
@mering
Copy link
Contributor Author

mering commented Jan 23, 2024

I resolved the Python issue. I updated to the newly release rules_python 0.29.0 but the issue still occurred. Then I noticed that when building from a dependent module without setting the Python toolchain there, it uses some default. So I declared many compatible Python versions which seems to be the way to go such that it just picks the correct one.

@mering mering marked this pull request as ready for review January 23, 2024 13:09
@Wyverald Wyverald dismissed meteorcloudy’s stale review January 23, 2024 20:59

requested change has been made

@Wyverald Wyverald enabled auto-merge (squash) January 23, 2024 21:00
@Wyverald Wyverald merged commit e8c5009 into bazelbuild:main Jan 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmeum fmeum fmeum left review comments

@Wyverald Wyverald Wyverald approved these changes

@meteorcloudy meteorcloudy Awaiting requested review from meteorcloudy

Assignees
No one assigned
Labels
presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval skip-url-stability-check Skip the URL stability check for the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mering @katre @Wyverald @meteorcloudy @fmeum