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

Handling of UTF8 command argument files regressed in v26.1 breaking Grpc.Tools #17036

Closed
tonydnewell opened this issue Jun 5, 2024 · 6 comments
Labels

Comments

@tonydnewell
Copy link
Contributor

What version of protobuf and what language are you using?
Version: v26.1
Language: C# (but any will see problem)

What operating system (Linux, Windows, ...) and version?
Windows - any version

What did you do?
This is causing the Grpc.Tools issue grpc/grpc#36518
Grpc.Tools is broken by the protocol buffers change.

Handling of non-ASCII characters in UTF-8 command line argument files is broken. It was working in v25.1 and is not working in V26.1. I suspect the cause of the issue to be the "fix" in #14197

Grpc.Tools V2.62.0 ships with protoc version 25.1
Grpc.Tools V2.63.0 ships with protoc version 26.1

Steps to reproduce:

  • Create a directory on Windows containing a non-ASCII character in its name
  • Use that directory in the a command line file (an @ file)

Example of @ file - test.rps

--plugin=protoc-gen-grpc=E:\work\grpc\protoctest\test-Dré\tools\grpc_csharp_plugin.exe
--grpc_out=E:\work\grpc\protoctest\test-Dré\grpc_out
--proto_path=E:\work\grpc\protoctest\test-Dré
hélloworld.proto
  • example command line on Windows:
E:\work\grpc\protoctest\test-Dré>v1.63.0\protoc.exe @..\test-Dré\test.rsp

What did you expect to see
No errors

What did you see instead?

E:\work\grpc\protoctest\test-Dré>v1.63.0\protoc.exe @..\test-Dré\test.rsp
Failed to open argument file: ..\test-Dr�\test.rsp

** Results with older version (working):**

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


@acozzette
Copy link
Member

Thank you for the bug report. Do you have any suggestions on how to fix it? It sounds like #14197 was supposed to fix this problem, so that is surprising if it actually is the root cause of the problem.

@acozzette acozzette added protoc and removed untriaged auto added to all issues by default when created. labels Jun 10, 2024
@slonopotamus
Copy link

It isn't clear what issue #14197 is fixing at all. Grpc.Tools V2.62.0 works perfectly for us when path contains non-ascii symbols, while V2.63.0 breaks. Maybe just git revert?

@tonydnewell
Copy link
Contributor Author

Without trying to reproduce or examine the original issue that #14197 is trying to fix I'm not sure why this fix was needed.

The original code treated arguments on Windows (both command line and those read from a file) as just a string of UTF-8 bytes and thus no wchar needed for non-ascii characters. The original problem may just have been environmental - not using the correct Windows codepage when running the command (UTF-8 is codepage 65001).

My suggestion would be to revert the fix and then re-investigate the original issue.

@TopSwagCode
Copy link

I wish my parents naming me, could have been simple. But they ended up with "Joshua Jesper Krægpøth Ryder". This has fucked me several times. Just came back from vacation to find out my Dotnet work projects with GRPC / Proto files no longer build:

C:\Users\JoshuaJesperKrægpøth.nuget\packages\grpc.tools\2.64.0\tools\windows_x64\protoc.exe --csharp_out=obj\Debug\net8.0 --proto_path=C:\Users\JoshuaJesperKrægpøth.nuget\packages\grpc.tools\2.64.0\build\native\include --proto_path=Protos --dependency_out=obj\Debug\net8.0\12fbc7ffe642ca7e_common.protodep --error_format=msvs Protos/common.proto
0>Failed to open argument file: Error : C:\Users\JoshuaJesperKrµgp°th\AppData\Local\Temp\MSBuildTemp\tmpd22c16c07e134b838bcbee401f0f1fc2.rsp

Here's the kicker. I don't get to pick my windows name, since it's a work laptop and is predefined from sysadmins..... I am so happy they chose to pick Krægpøth instead of Ryder.....

Long rant aside, is there a estimate when this will be fixed ? :D

copybara-service bot pushed a commit that referenced this issue Jul 23, 2024
We have received several reports in #17036 that the addition of this flag
actually broke the use of command argument files with non-ASCII characters in
their names. It looks like #14253 ended up fixing the original issue with a
different solution anyway. Hopefully this change fixes the issue with non-ASCII
characters.

PiperOrigin-RevId: 655201610
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
We have received several reports in #17036 that the addition of this flag
actually broke the use of command argument files with non-ASCII characters in
their names. It looks like #14253 ended up fixing the original issue with a
different solution anyway. Hopefully this change fixes the issue with non-ASCII
characters.

PiperOrigin-RevId: 655201610
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
We have received several reports in #17036 that the addition of this flag
actually broke the use of command argument files with non-ASCII characters in
their names. It looks like #14253 ended up fixing the original issue with a
different solution anyway. Hopefully this change fixes the issue with non-ASCII
characters.

PiperOrigin-RevId: 655660885
@acozzette
Copy link
Member

This should hopefully be fixed now with #17581.

zhangskz pushed a commit that referenced this issue Jul 25, 2024
We have received several reports in #17036 that the addition of this flag
actually broke the use of command argument files with non-ASCII characters in
their names. It looks like #14253 ended up fixing the original issue with a
different solution anyway. Hopefully this change fixes the issue with non-ASCII
characters.

PiperOrigin-RevId: 655660885
zhangskz pushed a commit that referenced this issue Jul 25, 2024
We have received several reports in #17036 that the addition of this flag
actually broke the use of command argument files with non-ASCII characters in
their names. It looks like #14253 ended up fixing the original issue with a
different solution anyway. Hopefully this change fixes the issue with non-ASCII
characters.

PiperOrigin-RevId: 655660885
@tonydnewell
Copy link
Contributor Author

@acozzette Can we reopen this please. #17581 didn't fix the issue.
I've created a PR that does fix the issue: #17854

copybara-service bot pushed a commit that referenced this issue Aug 29, 2024
This relates to:

Issues:
- #17036
- grpc/grpc#36518
PRs:
- #15519

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

Closes #17854

COPYBARA_INTEGRATE_REVIEW=#17854 from tonydnewell:windows-utf8-command-fix de9ec54
PiperOrigin-RevId: 669083804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants