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

[grpc] Upgrade to gRPC-1.26.0 #9363

Merged
merged 8 commits into from
Jan 6, 2020
Merged

[grpc] Upgrade to gRPC-1.26.0 #9363

merged 8 commits into from
Jan 6, 2020

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Dec 18, 2019

This updates gRPC to 1.26.0. I removed a number of patches that are no longer needed,
and updated existing patches to match the new line numbers.

This fixes #8453

I do not believe this patch breaks or fixes any particular triplets, but I am less certain about the UWP triplets.

@JackBoosY
Copy link
Contributor

JackBoosY commented Dec 19, 2019

From UWP build log:

8>C:\vsts\_work\4\s\buildtrees\grpc\src\v1.26.0-152e906888\src\core\lib\security\credentials\alts\check_gcp_environment_windows.cc(48,15): error C2039:  'RegGetValueA': is not a member of '`global namespace'' [C:\vsts\_work\4\s\buildtrees\grpc\x64-uwp-dbg\grpc.vcxproj]

The documentation shows:

   
Minimum supported client Windows Vista, Windows XP Professional x64 Edition [desktop apps only]
Minimum supported server Windows Server 2008, Windows Server 2003 with SP1 [desktop apps only]

So I think the function RegGetValue needs to be changed to another suitable function.

@coryan
Copy link
Contributor Author

coryan commented Dec 19, 2019

From UWP build log:

[snip]

So I think the function RegGetValue needs to be changed to another suitable function.

I disabled the GCE checks on UWP, just like they were on the previous version. I really doubt folks will be deploying UWP applications to GCE instances, and the only downside (that I know of, at least at this time) is some more complicated authentication configuration if you are trying to access Google services via gRPC.

@JackBoosY JackBoosY requested a review from PhoebeHui December 20, 2019 02:23
@PhoebeHui
Copy link
Contributor

@jozefizso, would you like to help review this PR?

@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Dec 20, 2019
@jozefizso
Copy link
Contributor

@PhoebeHui definitely not. You threw away my contribution and I should work on someone else code? Why?

@dan-shaw
Copy link
Contributor

@coryan This PR looks good to me. We're going to merge #9135 before merging this one.

@coryan
Copy link
Contributor Author

coryan commented Dec 23, 2019

@dan-shaw that is fine. Once you do, I will see if a simple rebase works, but if not I might have to close this PR and start over (no biggie if that is the case).

In the future, would you like me to preserve existing patches? What if they stop working? Should I delete them or replace them?

@dan-shaw
Copy link
Contributor

@coryan Thanks - that would be great. The only conflict should be the UWP patch.

If patches stop working (but are still relevant), update the respective patch files.
If patches stop working (and are not relevant anymore), remove the patch files.

In either case, if a patch file is unsuccessfully applied, it should fail in the CI.

@dan-shaw
Copy link
Contributor

dan-shaw commented Jan 2, 2020

@coryan I've merged #9135

@coryan
Copy link
Contributor Author

coryan commented Jan 3, 2020

I just rebased this PR, in detail, this is why I changed the patches:

  • The 00001-*.patch modified 3 files:

    • The changes to CMakeLists.txt did not apply due to line number and context changes. I manually applied the same changes, except for the gRPC_BUILD_CODEGEN changes that they do not seem to be applicable anymore.
    • I updated the patch to src/core/lib/iomgr/resource_quota.cc.
    • The code in src/core/lib/security/credentials/alts/check_gcp_environment_windows.cc changed, I made an equivalent patch (remove GCE detection when compiled with UWP).
  • The 0000[235]-*.patch files needed to be updated with new line numbers and context.

  • The 00007-*.patch file is no longer needed, equivalent changes to disable the grpc_channelz with protobuf lite are applied upstream.

I installed grpc with x64-linux, x64-uwp, x64-windows-static, and x86-windows. We will see what the CI system has to say.

@coryan
Copy link
Contributor Author

coryan commented Jan 3, 2020

@dan-shaw PTAL

@dan-shaw dan-shaw merged commit ebe5050 into microsoft:master Jan 6, 2020
@dan-shaw
Copy link
Contributor

dan-shaw commented Jan 6, 2020

LGTM

@coryan coryan deleted the upgrade-grpc-1.26.0 branch January 13, 2020 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[grpc] update to 1.24.1
5 participants