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 broken windows installer #26511

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jan 13, 2024

Closes #26510.

Time for a rant.

Technically, this "broke" with 9e8d07d, 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.

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, which seems much saner, no?)

Now, why did this break with 9e8d07d specifically, you might wonder?

Well the reason is this line:

Logger.Log(success ? $"Threaded optimizations set to \"{value}\"!" : "Threaded optimizations set failed!");

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, and it will also refuse to create version shortcuts.

Sounds familiar, don't it?

There are days on which I tire of computers. Today is one of them.

Closes ppy#26510.

Time for a rant.

Technically, this "broke" with 9e8d07d,
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 9e8d07d
*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="/ppy#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.
@bdach bdach added area:installer platform/windows next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! labels Jan 13, 2024
@bdach bdach requested a review from a team January 13, 2024 21:10
@peppy
Copy link
Sponsor Member

peppy commented Jan 14, 2024

We ran into similar cascading failures in stable, like a using statement in Program.cs to a dll which isn't yet downloaded causing a crash. We'll have to pay special attention to Program.cs changes going forward..

@peppy peppy merged commit 779e509 into ppy:master Jan 14, 2024
15 of 17 checks passed
@bdach bdach deleted the i-wish-i-were-a-carpenter-right-now branch January 14, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:installer next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! platform/windows size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows installer is broken
2 participants