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

Support .NET 8.0 #740

Merged
merged 7 commits into from
Nov 9, 2023
Merged

Support .NET 8.0 #740

merged 7 commits into from
Nov 9, 2023

Conversation

filipw
Copy link
Member

@filipw filipw commented Nov 6, 2023

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.

[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
Copy link
Collaborator

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.

Copy link
Collaborator

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

Copy link
Collaborator

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?

Copy link
Member Author

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?

Copy link
Collaborator

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. 👍

@seesharper seesharper merged commit 3d6fa49 into master Nov 9, 2023
6 checks passed
@seesharper seesharper deleted the feature/net8 branch November 9, 2023 12:43
@filipw
Copy link
Member Author

filipw commented Nov 9, 2023

💥

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.

None yet

2 participants