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

Repro: .NET 7 Preview 4 breaks HarmonyX #49

Closed
wants to merge 4 commits into from
Closed

Repro: .NET 7 Preview 4 breaks HarmonyX #49

wants to merge 4 commits into from

Conversation

danielcweber
Copy link

@danielcweber danielcweber commented May 27, 2022

This PR shows that building and running on .NET SDK 7.0.100-preview.4.22252.9 breaks HarmonyX.
I found .NET 7 Preview 1 to work. I will, in subsequent commits, downgrade to preview 3, 2 and 1 to identify where the breaking change occured.

EDIT:
For Preview 1 and 2, there's 2 failing tests. Preview 1 works for us, but we're probably not using a broken feature.
For Preview 3 on, there's 56 failing tests.

EDIT:
Find the Github Actions runs here:

Preview 4
Preview 3
Preview 2
Preview 1

@ghorsington
Copy link
Contributor

ghorsington commented May 27, 2022

Greetings!

Thank you for your report! From a quick look, it seems like the CI runs tests meant for mono and older corefx runtimes only. Right now the code has some preprocessors for NET_6, for example here and some in HarmonyTests. All of them are directly taken from upstream pardeike/Harmony and haven't been altered if I recall correctly. From a quick look, I'd assume some tests might be fixed by simply adjusting the preprocessor to NET6_0_OR_GREATER or just NET.

@danielcweber
Copy link
Author

Thanks for having a look into it. I'll try to identify the directives and give it a try.

@danielcweber
Copy link
Author

It seems the line you references is the only one that mentions .NET6 but given the only core-target of the main library is netstandard, the NET6 directive doesn't do that much anyway. Also, thinking about it, it doesn't explain why there is a bigger break from preview 2 to preview 3.

Another finding: Some tests don't fail when you step into them in Visual Studio.

@ghorsington
Copy link
Contributor

Searching for all preprocessors gives some problematic places

@ rg "#if"
HarmonyTests\Traverse\TestTraverse_Types.cs
58:#if !NET6_0 // writing to static fields after init not allowed in NET6

HarmonyTests\Tools\TestAccessTools.cs
59:#if NETCOREAPP
75:#if NETCOREAPP

HarmonyTests\TestTools.cs
6:#if NETCOREAPP
190:#if NETCOREAPP
197:#if NETCOREAPP
265:#if NET35

HarmonyTests\Patching\Transpiling.cs
16:#if DEBUG

HarmonyTests\Patching\Specials.cs
486:#if !NETCOREAPP3_0 || NETCOREAPP3_1 || NET5_0

HarmonyTests\Patching\Assets\PatchClasses.cs
572:#if NETCOREAPP2_0

HarmonyTests\IL\Instructions.cs
118:#if DEBUG
270:#if NETFRAMEWORK

Harmony\Tools\Shims.net.cs
1:#if NET35 || NET45

Harmony\Tools\AccessTools.cs
1684:#if NET35
1695:#if NET35
1781:#if NET35
1832:#if NET35
1846:#if NET35
1858:#if NET35
1882:#if NET35

HarmonyTests\Extras\PatchSerialization.cs
30:#if NET50_OR_GREATER

Harmony\Internal\PatchTools.cs
40:#if NETCOREAPP2_0 || NETCOREAPP3_0 || NETCOREAPP3_1 || NETSTANDARD2_0 || NET6_0

The ones targeting .NET Core or .NET 5 all seem to be fixed (and not for example NET5_0_OR_GREATER) which seems like one of the problems. There are a few of those for tests, e.g. one in TestTraverse_Types which seems to fail based on your CI.

I'll check a bit later locally if cleaning all those up helps.

@danielcweber
Copy link
Author

Just wildly guessing here: If it's got something to do with tiered compilation, there was a reimplementation in .NET 7 Preview 3.

Complete changelog of preview 3.

@danielcweber danielcweber changed the title [WIP] Repro: .NET 7 Preview 4 breaks HarmonyX Repro: .NET 7 Preview 4 breaks HarmonyX May 27, 2022
@ghorsington
Copy link
Contributor

On a closer look, it appears that all hooking fails without any errors. As such, this is most likely the issue with MonoMod, the library that HarmonyX uses for runtime patching.

I posted an issue about this in MonoMod/MonoMod#94. I think it's best the problem is further tracked there since there isn't a direct issue with HarmonyX (sans the few tests that break in Preview 2, those can be fixed by adjusting some preprocessor tags from the looks of it).

@danielcweber
Copy link
Author

Thanks a lot @ghorsington!

@danielcweber
Copy link
Author

Closing here, see MonoMod/MonoMod#94 for further tracking.

@danielcweber danielcweber deleted the ReproDotnet7Breaks branch September 26, 2022 20:07
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.

2 participants