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

Support non ascii commandline args #14197

Conversation

hknielsen
Copy link
Contributor

@hknielsen hknielsen commented Sep 25, 2023

Using none ASCII chars for protoc on Windows are not handled well.
This adresses argument file at a location where either directory, or filename contains Unicode chars.

Specifically because MsBuild tool saves argument file in with the UserName appended to the .rsp file.
dotnet/msbuild#9232

But in general we should really handle if none ascii chars are in the path.

@google-cla
Copy link

google-cla bot commented Sep 25, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@hknielsen hknielsen force-pushed the support-non-ascii-commandline-args branch from f55efe2 to fa210c4 Compare September 25, 2023 11:20
@hknielsen hknielsen marked this pull request as ready for review September 25, 2023 11:20
@hknielsen hknielsen requested review from a team as code owners September 25, 2023 11:20
@hknielsen hknielsen requested review from haberman and removed request for a team September 25, 2023 11:20
@hknielsen
Copy link
Contributor Author

@haberman im not sure how to fix the "unsafe fork" issue, do I need to do anything?

@fowles
Copy link
Contributor

fowles commented Sep 25, 2023

Instead of shifting the code to wchar's can we shift the environment to utf-8 as outlined in https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page ?

@hknielsen hknielsen force-pushed the support-non-ascii-commandline-args branch from fa210c4 to 673d575 Compare September 26, 2023 07:01
@hknielsen
Copy link
Contributor Author

hknielsen commented Sep 26, 2023

@fowles thanks for the question! That let me on to this:
https://learn.microsoft.com/en-us/cpp/build/reference/utf-8-set-source-and-executable-character-sets-to-utf-8?view=msvc-170
I agree its much better than starting to go to wchar. Changed so we on windows compile with the /utf-8 flag

@fowles fowles added c++ 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Sep 26, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 26, 2023
copybara-service bot pushed a commit that referenced this pull request Jan 20, 2024
#14197
Tried to fix utf-8 issue, but it didnt handle multibyte chars.
Only way I found that works constantly is using `CommandLineToArgvW`.
To not ripple out `wchar_t`, I convert to and from where needed

Closes #14253

COPYBARA_INTEGRATE_REVIEW=#14253 from hknielsen:proper-fix-none-ascii-issue cad753e
FUTURE_COPYBARA_INTEGRATE_REVIEW=#14253 from hknielsen:proper-fix-none-ascii-issue cad753e
PiperOrigin-RevId: 599826579
copybara-service bot pushed a commit that referenced this pull request Jan 20, 2024
#14197
Tried to fix utf-8 issue, but it didnt handle multibyte chars.
Only way I found that works constantly is using `CommandLineToArgvW`.
To not ripple out `wchar_t`, I convert to and from where needed

Closes #14253

COPYBARA_INTEGRATE_REVIEW=#14253 from hknielsen:proper-fix-none-ascii-issue cad753e
PiperOrigin-RevId: 599990369
copybara-service bot pushed a commit that referenced this pull request 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 pull request 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 pull request 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
zhangskz pushed a commit that referenced this pull request 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 pull request 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 added a commit that referenced this pull request Jul 25, 2024
zhangskz added a commit that referenced this pull request Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants