From 1e3c332658a320fb2dbf49e4a97e2927cafc4dcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 13 Jan 2024 21:39:32 +0100 Subject: [PATCH] Fix broken installer Closes https://github.com/ppy/osu/issues/26510. Time for a rant. Technically, this "broke" with 9e8d07d3144bd4b072d28bd9bd0e255fee410de0, but it is actually an end result of upstream behaviours that I am failing to find a better description for than "utterly broken". Squirrel (the installer we use) has unit tests. Which is great, power to them. However, the method in which that testing is implemented leads to epic levels of WTF breakage. To determine whether Squirrel is being tested right now, it is checking all currently loaded assemblies, and determining that if any loaded assembly contains the magic string of "NUNIT" - among others - it must be being tested right now: https://github.com/clowd/Clowd.Squirrel/blob/24427217482deeeb9f2cacac555525edfc7bd9ac/src/Squirrel/SimpleSplat/PlatformModeDetector.cs#L17-L32 If one assumes that there is no conceivable way that an NUnit assembly *may* be loaded *without* it being a test context, this *may* seem sane. Foreshadowing. (Now, to avoid being hypocritical, we also do this, *but* we do this by checking if the *entry* assembly is an NUnit: https://github.com/ppy/osu-framework/blob/92db55a52742047b42c0b4b25327ce28bd991b44/osu.Framework/Development/DebugUtils.cs#L16-L34 which seems *much* saner, no?) Now, why did this break with 9e8d07d3144bd4b072d28bd9bd0e255fee410de0 *specifically*, you might wonder? Well the reason is this line: https://github.com/ppy/osu/blob/3d3f58c2523f519504d9156a684538e2aa0c094c/osu.Desktop/NVAPI.cs#L183 Yes you are reading this correctly, it's not NVAPI anything itself that breaks this, it is *a log statement*. To be precise, what the log statement *does* to provoke this, is calling into framework. That causes the framework assembly to load, *which* transitively loads the `nunit.framework` assembly. (If you ever find yourself wanting to find out this sort of cursed knowledge - I hope you never need to - you can run something along the lines of dotnet-trace collect --providers Microsoft-Windows-DotNETRuntime:4 -- .\osu!.exe then open the resulting trace in PerfView, and then search the `Microsoft-Windows-DotNETRuntime/AssemblyLoader/Start` log for the cursed assembly. In this case, the relevant entry said something along the lines of HasStack="True" ThreadID="23,924" ProcessorNumber="0" ClrInstanceID="6" AssemblyName="nunit.framework, Version=3.13.3.0, Culture=neutral, PublicKeyToken=2638cd05610744eb" AssemblyPath="" RequestingAssembly="osu.Framework, Version=2024.113.0.0, Culture=neutral, PublicKeyToken=null" AssemblyLoadContext="Default" RequestingAssemblyLoadContext="Default" ActivityID="/#21032/1/26/" Either that or just comment the log line for kicks. But the above is *much* faster.) Now, what *happens* if Squirrel "detects" that it is being "tested"? Well, it will refuse to close after executing the "hooks" defined via `SquirrelAwareApp`: https://github.com/clowd/Clowd.Squirrel/blob/24427217482deeeb9f2cacac555525edfc7bd9ac/src/Squirrel/SquirrelAwareApp.cs#L85-L88 and it will also refuse to create version shortcuts: https://github.com/clowd/Clowd.Squirrel/blob/24427217482deeeb9f2cacac555525edfc7bd9ac/src/Squirrel/UpdateManager.Shortcuts.cs#L63-L65 Sounds familiar, don't it? There are days on which I tire of computers. Today is one of them. --- osu.Desktop/Program.cs | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/osu.Desktop/Program.cs b/osu.Desktop/Program.cs index a652e31f62..6b95a82703 100644 --- a/osu.Desktop/Program.cs +++ b/osu.Desktop/Program.cs @@ -30,12 +30,19 @@ namespace osu.Desktop [STAThread] public static void Main(string[] args) { - // NVIDIA profiles are based on the executable name of a process. - // Lazer and stable share the same executable name. - // Stable sets this setting to "Off", which may not be what we want, so let's force it back to the default "Auto" on startup. - NVAPI.ThreadedOptimisations = NvThreadControlSetting.OGL_THREAD_CONTROL_DEFAULT; - - // run Squirrel first, as the app may exit after these run + /* + * WARNING: DO NOT PLACE **ANY** CODE ABOVE THE FOLLOWING BLOCK! + * + * Logic handling Squirrel MUST run before EVERYTHING if you do not want to break it. + * To be more precise: Squirrel is internally using a rather... crude method to determine whether it is running under NUnit, + * namely by checking loaded assemblies: + * https://github.com/clowd/Clowd.Squirrel/blob/24427217482deeeb9f2cacac555525edfc7bd9ac/src/Squirrel/SimpleSplat/PlatformModeDetector.cs#L17-L32 + * + * If it finds ANY assembly from the ones listed above - REGARDLESS of the reason why it is loaded - + * the app will then do completely broken things like: + * - not creating system shortcuts (as the logic is if'd out if "running tests") + * - not exiting after the install / first-update / uninstall hooks are ran (as the `Environment.Exit()` calls are if'd out if "running tests") + */ if (OperatingSystem.IsWindows()) { var windowsVersion = Environment.OSVersion.Version; @@ -59,6 +66,11 @@ namespace osu.Desktop setupSquirrel(); } + // NVIDIA profiles are based on the executable name of a process. + // Lazer and stable share the same executable name. + // Stable sets this setting to "Off", which may not be what we want, so let's force it back to the default "Auto" on startup. + NVAPI.ThreadedOptimisations = NvThreadControlSetting.OGL_THREAD_CONTROL_DEFAULT; + // Back up the cwd before DesktopGameHost changes it string cwd = Environment.CurrentDirectory;