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

tez011 · 2024-03-29T17:06:28Z

Description

#2149 was reverted in #2320 and needs to be split into three changes:

C++20
UCRT64 (this one!)
WGC

If msys2/MINGW-packages#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) 🤪
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
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.

I want maintainers to keep my branch updated

psyke83 · 2024-03-29T21:08:13Z

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 · 2024-03-29T21:26:06Z

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: cadd3da)

psyke83 · 2024-03-29T22:03:30Z

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 · 2024-03-29T23:37:09Z

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: msys2/MINGW-packages#20477 (comment)

psyke83 · 2024-03-30T01:30:03Z

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 · 2024-03-30T17:38:45Z

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 · 2024-03-31T17:21:05Z

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 · 2024-03-31T18:18:09Z

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.

src/main.cpp

FrogTheFrog · 2024-04-01T08:03:02Z

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

ReenigneArcher · 2024-04-01T12:52:41Z

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

ReenigneArcher · 2024-04-03T21:15:43Z

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

tez011 · 2024-04-04T06:21:55Z

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 · 2024-04-04T16:57:32Z

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?

cgutman · 2024-04-06T15:44:39Z

src_assets/windows/misc/migration/migrate-config.bat

 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)


Is this still needed with the change to config.cpp?

I think so?

Test 1:

Delete existing config directory

Freshly install Sunshine built from current PR branch

Set port = 46989

Stop sunshine service and relaunch from shortcut as non-admin

Result: Sunshine launches with the correct custom web UI port.

Test 2:

Repeat test 1 with build that omits migrate-config.bat OR keep PR build+config from test 1 and remove Users read folder privilege manually.

Result: several second delay on "Waiting for Web UI to be ready....", and UI opens to the (incorrect) default port.

tez011 · 2024-04-26T20:06:33Z

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 msys2/MINGW-packages#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 · 2024-04-26T20:15:30Z

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 · 2024-05-14T00:33:25Z

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 · 2024-05-14T00:35:43Z

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

codecov · 2024-05-25T15:33:52Z

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 ⚠️

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%)`	⬇️

... and 27 files with indirect coverage changes

ReenigneArcher · 2024-05-25T16:25:42Z

PR updating ffmpeg to UCRT64: #2572

Despite requesting UTF-8, MinGW appears to have been using the 'C' locale the whole time, so we should enforce that here even if UCRT's setlocale() actually works as advertised.

1) On UCRT64, std::filesystem::exists() interprets the credentials folder to not exist if the Users groups do not have folder read permissions, which results in a subsequent failure to create the already existing folder. 2) Similarly, non-admin shortcut launches may also throw an exception in boost::filesystem::create_directories() if the config folder has been deleted by the user after installation. Steps to reproduce: install Sunshine, quit Sunshine, delete config and then run launcher as non-admin. Fix 1) by setting User read access to the folder - but not its contents - so std::filesystem::exists() gives the expected result. Fix 2) by allowing non-privileged process to start the service even if no config was loaded, thus deferring config creation to service instance.

tez011 · 2024-05-25T21:39:22Z

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!

Co-authored-by: Conn O'Griofa <connogriofa@gmail.com>

ReenigneArcher marked this pull request as draft March 29, 2024 17:58

tez011 mentioned this pull request Mar 30, 2024

Build ffmpeg for windows using Universal CRT runtime. LizardByte/build-deps#306

Merged

11 tasks

ReenigneArcher added the pr:under condideration label Mar 30, 2024

ReenigneArcher reviewed Mar 31, 2024

View reviewed changes

src/main.cpp Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

cgutman reviewed Apr 6, 2024

View reviewed changes

tez011 marked this pull request as ready for review April 26, 2024 20:06

tez011 changed the title ~~Use UCRT64 environment instead of MinGW64.~~ build(win): use UCRT64 environment instead of MinGW64. Apr 26, 2024

tez011 added 2 commits May 25, 2024 14:28

Use UCRT64 environment instead of MinGW64.

13318e7

Use default minimal ANSI locale.

6f97ddc

Despite requesting UTF-8, MinGW appears to have been using the 'C' locale the whole time, so we should enforce that here even if UCRT's setlocale() actually works as advertised.

ReenigneArcher merged commit fed482c into LizardByte:master May 25, 2024
48 of 49 checks passed

tez011 mentioned this pull request May 26, 2024

feat(win): new capture method, Windows.Graphics.Capture #2580

Merged

11 tasks

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Jun 6, 2024

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

10ebd97

Co-authored-by: Conn O'Griofa <connogriofa@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tez011 commented Mar 29, 2024 •

edited

Loading

psyke83 commented Mar 29, 2024 •

edited

Loading

psyke83 commented Mar 29, 2024 •

edited

Loading

psyke83 commented Mar 29, 2024 •

edited

Loading

psyke83 commented Mar 29, 2024

psyke83 commented Mar 30, 2024

psyke83 commented Mar 30, 2024 •

edited by ReenigneArcher

Loading

FrogTheFrog commented Mar 31, 2024 •

edited

Loading

tez011 commented Mar 31, 2024 •

edited

Loading

FrogTheFrog commented Apr 1, 2024 •

edited

Loading

ReenigneArcher commented Apr 1, 2024

This comment was marked as resolved.

This comment was marked as resolved.

ReenigneArcher commented Apr 3, 2024

tez011 commented Apr 4, 2024 •

edited

Loading

psyke83 commented Apr 4, 2024

cgutman Apr 6, 2024

psyke83 Apr 6, 2024

tez011 commented Apr 26, 2024

ReenigneArcher commented Apr 26, 2024

tez011 commented May 14, 2024

ReenigneArcher commented May 14, 2024

codecov bot commented May 25, 2024 •

edited

Loading

ReenigneArcher commented May 25, 2024

tez011 commented May 25, 2024

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

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

Conversation

tez011 commented Mar 29, 2024 • edited Loading

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Branch Updates

psyke83 commented Mar 29, 2024 • edited Loading

psyke83 commented Mar 29, 2024 • edited Loading

psyke83 commented Mar 29, 2024 • edited Loading

psyke83 commented Mar 29, 2024

psyke83 commented Mar 30, 2024

psyke83 commented Mar 30, 2024 • edited by ReenigneArcher Loading

FrogTheFrog commented Mar 31, 2024 • edited Loading

tez011 commented Mar 31, 2024 • edited Loading

FrogTheFrog commented Apr 1, 2024 • edited Loading

ReenigneArcher commented Apr 1, 2024

This comment was marked as resolved.

This comment was marked as resolved.

ReenigneArcher commented Apr 3, 2024

tez011 commented Apr 4, 2024 • edited Loading

psyke83 commented Apr 4, 2024

cgutman Apr 6, 2024

Choose a reason for hiding this comment

psyke83 Apr 6, 2024

Choose a reason for hiding this comment

tez011 commented Apr 26, 2024

ReenigneArcher commented Apr 26, 2024

tez011 commented May 14, 2024

ReenigneArcher commented May 14, 2024

codecov bot commented May 25, 2024 • edited Loading

Codecov Report

ReenigneArcher commented May 25, 2024

tez011 commented May 25, 2024

tez011 commented Mar 29, 2024 •

edited

Loading

psyke83 commented Mar 29, 2024 •

edited

Loading

psyke83 commented Mar 29, 2024 •

edited

Loading

psyke83 commented Mar 29, 2024 •

edited

Loading

psyke83 commented Mar 30, 2024 •

edited by ReenigneArcher

Loading

FrogTheFrog commented Mar 31, 2024 •

edited

Loading

tez011 commented Mar 31, 2024 •

edited

Loading

FrogTheFrog commented Apr 1, 2024 •

edited

Loading

tez011 commented Apr 4, 2024 •

edited

Loading

codecov bot commented May 25, 2024 •

edited

Loading