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

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
Member

fowles commented Oct 13, 2023

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

@fowles
Copy link
Member

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

@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
@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
fowles reviewed Jan 5, 2024
View reviewed changes
Copy link
Member

@fowles fowles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last nit, otherwise looks good.

src/google/protobuf/compiler/main.cc Outdated Show resolved Hide resolved
@hknielsen hknielsen requested a review from fowles January 6, 2024 16:01
@fowles fowles added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 6, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 6, 2024
@fowles
Copy link
Member

fowles commented Jan 6, 2024

thanks for all your patience with this PR! At this point the PR will go through some internal machinations, so you won't see updates here for a few days, but it should (unless something goes wrong) land next week. If you don't see any motion next week, feel free to ping me!

@fowles
Copy link
Member

fowles commented Jan 6, 2024

All the windows tests are failing, so I suspect that something is actually wrong in this PR

@hknielsen
Copy link
Contributor Author

hknielsen commented Jan 6, 2024 •
edited

All the windows tests are failing, so I suspect that something is actually wrong in this PR

@fowles hmm it looks like windows are not using the correct compiler flags for c++17, can that be true? I see a bunch of warnings locally about it:
Command line warning D9002 : ignoring unknown option '-std=c++17'

That also explains:
https://github.com/protocolbuffers/protobuf/actions/runs/7432571452/job/20224540608?pr=14253#step:3:736
Since data() noneconst was added in C++17 AFAICS

@fowles
Copy link
Member

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
Member

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
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
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 is a code owner 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.

None yet
4 participants
@hknielsen @fowles @hlopko @acozzette