Skip to content

Proper fix utf8 command line arguments#14253

Closed
hknielsen wants to merge 8 commits intoprotocolbuffers:mainfrom
hknielsen:proper-fix-none-ascii-issue
Closed

Proper fix utf8 command line arguments#14253
hknielsen wants to merge 8 commits intoprotocolbuffers:mainfrom
hknielsen:proper-fix-none-ascii-issue

Conversation

@hknielsen
Copy link
Copy Markdown
Contributor

@hknielsen hknielsen commented Sep 29, 2023

#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
Copy Markdown
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 reviewed Oct 2, 2023
View reviewed changes
Comment thread src/google/protobuf/compiler/command_line_interface.cc Outdated
Comment thread src/google/protobuf/compiler/main.cc Outdated
@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
Copy Markdown
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
Copy Markdown
Contributor

fowles commented Oct 13, 2023

@hknielsen pending comments and needs a rebase

@hlopko
Copy link
Copy Markdown
Contributor

hlopko commented Dec 12, 2023

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

@hknielsen
Copy link
Copy Markdown
Contributor Author

hknielsen commented Dec 13, 2023

@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
Copy Markdown
Contributor Author

hknielsen commented Dec 20, 2023

@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
fowles
fowles reviewed Dec 29, 2023
View reviewed changes
Comment thread src/google/protobuf/compiler/main.cc Outdated
@hknielsen hknielsen force-pushed the proper-fix-none-ascii-issue branch from 8e9879b to 881ae8e Compare January 2, 2024 12:13
fowles
fowles reviewed Jan 2, 2024
View reviewed changes
Comment thread src/google/protobuf/compiler/main.cc
@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
Copy Markdown
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
Copy Markdown
Contributor Author

hknielsen commented Jan 11, 2024

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
Copy Markdown
Contributor Author

hknielsen commented Jan 18, 2024

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
Copy Markdown
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
Closed
copybara-service Bot pushed a commit that referenced this pull request Jul 23, 2024
@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
@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
@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
@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
@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

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

1 more reviewer

@fowles fowles fowles approved these changes

Reviewers whose approvals may not affect merge requirements

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