-
Notifications
You must be signed in to change notification settings - Fork 174
Support .NET 8.0 #740
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
Support .NET 8.0 #740
Conversation
…o it better in the future)
[Fact] | ||
public void ShouldCompileAndExecuteWithWebSdk() | ||
{ | ||
var processResult = ScriptTestRunner.Default.ExecuteFixture("WebApi", "--no-cache"); | ||
Assert.Equal(0, processResult.ExitCode); | ||
} | ||
#endif | ||
// todo: the same test should run for .NET 8.0 - currently it fails with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. When I tested this I got a MissingMethodException.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, this is a load context problem. Try adding this test
#if NET8_0
[Fact]
public void ShouldCompileAndExecuteWithWebSdk()
{
var processResult = ScriptTestRunner.Default.ExecuteFixture("WebApi", "--no-cache --isolated-load-context");
Assert.Equal(0, processResult.ExitCode);
}
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we should revisit the idea of making --isolated-load-context
the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right! thank you!
I agree with the default, I think this occassion is as good as any, so maybe let's merge this and then in a separate PR we can:
- rev up the version to 2.0
- make isolated load context default
- remove the flag and add the opposite flag instead (to disable it, just like we can disable cache)
would this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would make perfect sense. I think the cases where isolated load context does not work are so rare that we will have less issues having that as a default. 👍
💥 |
Added multi targeting for .NET 8.0.
There is still one thing that is broken, and that is the test
ShouldCompileAndExecuteWithWebSdk
which means the SDK support on .NET 8.0. I think we can merge .NET 8.0 support as-is to be ready and then address that in a separate PR.