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

Windows: add a manifest to use UTF-8 code pages #1900

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

wantehchang
Copy link
Collaborator

As of Windows Version 1903 (May 2019 Update), we can use the ActiveCodePage property in the fusion manifest for unpackaged apps to force a process to use UTF-8 as the process code page. This method requires no changes to the source code.

https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page#set-a-process-code-page-to-utf-8

@wantehchang wantehchang marked this pull request as draft December 23, 2023 03:02
@kmilos
Copy link
Contributor

kmilos commented Dec 23, 2023

Please add it via a .rc resource file wrapper instead so MinGW (UCRT64 and CLANG64/CLANGARM64) is supported as well? See e.g. how RStudio did it: https://github.com/rstudio/rstudio/blob/6f7af1eab02568be3de327190a96fc5dba2dc247/src/cpp/session/CMakeLists.txt#L466-L499

There is no mt.exe manifest compiler on MingGW, but .rc is supported by windres.exe, and of course natively by MSVC.

Sorry for sounding like a broken record, but this came up already: #1752 (comment)

@wantehchang wantehchang force-pushed the utf8-windows branch 2 times, most recently from 917ed5d to c0d9a21 Compare December 25, 2023 16:13
As of Windows Version 1903 (May 2019 Update), we can use the
ActiveCodePage property in the fusion manifest for unpackaged apps to
force a process to use UTF-8 as the process code page. This method
requires no changes to the source code.

https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page#set-a-process-code-page-to-utf-8
@wantehchang wantehchang force-pushed the utf8-windows branch 4 times, most recently from 11de94f to c8689ab Compare December 25, 2023 23:42
@wantehchang
Copy link
Collaborator Author

I somehow forgot that Vincent wrote #1752 to do this. I am sorry!

Miloš: Thank you for the reminder. I didn't forget your suggestion. In the first commit of this pull request, I was aiming to make the initial solution pass the tests on all platforms. In the second commit, I tried wrapping the manifest in a resource file. It apparently works in all variants of MinGW, whether they use UCRT or MSVCRT. So the only downside of this solution is that it requires Windows 10 version 1903 instead of 1803. But I think this is a worthwhile trade-off.

@wantehchang
Copy link
Collaborator Author

I wrote this pull request originally when I looked into how to fix #1896.

@kmilos
Copy link
Contributor

kmilos commented Dec 26, 2023

Thanks Wan-Teh for working on supporting a variety of Windows toolchains.

@vrabaud
Copy link
Collaborator

vrabaud commented Dec 28, 2023

Less code, no UTF-8 hack, LGTM !

@wantehchang wantehchang marked this pull request as ready for review January 2, 2024 21:40
Copy link
Collaborator Author

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

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

Vincent: Please review. Thanks!

When I wrote this pull request, I somehow forgot that you already wrote #1752. I am really sorry.

apps/avifdec.c Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly manifestVersion="1.0" xmlns="urn:schemas-microsoft-com:asm.v1">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I copied this file from https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page#examples. The original file has this line:

  <assemblyIdentity type="win32" name="..." version="6.0.0.0"/>

I noticed that the name "..." and version "6.0.0.0" in this line look like placeholders. I think we are supposed to replace them with the name and version of avifdec, avifenc, and are_images_equal.

According to Microsoft documentation, the assemblyIdentity element is required in an application manifest:
https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests

However, I deleted this line and our tests still pass. So I suggest we try going without the assemblyIdentity element.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thx for clarifying

@wantehchang wantehchang merged commit 3ec01ce into AOMediaCodec:main Jan 8, 2024
22 checks passed
@wantehchang wantehchang deleted the utf8-windows branch January 8, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants