-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 code gen to generate health and reflection stubs #10886
gRPC code gen to generate health and reflection stubs #10886
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments and questions.
if (OS.determineOS() == OS.LINUX) { | ||
return path.replaceAll(" ", "\\ "); | ||
} else { | ||
return path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spaces will work as is on Windows and macOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, I don't really have a way to check.
@rsvoboda reported that it doesn't work on linux
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To shed some more light on it, the paths are passed as separate arguments in the ProcessBuilder
's command, so they should be okay without backslashes before spaces. It's protoc
that doesn't handle them properly on Linux. I don't know about other systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @cescoffier can check on macOS if you can put together a test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cescoffier checked and this version (without \
before spaces) worked for him on Mac
f829066
to
5964ae0
Compare
5964ae0
to
3ecb368
Compare
I updated the doc by the way |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some small comments on the doc.
if (OS.determineOS() == OS.LINUX) { | ||
return path.replaceAll(" ", "\\ "); | ||
} else { | ||
return path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @cescoffier can check on macOS if you can put together a test case?
160c7bc
to
bbb79f1
Compare
fixes #10214