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

fix: Fix local files with UTF8 names #1246

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

joeyparrish
Copy link
Member

@joeyparrish joeyparrish commented Jul 18, 2023

This fixes our use of std::filesystem to interpret all paths names as UTF8. Before this, UTF8 paths did not work correctly in all cases.

This also adds a new unit test to cover this case.

On Windows, it is critical that a UTF8 locale be set at runtime. Applications linking with Packager as a library should call setlocale(), and the Packager frontends now do this automatically after converting wide character arguments into narrow strings.

Closes #652

@cosmin
Copy link
Contributor

cosmin commented Jul 18, 2023

LocalFileTest.UnicodePath seems to be failing on Windows.

@joeyparrish
Copy link
Member Author

Yeah, I thought I had this right. file->Size() isn't working for some reason, only on Windows with Unicode paths, even though reading and writing is fine. I'll convert this back to draft while I iterate on it.

@joeyparrish joeyparrish marked this pull request as draft July 18, 2023 15:06
@joeyparrish joeyparrish force-pushed the unicode-path-test branch 2 times, most recently from 8c6327d to bb31a30 Compare July 18, 2023 16:46
@joeyparrish joeyparrish marked this pull request as ready for review July 18, 2023 16:48
This fixes our use of std::filesystem to interpret all paths names as
UTF8.  Before this, UTF8 paths on Windows did not work correctly.

This also adds a new unit test to cover this case.

Closes shaka-project#652
@joeyparrish
Copy link
Member Author

Okay, this is finally working. I was able to verify it on a Windows machine in our lab before uploading this latest version. The critical bit is setlocale(), since we are explicitly interpretting narrow string paths as UTF-8. We need to set the locale to UTF8 for functions like fopen() to do the right thing with those narrow strings. We also need to setlocale() in wmain(), after converting wchar_t argv into UTF-8, for the same reason.

@joeyparrish joeyparrish merged commit 5a2571b into shaka-project:cmake Jul 18, 2023
@joeyparrish joeyparrish deleted the unicode-path-test branch July 18, 2023 18:59
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Sep 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants