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.

Sign up for GitHub

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

Jump to bottom

Proper fix utf8 command line arguments #14253

Closed
wants to merge 8 commits into from
Closed

Proper fix utf8 command line arguments #14253

wants to merge 8 commits into from

Conversation

hknielsen
Copy link
Contributor

#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

@hknielsen hknielsen marked this pull request as ready for review September 29, 2023 07:52
@hknielsen hknielsen requested review from a team as code owners September 29, 2023 07:52
@hknielsen hknielsen requested review from haberman and removed request for a team September 29, 2023 07:52
@hknielsen
Copy link
Contributor Author

hknielsen commented Sep 29, 2023 •
edited
Loading

Fyi @fowles
Found that the prev PR didnt work with multi byte chars. This PR fixes that.
As for your initial concern with wchar_t, to not ripple wchar_t out trough the codebase I convert to wstring, ie. when reading the file

@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 2, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 2, 2023
@fowles
Copy link
Contributor

fowles commented Oct 13, 2023

a bunch of stuff in our repo/ci moved around recently. Can you rebase this change?

@fowles
Copy link
Contributor

fowles commented Oct 13, 2023

@hknielsen pending comments and needs a rebase

@hlopko
Copy link
Contributor

hlopko commented Dec 12, 2023

Hi @hknielsen, do you plan to continue working on this CL? Thanks!

@hknielsen
Copy link
Contributor Author

@fowles @hlopko sorry for the delay. I pushed a rebase, let me know if I need to do anything more

@hknielsen hknielsen force-pushed the proper-fix-none-ascii-issue branch from 88b3c6e to cfbdd35 Compare December 13, 2023 08:58
@hlopko hlopko added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 14, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 14, 2023
@hlopko hlopko added the protoc label Dec 15, 2023
@hknielsen
Copy link
Contributor Author

@hlopko @fowles are there anything more I need to do to move this PR along :)?

@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 26, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Dec 26, 2023
@hknielsen hknielsen force-pushed the proper-fix-none-ascii-issue branch from 8e9879b to 881ae8e Compare January 2, 2024 12:13
@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 2, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 2, 2024
@fowles
Copy link
Contributor

fowles commented Jan 6, 2024

yeah, we have to go back to the const_cast because we still support C++14 for 1 more year.

@hknielsen
Copy link
Contributor Author

yeah, we have to go back to the const_cast because we still support C++14 for 1 more year.

Reverted back to const_cast

@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 11, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 11, 2024
@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 11, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 11, 2024
@fowles fowles added bug 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Jan 16, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 16, 2024
@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 16, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 16, 2024
@hknielsen
Copy link
Contributor Author

Tests / Python / MacOS Pure 3.12 (pull_request_target) and feedback/copybara are failing, both of these I cant see what the issue is. @fowles are there something here I need to look into?

@fowles
Copy link
Contributor

fowles commented Jan 19, 2024

Nothing for you to do. There was a merge conflict on internal import that I will handle

copybara-service bot pushed a commit that referenced this pull request Jan 20, 2024
@hknielsen @copybara-github
Proper fix utf8 command line arguments (#14253)
e67b009 
#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 copybara-service bot mentioned this pull request Jan 20, 2024
Merged
@tonydnewell tonydnewell mentioned this pull request Jun 5, 2024
Open
copybara-service bot pushed a commit that referenced this pull request Jul 23, 2024
@acozzette @copybara-github
Remove /utf-8 flag added in #14197
5cf412e 
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 copybara-service bot mentioned this pull request Jul 23, 2024
Merged
copybara-service bot pushed a commit that referenced this pull request Jul 24, 2024
@acozzette @copybara-github
Remove /utf-8 flag added in #14197
7b15966 
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
@acozzette @copybara-github
Remove /utf-8 flag added in #14197
35cfde6 
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
@acozzette @zhangskz
Remove /utf-8 flag added in #14197
e20cb7a 
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 zhangskz mentioned this pull request Jul 25, 2024
Merged
zhangskz pushed a commit that referenced this pull request Jul 25, 2024
@acozzette @zhangskz
Remove /utf-8 flag added in #14197
440dc83 
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 zhangskz mentioned this pull request Jul 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fowles fowles fowles approved these changes

@haberman haberman Awaiting requested review from haberman haberman was automatically assigned from protocolbuffers/protobuf-btr

Assignees
No one assigned
Labels
bug protoc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hknielsen @fowles @hlopko @acozzette