C#: Move GodotSharp to .NET8 #92131

paulloz · 2024-05-19T20:17:55Z

This is to prepare the EOS of .NET6 in November.

Would need to be more thoroughly tested. There are probably a bunch of other spots where we could mark things as scoped.

Supersedes C#: Move GodotSharp to net7.0 #71241

Delsin-Yu · 2024-05-21T02:49:46Z

As we are upgrading the SDK version, should the end of support for Windows 7 and 8.1 on .Net 8 be documented?

raulsntos

Thank you again for doing all this work!

This is only a partial review, I still have to review the massive scoped changes but I thought it'd be better to comment on the parts I've already reviewed than to keep you waiting until I'm done with everything.

raulsntos · 2024-05-21T02:40:43Z

modules/mono/editor/Godot.NET.Sdk/Godot.SourceGenerators.Tests/Constants.cs


 namespace Godot.SourceGenerators.Tests;

 public static class Constants
 {
    public static Assembly GodotSharpAssembly => typeof(GodotObject).Assembly;

+    // Can't find what needs updating to be able to access ReferenceAssemblies.Net.Net80, so we're making our own one.


It should be Microsoft.CodeAnalysis.Analyzer.Testing, which is a transitive dependency. But as you can see here:

https://github.com/dotnet/roslyn-sdk/blob/68ac704c81fc9832a8120799fc6842b643f5a45e/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/PublicAPI.Unshipped.txt#L276

The API hasn't shipped yet. See dotnet/roslyn-sdk#1047 (comment).

So this can stay like this for now, I guess. Do you want me to update the comment?

Version 1.1.2 of Microsoft.CodeAnalysis.Analyzer.Testing has been released which should contain Net80.

modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs

modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/VariantUtils.cs

modules/mono/editor/GodotTools/GodotTools.Core/GodotTools.Core.csproj

raulsntos · 2024-06-23T19:15:32Z

.../mono/glue/GodotSharp/Godot.SourceGenerators.Internal/Godot.SourceGenerators.Internal.csproj

  <ItemGroup>
-    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="3.10.0" PrivateAssets="all" />
-    <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.3" PrivateAssets="all" />
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.9.2" PrivateAssets="all" />


This bumps the minimum supported Visual Studio version to Visual Studio 2022 version 17.9. I think it's fine since .NET 8.0 is only officially supported in Visual Studio 17.8+ anyway, but I wanted to mention it for the record.

I'll double check this is necessary, it might come from me struggling to find the reference assembly and updating packages like a maniac x)

modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs

modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/VariantUtils.generic.cs

raulsntos · 2024-06-23T19:52:33Z

modules/mono/glue/GodotSharp/Godot.SourceGenerators.Internal/UnmanagedCallbacksGenerator.cs

+            (RefKind.Out, _) => source.Append("out"),
+            (RefKind.In, ScopedKind.ScopedRef) => source.Append("scoped in"),
+            (RefKind.In, _) => source.Append("in"),
+            (RefKind.Ref, ScopedKind.ScopedRef) => source.Append("scoped ref"),
+            (RefKind.Ref, _) => source.Append("ref"),


Instead of basically duplicating every entry in the switch, why not append the scoped keyword before the switch?

Also, this only handles ScopedRef but not ScopedValue. Although I guess there are no such cases in our partial method declarations.

I kinda like this syntax because it visually lays out all the possibilities. But I can rewrite this if it feels too weird.

raulsntos · 2024-06-23T20:01:35Z

modules/mono/glue/GodotSharp/Godot.SourceGenerators.Internal/UnmanagedCallbacksGenerator.cs

@@ -172,7 +172,7 @@ void AppendPartialContainingTypeDeclarations(INamedTypeSymbol? containingType)
            {
                var parameter = callback.Parameters[i];

-                AppendRefKind(source, parameter.RefKind);
+                AppendRefKind(source, parameter.RefKind, parameter.ScopedKind);


Not sure if using the scoped keyword makes sense for these interop methods. Isn't it a lie? Once we cross to the C++ side, there's nothing stopping these values from escaping the scope and C# would not be aware at all.

Because of ref safety changes in the languages, all methods that return an interop struct have to have all other reference parameters marked as scoped to signal the the method does not capture that reference. The variant change is necessary, because for some reason a type of the exact shape godot_variant is in, crashes the .NET 7 JIT, but when changing it to be sequential with the same effective layout it works.

paulloz · 2024-12-12T18:20:54Z

Rebased and up to date. There are a few new places I'm not sure whether we should use scoped or not (e.g. new NativeFuncs methods introduced in #98545). But otherwise it should be good to test.

raulsntos

I've taken another look at the changes and tested them again. Everything looks great and seems to be working as expected.

I think it's good to merge now, and we can get some extra testing before the stable release. We can always improve upon this with subsequent PRs.

Thank you so much for your hard work and patience in seeing this through.

paulloz · 2024-12-13T12:33:02Z

I forgot to squash commits. Is it fine to keep two separate commits here? I don't want to erase the large contribution of the previous PR.

akien-mga · 2024-12-13T12:39:35Z

Yeah keeping two separate commits seems fine to me.

Note that there are a number of C# warnings raised by the CI in a non-obvious way: they're visible if you view the last commit and scroll all the way down:

1c7ecfb

I don't see those on other PRs so they must be introduced by this change, and should ideally be fixed now otherwise all future PRs (even non C# related) will have these warnings.

- Change TFM and LangVersion - Better exception throwing (CA1510, CA1512, CA1513) - Better exception utility method definition (CA1859) - Prefer comparing `.Count` over calling `.Any()` (CA1860) - Prefer `.AsSpan()` over `.Substring()` (CA1846) - Add a few more `scoped` - Use `RuntimeHelpers.GetUninitializedObject()` instead of `FormatterServices.GetUninitializedObject()` - Use delegate instead of delegate pointer in variant generic conversions - Enable EnforceExtendedAnalyzerRules in source generator projects - Disable CS8981 on structs named movable in Godot.NativeInterop

paulloz · 2024-12-13T13:02:55Z

Good catch, thank you. Those are warning introduced in waves 6 and up.

akien-mga · 2024-12-13T14:32:21Z

Great work @paulloz and @RedworkDE!

paulloz added the topic:dotnet label May 19, 2024

paulloz added this to the 4.4 milestone May 19, 2024

paulloz force-pushed the net8 branch from 2d661b6 to 25defc4 Compare May 19, 2024 21:17

raulsntos mentioned this pull request May 21, 2024

C#: Enforce C# 10 in .csproj files #86375

Closed

raulsntos reviewed Jun 24, 2024

View reviewed changes

paulloz force-pushed the net8 branch from 25defc4 to 128e473 Compare June 24, 2024 05:18

raulsntos mentioned this pull request Nov 7, 2024

C#: Upgrade the platform-specific TargetFramework version automatically #82927

Closed

raulsntos mentioned this pull request Nov 16, 2024

Add progress report article about .NET 8 godotengine/godot-website#956

Merged

paulloz force-pushed the net8 branch from 5761934 to 1c7ecfb Compare December 12, 2024 17:44

paulloz marked this pull request as ready for review December 12, 2024 18:15

paulloz requested review from a team as code owners December 12, 2024 18:15

raulsntos approved these changes Dec 13, 2024

View reviewed changes

akien-mga mentioned this pull request Dec 13, 2024

C#: Move GodotSharp to net7.0 #71241

Closed

paulloz force-pushed the net8 branch from 1c7ecfb to fb8553e Compare December 13, 2024 13:01

akien-mga added the enhancement label Dec 13, 2024

akien-mga merged commit 7f5c469 into godotengine:master Dec 13, 2024
20 checks passed

paulloz deleted the net8 branch December 14, 2024 15:15

akien-mga mentioned this pull request Dec 17, 2024

.NET: Building GodotSharp is broken in current master branch #100502

Closed

This was referenced Dec 19, 2024

C#: Update platform support for 4.4 and new minimum required TFM godotengine/godot-docs#10427

Merged

[.NET] Update required .NET SDK version in error messages #100667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: Move GodotSharp to .NET8 #92131

C#: Move GodotSharp to .NET8 #92131

paulloz commented May 19, 2024

Delsin-Yu commented May 21, 2024

raulsntos left a comment

raulsntos May 21, 2024

paulloz Jun 26, 2024

raulsntos Aug 19, 2024

raulsntos Jun 23, 2024

paulloz Jun 24, 2024

raulsntos Jun 23, 2024

paulloz Jun 24, 2024

raulsntos Jun 23, 2024

paulloz commented Dec 12, 2024

raulsntos left a comment

paulloz commented Dec 13, 2024

akien-mga commented Dec 13, 2024 •

edited

Loading

paulloz commented Dec 13, 2024

akien-mga commented Dec 13, 2024

C#: Move GodotSharp to .NET8 #92131

C#: Move GodotSharp to .NET8 #92131

Conversation

paulloz commented May 19, 2024

Delsin-Yu commented May 21, 2024

raulsntos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulloz commented Dec 12, 2024

raulsntos left a comment

Choose a reason for hiding this comment

paulloz commented Dec 13, 2024

akien-mga commented Dec 13, 2024 • edited Loading

paulloz commented Dec 13, 2024

akien-mga commented Dec 13, 2024

akien-mga commented Dec 13, 2024 •

edited

Loading