3.2 New csproj style with backport of Godot.NET.Sdk

This is a cherry-pick of
ced77b1e9b
with several 3.2 specific alterations.

There are a lot of build issues coming from
old style projects. At this point fixing every
single one of those would require adding patch
after patch to the project file, which is a
considerable amount work and makes the csproj
even more bloated than it already is.

As such I decided this effort would be better
spent back-porting the Sdk style support that's
already available in 4.0-dev to the 3.2 branch.

This will prevent many issues, but it will also
introduce other benefits, among them:

- While target framework stays as .NET Framework
  v4.7.2, it can be changed to .NET Standard 2.0
  or greater if desired.
- It makes it much easier to add future patches.
  They are added to Godot.NET.Sdk and the only
  change required in Godot code is to update the
  Sdk version to use.
- Default Godot define constants are also
  backported, which fixes IDE issues with the
  preprocessor.

There are a few differences in the changes
applied during patching of the csproj compared
to 4.0 with the purpose of preventing breaking
builds:

- 'TargetFramework' stays net472 both for new
  projects and when importing old ones. It can
  be manually changed to netstandard 2.0+ if
  desired though.

The following features are enabled by default for
new projects. Enabling them in imported projects
may result in errors that must be fixed manually:

- 'EnableDefaultCompileItems' is disabled as it
  can result in undesired C# source files being
  included. Existing include items are kept.
  As long as 'EnableDefaultCompileItems' remains
  disabled, Godot will continue taking care of
  adding and removing C# files to the csproj.
- 'GenerateAssemblyInfo' is disabled as it
  guarantees a build error because of conflicts
  between the existing 'AssemblyInfo.cs' and the
  auto-generated one.
- 'Deterministic' is disabled because it doesn't
  like wildcards in the assembly version (1.0.*)
  that was in the old 'AssemblyInfo.cs'.

Of importance:

This is a breaking change. A great effort was
put in avoiding build errors after upgrading a
project, but there may still be exceptions.

This also breaks forward compatibility. Projects
opened with Godot 3.2.3 won't work out of the box
with older Godot versions. This was already the
case with changes introduced in 3.2.2.

Albeit C# support in 3.2.x was still labeled as
alpha, we've been trying to treat it as stable
for some time. Still the amount of problems this
change solves justifies it, but no more changes
that break project compatibility are to be
introduced from now on (at least for 3.x).

modules/mono/editor/GodotTools/GodotTools.ProjectEditor/ProjectUtils.cs

@@ -1,9 +1,12 @@
+using System;
 using GodotTools.Core;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
 using System.Linq;
-using System.Reflection;
+using System.Xml;
+using System.Xml.Linq;
+using JetBrains.Annotations;
 using Microsoft.Build.Construction;
 using Microsoft.Build.Globbing;
 
@@ -11,7 +14,7 @@ namespace GodotTools.ProjectEditor
 {
     public sealed class MSBuildProject
     {
-        public ProjectRootElement Root { get; }
+        internal ProjectRootElement Root { get; set; }
 
         public bool HasUnsavedChanges { get; set; }
 
@@ -31,12 +34,20 @@ namespace GodotTools.ProjectEditor
             return root != null ? new MSBuildProject(root) : null;
         }
 
+        [PublicAPI]
         public static void AddItemToProjectChecked(string projectPath, string itemType, string include)
         {
             var dir = Directory.GetParent(projectPath).FullName;
             var root = ProjectRootElement.Open(projectPath);
             Debug.Assert(root != null);
 
+            if (root.AreDefaultCompileItemsEnabled())
+            {
+                // No need to add. It's already included automatically by the MSBuild Sdk.
+                // This assumes the source file is inside the project directory and not manually excluded in the csproj
+                return;
+            }
+
             var normalizedInclude = include.RelativeToPath(dir).Replace("/", "\\");
 
             if (root.AddItemChecked(itemType, normalizedInclude))
@@ -49,6 +60,13 @@ namespace GodotTools.ProjectEditor
             var root = ProjectRootElement.Open(projectPath);
             Debug.Assert(root != null);
 
+            if (root.AreDefaultCompileItemsEnabled())
+            {
+                // No need to add. It's already included automatically by the MSBuild Sdk.
+                // This assumes the source file is inside the project directory and not manually excluded in the csproj
+                return;
+            }
+
             var normalizedOldInclude = oldInclude.NormalizePath();
             var normalizedNewInclude = newInclude.NormalizePath();
 
@@ -66,6 +84,13 @@ namespace GodotTools.ProjectEditor
             var root = ProjectRootElement.Open(projectPath);
             Debug.Assert(root != null);
 
+            if (root.AreDefaultCompileItemsEnabled())
+            {
+                // No need to add. It's already included automatically by the MSBuild Sdk.
+                // This assumes the source file is inside the project directory and not manually excluded in the csproj
+                return;
+            }
+
             var normalizedInclude = include.NormalizePath();
 
             if (root.RemoveItemChecked(itemType, normalizedInclude))
@@ -78,6 +103,13 @@ namespace GodotTools.ProjectEditor
             var root = ProjectRootElement.Open(projectPath);
             Debug.Assert(root != null);
 
+            if (root.AreDefaultCompileItemsEnabled())
+            {
+                // No need to add. It's already included automatically by the MSBuild Sdk.
+                // This assumes the source file is inside the project directory and not manually excluded in the csproj
+                return;
+            }
+
             bool dirty = false;
 
             var oldFolderNormalized = oldFolder.NormalizePath();
@@ -102,6 +134,13 @@ namespace GodotTools.ProjectEditor
             var root = ProjectRootElement.Open(projectPath);
             Debug.Assert(root != null);
 
+            if (root.AreDefaultCompileItemsEnabled())
+            {
+                // No need to add. It's already included automatically by the MSBuild Sdk.
+                // This assumes the source file is inside the project directory and not manually excluded in the csproj
+                return;
+            }
+
             var folderNormalized = folder.NormalizePath();
 
             var itemsToRemove = root.FindAllItemsInFolder(itemType, folderNormalized).ToList();
@@ -136,6 +175,29 @@ namespace GodotTools.ProjectEditor
             var root = ProjectRootElement.Open(projectPath);
             Debug.Assert(root != null);
 
+            if (root.AreDefaultCompileItemsEnabled())
+            {
+                var excluded = new List<string>();
+                result = GetAllFilesRecursive(Path.GetDirectoryName(projectPath), "*.cs").ToList();
+
+                foreach (var item in root.Items)
+                {
+                    if (string.IsNullOrEmpty(item.Condition))
+                        continue;
+
+                    if (item.ItemType != itemType)
+                        continue;
+
+                    string normalizedExclude = item.Exclude.NormalizePath();
+
+                    var glob = MSBuildGlob.Parse(normalizedExclude);
+
+                    excluded.AddRange(result.Where(includedFile => glob.IsMatch(includedFile)));
+                }
+
+                result.RemoveAll(f => excluded.Contains(f));
+            }
+
             foreach (var itemGroup in root.ItemGroups)
             {
                 if (itemGroup.Condition.Length != 0)
@@ -150,8 +212,6 @@ namespace GodotTools.ProjectEditor
 
                     var glob = MSBuildGlob.Parse(normalizedInclude);
 
-                    // TODO Check somehow if path has no blob to avoid the following loop...
-
                     foreach (var existingFile in existingFiles)
                     {
                         if (glob.IsMatch(existingFile))
@@ -165,222 +225,186 @@ namespace GodotTools.ProjectEditor
             return result.ToArray();
         }
 
-        public static void EnsureHasProjectTypeGuids(MSBuildProject project)
+        public static void MigrateToProjectSdksStyle(MSBuildProject project, string projectName)
         {
             var root = project.Root;
 
-            bool found = root.PropertyGroups.Any(pg =>
-                string.IsNullOrEmpty(pg.Condition) && pg.Properties.Any(p => p.Name == "ProjectTypeGuids"));
-
-            if (found)
+            if (!string.IsNullOrEmpty(root.Sdk))
                 return;
 
-            root.AddProperty("ProjectTypeGuids", ProjectGenerator.GodotDefaultProjectTypeGuids);
+            root.Sdk = ProjectGenerator.GodotSdkAttrValue;
+
+            root.ToolsVersion = null;
+            root.DefaultTargets = null;
+
+            root.AddProperty("TargetFramework", "net472");
+
+            // Remove obsolete properties, items and elements. We're going to be conservative
+            // here to minimize the chances of introducing breaking changes. As such we will
+            // only remove elements that could potentially cause issues with the Godot.NET.Sdk.
+
+            void RemoveElements(IEnumerable<ProjectElement> elements)
+            {
+                foreach (var element in elements)
+                    element.Parent.RemoveChild(element);
+            }
+
+            // Default Configuration
+
+            RemoveElements(root.PropertyGroups.SelectMany(g => g.Properties)
+                .Where(p => p.Name == "Configuration" && p.Condition.Trim() == "'$(Configuration)' == ''" && p.Value == "Debug"));
+
+            // Default Platform
+
+            RemoveElements(root.PropertyGroups.SelectMany(g => g.Properties)
+                .Where(p => p.Name == "Platform" && p.Condition.Trim() == "'$(Platform)' == ''" && p.Value == "AnyCPU"));
+
+            // Simple properties
+
+            var yabaiProperties = new[]
+            {
+                "OutputPath",
+                "BaseIntermediateOutputPath",
+                "IntermediateOutputPath",
+                "TargetFrameworkVersion",
+                "ProjectTypeGuids",
+                "ApiConfiguration"
+            };
+
+            RemoveElements(root.PropertyGroups.SelectMany(g => g.Properties)
+                .Where(p => yabaiProperties.Contains(p.Name)));
+
+            // Configuration dependent properties
+
+            var yabaiPropertiesForConfigs = new[]
+            {
+                "DebugSymbols",
+                "DebugType",
+                "Optimize",
+                "DefineConstants",
+                "ErrorReport",
+                "WarningLevel",
+                "ConsolePause"
+            };
+
+            foreach (var config in new[] {"ExportDebug", "ExportRelease", "Debug"})
+            {
+                var group = root.PropertyGroups
+                    .First(g => g.Condition.Trim() == $"'$(Configuration)|$(Platform)' == '{config}|AnyCPU'");
+
+                RemoveElements(group.Properties.Where(p => yabaiPropertiesForConfigs.Contains(p.Name)));
+
+                if (group.Count == 0)
+                {
+                    // No more children, safe to delete the group
+                    group.Parent.RemoveChild(group);
+                }
+            }
+
+            // Godot API References
+
+            var apiAssemblies = new[] {ApiAssemblyNames.Core, ApiAssemblyNames.Editor};
+
+            RemoveElements(root.ItemGroups.SelectMany(g => g.Items)
+                .Where(i => i.ItemType == "Reference" && apiAssemblies.Contains(i.Include)));
+
+            // Microsoft.NETFramework.ReferenceAssemblies PackageReference
+
+            RemoveElements(root.ItemGroups.SelectMany(g => g.Items).Where(i =>
+                i.ItemType == "PackageReference" &&
+                i.Include.Equals("Microsoft.NETFramework.ReferenceAssemblies", StringComparison.OrdinalIgnoreCase)));
+
+            // Imports
+
+            var yabaiImports = new[]
+            {
+                "$(MSBuildBinPath)/Microsoft.CSharp.targets",
+                "$(MSBuildBinPath)Microsoft.CSharp.targets"
+            };
+
+            RemoveElements(root.Imports.Where(import => yabaiImports.Contains(
+                import.Project.Replace("\\", "/").Replace("//", "/"))));
+
+            // 'EnableDefaultCompileItems' and 'GenerateAssemblyInfo' are kept enabled by default
+            // on new projects, but when migrating old projects we disable them to avoid errors.
+            root.AddProperty("EnableDefaultCompileItems", "false");
+            root.AddProperty("GenerateAssemblyInfo", "false");
+
+            // Older AssemblyInfo.cs cause the following error:
+            // 'Properties/AssemblyInfo.cs(19,28): error CS8357:
+            // The specified version string contains wildcards, which are not compatible with determinism.
+            // Either remove wildcards from the version string, or disable determinism for this compilation.'
+            // We disable deterministic builds to prevent this. The user can then fix this manually when desired
+            // by fixing 'AssemblyVersion("1.0.*")' to not use wildcards.
+            root.AddProperty("Deterministic", "false");
 
             project.HasUnsavedChanges = true;
+
+            var xDoc = XDocument.Parse(root.RawXml);
+
+            if (xDoc.Root == null)
+                return; // Too bad, we will have to keep the xmlns/namespace and xml declaration
+
+            XElement GetElement(XDocument doc, string name, string value, string parentName)
+            {
+                foreach (var node in doc.DescendantNodes())
+                {
+                    if (!(node is XElement element))
+                        continue;
+                    if (element.Name.LocalName.Equals(name) && element.Value == value &&
+                        element.Parent != null && element.Parent.Name.LocalName.Equals(parentName))
+                    {
+                        return element;
+                    }
+                }
+
+                return null;
+            }
+
+            // Add comment about Microsoft.NET.Sdk properties disabled during migration
+
+            GetElement(xDoc, name: "EnableDefaultCompileItems", value: "false", parentName: "PropertyGroup")
+                .AddBeforeSelf(new XComment("The following properties were overriden during migration to prevent errors.\n" +
+                                            "    Enabling them may require other manual changes to the project and its files."));
+
+            void RemoveNamespace(XElement element)
+            {
+                element.Attributes().Where(x => x.IsNamespaceDeclaration).Remove();
+                element.Name = element.Name.LocalName;
+
+                foreach (var node in element.DescendantNodes())
+                {
+                    if (node is XElement xElement)
+                    {
+                        // Need to do the same for all children recursively as it adds it to them for some reason...
+                        RemoveNamespace(xElement);
+                    }
+                }
+            }
+
+            // Remove xmlns/namespace
+            RemoveNamespace(xDoc.Root);
+
+            // Remove xml declaration
+            xDoc.Nodes().FirstOrDefault(node => node.NodeType == XmlNodeType.XmlDeclaration)?.Remove();
+
+            string projectFullPath = root.FullPath;
+
+            root = ProjectRootElement.Create(xDoc.CreateReader());
+            root.FullPath = projectFullPath;
+
+            project.Root = root;
         }
 
-        ///  Simple function to make sure the Api assembly references are configured correctly
-        public static void FixApiHintPath(MSBuildProject project)
+        public static void EnsureGodotSdkIsUpToDate(MSBuildProject project)
         {
             var root = project.Root;
+            string godotSdkAttrValue = ProjectGenerator.GodotSdkAttrValue;
 
-            void AddPropertyIfNotPresent(string name, string condition, string value)
-            {
-                if (root.PropertyGroups
-                    .Any(g => (string.IsNullOrEmpty(g.Condition) || g.Condition.Trim() == condition) &&
-                              g.Properties
-                                  .Any(p => p.Name == name &&
-                                            p.Value == value &&
-                                            (p.Condition.Trim() == condition || g.Condition.Trim() == condition))))
-                {
-                    return;
-                }
-
-                root.AddProperty(name, value).Condition = " " + condition + " ";
-                project.HasUnsavedChanges = true;
-            }
-
-            AddPropertyIfNotPresent(name: "ApiConfiguration",
-                condition: "'$(Configuration)' != 'ExportRelease'",
-                value: "Debug");
-            AddPropertyIfNotPresent(name: "ApiConfiguration",
-                condition: "'$(Configuration)' == 'ExportRelease'",
-                value: "Release");
-
-            void SetReferenceHintPath(string referenceName, string condition, string hintPath)
-            {
-                foreach (var itemGroup in root.ItemGroups.Where(g =>
-                    g.Condition.Trim() == string.Empty || g.Condition.Trim() == condition))
-                {
-                    var references = itemGroup.Items.Where(item =>
-                        item.ItemType == "Reference" &&
-                        item.Include == referenceName &&
-                        (item.Condition.Trim() == condition || itemGroup.Condition.Trim() == condition));
-
-                    var referencesWithHintPath = references.Where(reference =>
-                        reference.Metadata.Any(m => m.Name == "HintPath"));
-
-                    if (referencesWithHintPath.Any(reference => reference.Metadata
-                        .Any(m => m.Name == "HintPath" && m.Value == hintPath)))
-                    {
-                        // Found a Reference item with the right HintPath
-                        return;
-                    }
-
-                    var referenceWithHintPath = referencesWithHintPath.FirstOrDefault();
-                    if (referenceWithHintPath != null)
-                    {
-                        // Found a Reference item with a wrong HintPath
-                        foreach (var metadata in referenceWithHintPath.Metadata.ToList()
-                            .Where(m => m.Name == "HintPath"))
-                        {
-                            // Safe to remove as we duplicate with ToList() to loop
-                            referenceWithHintPath.RemoveChild(metadata);
-                        }
-
-                        referenceWithHintPath.AddMetadata("HintPath", hintPath);
-                        project.HasUnsavedChanges = true;
-                        return;
-                    }
-
-                    var referenceWithoutHintPath = references.FirstOrDefault();
-                    if (referenceWithoutHintPath != null)
-                    {
-                        // Found a Reference item without a HintPath
-                        referenceWithoutHintPath.AddMetadata("HintPath", hintPath);
-                        project.HasUnsavedChanges = true;
-                        return;
-                    }
-                }
-
-                // Found no Reference item at all. Add it.
-                root.AddItem("Reference", referenceName).Condition = " " + condition + " ";
-                project.HasUnsavedChanges = true;
-            }
-
-            const string coreProjectName = "GodotSharp";
-            const string editorProjectName = "GodotSharpEditor";
-
-            const string coreCondition = "";
-            const string editorCondition = "'$(Configuration)' == 'Debug'";
-
-            var coreHintPath = $"$(ProjectDir)/.mono/assemblies/$(ApiConfiguration)/{coreProjectName}.dll";
-            var editorHintPath = $"$(ProjectDir)/.mono/assemblies/$(ApiConfiguration)/{editorProjectName}.dll";
-
-            SetReferenceHintPath(coreProjectName, coreCondition, coreHintPath);
-            SetReferenceHintPath(editorProjectName, editorCondition, editorHintPath);
-        }
-
-        public static void MigrateFromOldConfigNames(MSBuildProject project)
-        {
-            var root = project.Root;
-
-            bool hasGodotProjectGeneratorVersion = false;
-            bool foundOldConfiguration = false;
-
-            foreach (var propertyGroup in root.PropertyGroups.Where(g => string.IsNullOrEmpty(g.Condition)))
-            {
-                if (!hasGodotProjectGeneratorVersion && propertyGroup.Properties.Any(p => p.Name == "GodotProjectGeneratorVersion"))
-                    hasGodotProjectGeneratorVersion = true;
-
-                foreach (var configItem in propertyGroup.Properties
-                    .Where(p => p.Condition.Trim() == "'$(Configuration)' == ''" && p.Value == "Tools"))
-                {
-                    configItem.Value = "Debug";
-                    foundOldConfiguration = true;
-                    project.HasUnsavedChanges = true;
-                }
-            }
-
-            if (!hasGodotProjectGeneratorVersion)
-            {
-                root.PropertyGroups.First(g => string.IsNullOrEmpty(g.Condition))?
-                    .AddProperty("GodotProjectGeneratorVersion", Assembly.GetExecutingAssembly().GetName().Version.ToString());
-                project.HasUnsavedChanges = true;
-            }
-
-            if (!foundOldConfiguration)
-            {
-                var toolsConditions = new[]
-                {
-                    "'$(Configuration)|$(Platform)' == 'Tools|AnyCPU'",
-                    "'$(Configuration)|$(Platform)' != 'Tools|AnyCPU'",
-                    "'$(Configuration)' == 'Tools'",
-                    "'$(Configuration)' != 'Tools'"
-                };
-
-                foundOldConfiguration = root.PropertyGroups
-                    .Any(g => toolsConditions.Any(c => c == g.Condition.Trim()));
-            }
-
-            if (foundOldConfiguration)
-            {
-                void MigrateConfigurationConditions(string oldConfiguration, string newConfiguration)
-                {
-                    void MigrateConditions(string oldCondition, string newCondition)
-                    {
-                        foreach (var propertyGroup in root.PropertyGroups.Where(g => g.Condition.Trim() == oldCondition))
-                        {
-                            propertyGroup.Condition = " " + newCondition + " ";
-                            project.HasUnsavedChanges = true;
-                        }
-
-                        foreach (var propertyGroup in root.PropertyGroups)
-                        {
-                            foreach (var prop in propertyGroup.Properties.Where(p => p.Condition.Trim() == oldCondition))
-                            {
-                                prop.Condition = " " + newCondition + " ";
-                                project.HasUnsavedChanges = true;
-                            }
-                        }
-
-                        foreach (var itemGroup in root.ItemGroups.Where(g => g.Condition.Trim() == oldCondition))
-                        {
-                            itemGroup.Condition = " " + newCondition + " ";
-                            project.HasUnsavedChanges = true;
-                        }
-
-                        foreach (var itemGroup in root.ItemGroups)
-                        {
-                            foreach (var item in itemGroup.Items.Where(item => item.Condition.Trim() == oldCondition))
-                            {
-                                item.Condition = " " + newCondition + " ";
-                                project.HasUnsavedChanges = true;
-                            }
-                        }
-                    }
-
-                    foreach (var op in new[] {"==", "!="})
-                    {
-                        MigrateConditions($"'$(Configuration)|$(Platform)' {op} '{oldConfiguration}|AnyCPU'", $"'$(Configuration)|$(Platform)' {op} '{newConfiguration}|AnyCPU'");
-                        MigrateConditions($"'$(Configuration)' {op} '{oldConfiguration}'", $"'$(Configuration)' {op} '{newConfiguration}'");
-                    }
-                }
-
-                MigrateConfigurationConditions("Debug", "ExportDebug");
-                MigrateConfigurationConditions("Release", "ExportRelease");
-                MigrateConfigurationConditions("Tools", "Debug"); // Must be last
-            }
-        }
-
-        public static void EnsureHasNugetNetFrameworkRefAssemblies(MSBuildProject project)
-        {
-            var root = project.Root;
-
-            bool found = root.ItemGroups.Any(g => string.IsNullOrEmpty(g.Condition) && g.Items.Any(
-                item => item.ItemType == "PackageReference" && item.Include == "Microsoft.NETFramework.ReferenceAssemblies"));
-
-            if (found)
+            if (!string.IsNullOrEmpty(root.Sdk) && root.Sdk.Trim().Equals(godotSdkAttrValue, StringComparison.OrdinalIgnoreCase))
                 return;
 
-            var frameworkRefAssembliesItem = root.AddItem("PackageReference", "Microsoft.NETFramework.ReferenceAssemblies");
-
-            // Use metadata (child nodes) instead of attributes for the PackageReference.
-            // This is for compatibility with 3.2, where GodotTools uses an old Microsoft.Build.
-            frameworkRefAssembliesItem.AddMetadata("Version", "1.0.0");
-            frameworkRefAssembliesItem.AddMetadata("PrivateAssets", "All");
-
+            root.Sdk = godotSdkAttrValue;
             project.HasUnsavedChanges = true;
         }
     }