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

Windows - Fix handling of utf8 command line arguments #17854

Closed

Conversation

tonydnewell
Copy link
Contributor

@tonydnewell tonydnewell commented Aug 19, 2024

This relates to:

Issues:

The fix in #15519 has one line missing that breaks Grpc.Tools and doesn't actually fix the problem it proports to fix.

This PR fixes this.

This fix needs to also be applied to the version of protobuf that is used by gRPC.

There are various scenarios that need to be tested on Windows:

A. Non-ascii command line arguments, e.g.

protoc.exe --csharp_out=out --proto_path=E:\work\grpc\protoctest\test-Dré E:\work\grpc\protoctest\test-Dré\helloworld.proto

Failed output:

E:\work\grpc\protoctest\test-DrΘ: warning: directory does not exist.
Could not make proto path relative: E:\work\grpc\protoctest\test-DrΘ\helloworld.proto: No such file or directory

Success output:

  • no errors printed
  • generated .cs file in the out directory

B. Non-ascii arguments in a file containing the protoc arguments (no path to file) e.g.:

protoc.exe @test.rsp

where test.rsp contains:

--plugin=protoc-gen-grpc=E:\work\grpc\protoctest\tools\grpc_csharp_plugin.exe
--csharp_out=E:\work\grpc\protoctest\test-Dré\out
--grpc_out=E:\work\grpc\protoctest\test-Dré\out
--proto_path=E:\work\grpc\protoctest\test-Dré
helloworld.proto

Success output for both old and fixed code:

  • no errors printed
  • generated .cs file in the out directory

C. Non-ascii arguments in a file containing the protoc arguments (with non-ascii path to file).

(This is what Grpc.Tools uses.) e.g.

protoc.exe @E:\work\grpc\protoctest\test-Dré\test.rsp

Failed output:

Failed to open argument file: E:\work\grpc\protoctest\test-DrΘ\test.rsp

Success output:

  • no errors printed
  • generated .cs file in the out directory

@tonydnewell tonydnewell requested a review from a team as a code owner August 19, 2024 12:07
@tonydnewell tonydnewell requested review from fowles and removed request for a team August 19, 2024 12:07
@acozzette acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Aug 28, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Aug 28, 2024
@acozzette
Copy link
Member

@tonydnewell Thanks for the bug fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants