build(win): use UCRT64 environment instead of MinGW64.

[tez011]

Description

https://github.com/LizardByte/Sunshine/pull/2149 was reverted in https://github.com/LizardByte/Sunshine/pull/2320 and needs to be split into three changes:

• C++20
• UCRT64 (this one!)
• WGC

If https://github.com/msys2/MINGW-packages/pull/20477 is integrated, WGC can proceed without this change.

Screenshot

Issues Fixed or Closed

Type of Change

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) 🤪
• [x] Dependency update (updates to dependencies)
• [ ] Documentation update (changes to documentation)
• [x] Repository update (changes to repository files, e.g. .github/...)

Checklist

• [x] My code follows the style guidelines of this project
• [x] I have performed a self-review of my own code
• [x] I have commented my code, particularly in hard-to-understand areas
• [x] I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch must be updated before it can be merged. You must also Allow edits from maintainers.

• [x] I want maintainers to keep my branch updated

[psyke83]

This resolves the regex CPU/memory hog issue:

diff --git a/src/main.cpp b/src/main.cpp
index a3901448..7672bac6 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -102,8 +102,8 @@ main(int argc, char *argv[]) {
   task_pool_util::TaskPool::task_id_t force_shutdown = nullptr;

 #ifdef _WIN32
-  // Switch default C standard library locale to UTF-8 on Windows 10 1803+
-  setlocale(LC_ALL, ".UTF-8");
+  // Switch default C standard library locale to UTF-16 on Windows 10 1803+
+  setlocale(LC_ALL, ".UTF-16");
 #endif

 #pragma GCC diagnostic push

Setting to UTF16 or UTF32 resolves the slowdown, but I'm not sure if that actually breaks our current regex parsing?

[psyke83]

Relevant discussion (see also in comments): https://stackoverflow.com/questions/59067280/does-stdwregex-support-utf-16-unicode-or-only-ucs-2

Setting to UTF16 doesn't break my en-IE keyboard mappings, but it could have consequences for other languages and may also break log parsing (see: https://github.com/LizardByte/Sunshine/commit/cadd3da9a73ce8ca1cbca9531025e31f4b9a9ac6)

[psyke83]

I retested the #1477 by printing some Chinese characters with .UTF-16 set, but didn't change the UTF-8 conversion.

The result: Sunshine doesn't crash, regex doesn't consume CPU or memory, and the Chinese characters print in the web UI log correctly. This may be a viable fix, but @cgutman will probably want to review that change to make sure no other changes are required.

[psyke83]

Apologies for the spam, but my observations were flawed. It appears that MINGW64 builds were not setting the UTF-8 codepage correctly, so the default C codepage was in use. UCRT64 builds do set a UTF-8 codepage, but cause the std::regex issue. Leaving the codepage to C (not UTF-16, as that was falling back to C on UCRT64 as well) seems to be the correct solution.

More context: https://github.com/msys2/MINGW-packages/pull/20477#issuecomment-2027821426

[psyke83]

Two points on MSVCRT in the MSYS2 environments documentation:

• It doesn't support the UTF-8 locale

This explains the behaviour we're seeing. I didn't mention in my prior comment, but setting the system default locale via std::setlocale(LC_ALL, ""); does apply correctly on both MINGW64 and UCRT64. For my system, it sets English_Ireland.1252 for both runtimes, and regex doesn't exhibit any issues with either runtime.

• Binaries linked with MSVCRT should not be mixed with UCRT ones because the internal structures and data types are different. (More strictly, object files or static libraries built for different targets shouldn't be mixed. DLLs built for different CRTs can be mixed as long as they don't share CRT objects, e.g. FILE*, across DLL boundaries.) Same rule is applied for MSVC compiled binaries because MSVC uses UCRT by default (if not changed).

This suggests to me that we need a complementary PR for build-deps to also switch to UCRT64, so that the statically linked ffmpeg libraries don't cause unforseen issues.

[psyke83]

Regarding the UAC failing to trigger via shortcut launch: std::filesystem::exists returns false due to the restrictive permissions of the credentials folder. This may be a bug in UCRT64 or perhaps it is the more correct behaviour; I'm not sure.

A workaround (or perhaps the only solution) is the following:

diff --git a/src_assets/windows/misc/migration/migrate-config.bat b/src_assets/windows/misc/migration/migrate-config.bat
index 65d00d8f..aa8d10aa 100644
--- a/src_assets/windows/misc/migration/migrate-config.bat
+++ b/src_assets/windows/misc/migration/migrate-config.bat
@@ -39,9 +39,10 @@ rem Create the credentials directory if it wasn't migrated or already existing
 if not exist "%NEW_DIR%\credentials\" mkdir "%NEW_DIR%\credentials"

 rem Disallow read access to the credentials directory for normal users
-rem Note: We must use the SID directly because "Administrators" is localized
+rem Note: We must use the SIDs directly because "Users" and "Administrators" are localized
 icacls "%NEW_DIR%\credentials" /inheritance:r
 icacls "%NEW_DIR%\credentials" /grant:r *S-1-5-32-544:(OI)(CI)(F)
+icacls "%NEW_DIR%\credentials" /grant:r *S-1-5-32-545:(R)

 rem Migrate the covers directory
 if exist "%OLD_DIR%\covers\" (

This will grant the Users aka (S-1-5-32-545) group read access for the folder (but not its contents). Technically it may reduce security because users can now list folder contents, but they still can't read or make any changes to the files contained within.

Edit: amended to use the SID to avoid localization issues.

[FrogTheFrog]

Whatever you end up doing, do not use std::setlocale(LC_ALL, "");.

On Windows you can actually set global locale to be UTF-8 and calling std::setlocale(LC_ALL, ""); will then set it to UTF-8 instead of the default C, and then we're back at the square 1.

[tez011]

oof, I'm embarrassed by that. Should have read setlocale docs before blindly setting it to what I thought was the 'default' locale. Thanks.

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/setlocale-wsetlocale?view=msvc-170

At program startup, the equivalent of the following statement is executed: setlocale( LC_ALL, "C" );

I guess it's possible we don't need to do this at all, but I think it might be a good idea to leave this in there anyway, so there isn't any appetite to add back UTF-8 in the future.

[FrogTheFrog]

https://github.com/msys2/MINGW-packages/pull/20477 has been merged. Maybe we don't need to migrate to ucrt anymore (but still remove the UTF-8 locale)?

[ReenigneArcher]

It seems that mingw is becoming legacy, so probably worth migrating anyway. The WGC just won't depend on this now.

[ReenigneArcher]

Regarding the UAC failing to trigger via shortcut launch: std::filesystem::exists returns false due to the restrictive permissions of the credentials folder. This may be a bug in UCRT64 or perhaps it is the more correct behaviour; I'm not sure.

A workaround (or perhaps the only solution) is the following:

diff --git a/src_assets/windows/misc/migration/migrate-config.bat b/src_assets/windows/misc/migration/migrate-config.bat
index 65d00d8f..aa8d10aa 100644
--- a/src_assets/windows/misc/migration/migrate-config.bat
+++ b/src_assets/windows/misc/migration/migrate-config.bat
@@ -39,9 +39,10 @@ rem Create the credentials directory if it wasn't migrated or already existing
 if not exist "%NEW_DIR%\credentials\" mkdir "%NEW_DIR%\credentials"

 rem Disallow read access to the credentials directory for normal users
-rem Note: We must use the SID directly because "Administrators" is localized
+rem Note: We must use the SIDs directly because "Users" and "Administrators" are localized
 icacls "%NEW_DIR%\credentials" /inheritance:r
 icacls "%NEW_DIR%\credentials" /grant:r *S-1-5-32-544:(OI)(CI)(F)
+icacls "%NEW_DIR%\credentials" /grant:r *S-1-5-32-545:(R)

 rem Migrate the covers directory
 if exist "%OLD_DIR%\covers\" (

This will grant the Users aka (S-1-5-32-545) group read access for the folder (but not its contents). Technically it may reduce security because users can now list folder contents, but they still can't read or make any changes to the files contained within.

Edit: amended to use the SID to avoid localization issues.

@tez011 cgutman mentioned he was fine with this approach on our Discord server, if you'd like to implement the change.

[psyke83]

Regarding the UAC failing to trigger via shortcut launch: std::filesystem::exists returns false due to the restrictive permissions of the credentials folder. This may be a bug in UCRT64 or perhaps it is the more correct behaviour; I'm not sure. A workaround (or perhaps the only solution) is the following:

diff --git a/src_assets/windows/misc/migration/migrate-config.bat b/src_assets/windows/misc/migration/migrate-config.bat
index 65d00d8f..aa8d10aa 100644
--- a/src_assets/windows/misc/migration/migrate-config.bat
+++ b/src_assets/windows/misc/migration/migrate-config.bat
@@ -39,9 +39,10 @@ rem Create the credentials directory if it wasn't migrated or already existing
 if not exist "%NEW_DIR%\credentials\" mkdir "%NEW_DIR%\credentials"

 rem Disallow read access to the credentials directory for normal users
-rem Note: We must use the SID directly because "Administrators" is localized
+rem Note: We must use the SIDs directly because "Users" and "Administrators" are localized
 icacls "%NEW_DIR%\credentials" /inheritance:r
 icacls "%NEW_DIR%\credentials" /grant:r *S-1-5-32-544:(OI)(CI)(F)
+icacls "%NEW_DIR%\credentials" /grant:r *S-1-5-32-545:(R)

 rem Migrate the covers directory
 if exist "%OLD_DIR%\covers\" (

This will grant the Users aka (S-1-5-32-545) group read access for the folder (but not its contents). Technically it may reduce security because users can now list folder contents, but they still can't read or make any changes to the files contained within. Edit: amended to use the SID to avoid localization issues.

@tez011 cgutman mentioned he was fine with this approach on our Discord server, if you'd like to implement the change.

I'd prefer not to use the patch quoted in my earlier comment; I submitted an updated version via PR: https://github.com/tez011/Sunshine/pull/1

If @tez011 doesn't merge my PR into his branch, he can just cherry-pick the commit, but I'd rather the newer version be used than what you quoted here.

[ReenigneArcher]

@tez011 did you see @psyke83 's PR into your branch? https://github.com/tez011/Sunshine/pull/1

[tez011]

Thanks for the ping and for patience. I didn't get any email notifications about that pull request until yours... though I did see it yesterday and thought it was closed.

I guess if the service launches sunshine as administrator, it'll have access to the configuration files specified and the patch will work good. I've merged that change. Thanks for the patch @psyke83 !

[psyke83]

Thanks for the ping and for patience. I didn't get any email notifications about that pull request until yours... though I did see it yesterday and thought it was closed.

I guess if the service launches sunshine as administrator, it'll have access to the configuration files specified and the patch will work good. I've merged that change. Thanks for the patch @psyke83 !

No problem, but it seems that my commit was lost after you rebased your ucrt64 branch against nightly?

[tez011]

Now that C++20 is in, this can be considered.

What's the appetite for merging this environment change? If it's slim-to-none, I can try porting the WGC change to mingw64 now that https://github.com/msys2/MINGW-packages/pull/20477 is in. I have a suspicion that MSVCRT and the UWP runtime won't play together well, so hoping to get some confirmation here before I spend time in that rabbit hole.

[ReenigneArcher]

What's the appetite for merging this environment change?

We'd like to move to UCRT64 since mingw seems to be considered legacy by the packaging team.

[tez011]

Could we move forward with more detailed testing/review of this pull request soon? Assuming, of course, than 0.23.2 isn't coming up shortly.

[ReenigneArcher]

Yes, I am trying to knock out as many of these open PRs as possible. Sorry for the delay.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 6.96%. Comparing base (3c341ea) to head (d15bc84). Report is 156 commits behind head on master.

Files	Patch %	Lines
src/config.cpp	0.00%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #2323      +/-   ##
=========================================
- Coverage    7.03%   6.96%   -0.08%     
=========================================
  Files          87      87              
  Lines       17697   17692       -5     
  Branches     8406    8407       +1     
=========================================
- Hits         1245    1232      -13     
- Misses      13716   15773    +2057     
+ Partials     2736     687    -2049     

Flag	Coverage Δ
Linux	5.35% <0.00%> (ø)
Windows	2.56% <0.00%> (ø)
macOS-12	?
macOS-13	?
macOS-14	8.27% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/config.cpp	4.19% <0.00%> (-0.65%)	:arrow_down:

... and 27 files with indirect coverage changes

[ReenigneArcher]

PR updating ffmpeg to UCRT64: https://github.com/LizardByte/Sunshine/pull/2572

[tez011]

Thanks for merging this!

I'll let this change stew for a bit while I ready the final WGC pull request. I hope to send that out soon!