Skip to content

Use x64 tools on 64-bit Windows in Grpc.Tools#17437

Closed
kkm000 wants to merge 1 commit intogrpc:v1.17.xfrom
kkm000:13098-use-x64-tools-on-windows-x64
Closed

Use x64 tools on 64-bit Windows in Grpc.Tools#17437
kkm000 wants to merge 1 commit intogrpc:v1.17.xfrom
kkm000:13098-use-x64-tools-on-windows-x64

Conversation

@kkm000
Copy link
Copy Markdown
Contributor

@kkm000 kkm000 commented Dec 7, 2018

The Windows Nano Server Docker image does not support 32-bit executables. See #13207 (comment) and next 2 comments. Thanks to @hagen93 for bringing up the issue!

Docs: https://docs.microsoft.com/windows-server/get-started/getting-started-with-nano-server#important-differences-in-nano-server

I confirmed generation working on my Windows 10 x64 machine:

_Protobuf_CoreCompile:
  C:\Users\kkm\.nuget\packages\grpc.tools\1.17.1-dev2\tools\windows_x64\protoc.exe --csharp_out=obj\Debug\netstandard1.5 --plugin=protoc-gen-grpc=C:\Users\kkm\.nuget\packages\grpc.tools\1.17.1-dev2\tools\windows_x64\grpc_csharp_plugin.exe --grpc_out=obj\Debug\netstandard1.5 --proto_path=C:\Users\kkm\.nuget\packages\grpc.tools\1.17.1-dev2\build\native\include --proto_path=..\..\..\protos --dependency_out=obj\Debug\netstandard1.5\50c9e7acc97344d0_helloworld.protodep ../../../protos/helloworld.proto

Both protoc and the plugin are run from the windows_x64 package directory now.

(If only I could come up with unit tests for the scripts!)

I was so wrong in #13098 about Windows always being able to run 32-bit processes! You never know...

@jtattermusch, I am targeting the v1.17.x branch, as this is essentially a hotfix, correct? Are there release notes that need to be updated?

Close: #13098 (wontfix)

@kkm000 kkm000 mentioned this pull request Dec 7, 2018
Merged
NN---
NN--- reviewed Dec 11, 2018
View reviewed changes
Copy link
Copy Markdown

@NN--- NN--- left a comment

Choose a reason for hiding this comment

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

Looks great

@jtattermusch jtattermusch mentioned this pull request Dec 17, 2018
Closed
@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Dec 17, 2018

@kkm000 the change itself looks good, thanks for the fix.
I think I prefer merging to master (rather than to v1.17.x) though, because the bug doesn't seem to be severe enough to warrant a patch release (which has some overhead). Also, because v1.17.x is the first release with msbuild integration, it is reasonable to expect there might be some flaws. If there are multiple fixes, we can reconsider doing a C# patch release.

@jtattermusch jtattermusch added the release notes: yes Indicates if PR needs to be in release notes label Dec 17, 2018
@jtattermusch jtattermusch changed the base branch from v1.17.x to master December 17, 2018 16:35
@jtattermusch jtattermusch changed the base branch from master to v1.17.x December 17, 2018 16:35
@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Dec 17, 2018

I created #17524

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 17, 2018 

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,503      Total (=)      2,020,503

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,175,629      Total (<)     11,175,632

 No significant differences in binary sizes

jtattermusch added a commit that referenced this pull request Dec 18, 2018
@jtattermusch
Merge pull request #17524 from jtattermusch/64bit_windows_grpc_tools
70a4915 
Use x64 tools on 64-bit Windows in Grpc.Tools (#17437 for master branch)
@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Dec 18, 2018

#17524 has been merged into master. The fix will be available in the v1.18.x (once it's released), until then a daily build of Grpc.Tools with the fix will be available on https://packages.grpc.io/ soon.

@kkm000 kkm000 deleted the 13098-use-x64-tools-on-windows-x64 branch December 18, 2018 07:00
@kkm000
Copy link
Copy Markdown
Contributor Author

kkm000 commented Dec 18, 2018

/cc @hagen93, FYI, the fix is targeting 1.18. I know that you've switched to the full server image already, but if you happen to have time and desire to check, I'd appreciate getting your pass/fail report!

@hagen93
Copy link
Copy Markdown

hagen93 commented Dec 21, 2018

@kkm000 I'd love to try this one and see if it fixes the problem but I can't seem to figure out how to install the package. As far as I can tell the daily packages are not uploaded to a NuGet feed anywhere which makes it quite cumbersome to test this issue. Any pointers?

@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Dec 21, 2018

@hagen93 you need to create a local nuget feed (just a local directory) and you can install the nuget package from there https://medium.com/@churi.vibhav/creating-and-using-a-local-nuget-package-repository-9f19475d6af8

@hagen93
Copy link
Copy Markdown

hagen93 commented Dec 21, 2018

@kkm000 Can confirm that it works now 😄 @jtattermusch Do you have any info about when the 1.18 release will be available so we can start using this fix?

@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Dec 21, 2018

@hagen93 https://github.com/grpc/grpc/blob/master/doc/grpc_release_schedule.md

@kkm000
Copy link
Copy Markdown
Contributor Author

kkm000 commented Dec 21, 2018

@hagen93 Thanks for verifying the fix!

@lock lock bot locked as resolved and limited conversation to collaborators Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@NN--- NN--- NN--- left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jtattermusch jtattermusch

Labels

release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kkm000 @jtattermusch @grpc-testing @hagen93 @NN--- @grpc-kokoro