Moving .cs file in editor clobbers .csproj compile includes

[nobuyukinyuu]

Godot version:
3.2.3 stable.mono.official

OS/device including version:
Windows 10 x64

Issue description:
I have a somewhat lazily customized csproj file which includes all files without having to deal with the "fragility" of Godot's handling of includes. To handle adding new .cs files to my project, I've included this line:

  <ItemGroup>
    <Compile Include="**\*.cs" />
  </ItemGroup>

This usually survives changes and upgrades to the project format, however, I noticed that moving a cs file in the editor disregards this customization. The behavior is that it will remove the above snippet and replace it with one including only the moved file. This obviously breaks a lot of things. Not sure if I'm using an unrecommended way to include files in my project, but it's easier than forcing myself to create every new cs file in godot when I'd rather just be lazy and do it in vscode.

Steps to reproduce:

1. Modify a CSProj file where the compile includes itemGroup contains only the above snippet.
2. In the Godot editor, try moving one of your scripted cs files, the kind that is already linked to a node in the project, to trigger a refactor.
3. Check csproj file

Minimal reproduction project:

[nobuyukinyuu]

Update: The default project template seems to have changed a lot between versions 3.2.2 and 3.2.3 and I should note that this requires a project file which has <EnableDefaultCompileItems> set to false. This is still not an uncommon scenario when a project has script templates, since these need to be manually excluded from the project.

[akien-mga]

Is this still reproducible in 3.4 RC 1 or later?

[nobuyukinyuu]

Seems so. Didn't work when drag and dropping, but when I right-clicked a test script and use "Move to..." it did this:

(Line 11 was added automatically when upgrading the project template from SDK from 3.2.3; Line 9 was only improperly modified after using "Move to..." dialog)

[raulsntos]

The GodotSharpEditor EditorPlugin connects to the files_moved signal of the File System dock, and executes the _FileSystemDockFileMoved method which inside calls ProjectUtils.RenameItemInProjectChecked.

The rename method tries to find the <Compile> element of the .cs file before it was moved to rename it with the new path:

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

Lines 59 to 82 in ce11f8f

	public static void RenameItemInProjectChecked(string projectPath, string itemType, string oldInclude, string newInclude)
	{
	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 normalizedOldInclude = oldInclude.NormalizePath();
	var normalizedNewInclude = newInclude.NormalizePath();
	var item = root.FindItemOrNullAbs(itemType, normalizedOldInclude);
	if (item == null)
	return;
	item.Include = normalizedNewInclude.RelativeToPath(dir).Replace("/", "\\");
	root.Save();
	}

The method that tries to find the old element is FindItemOrNullAbs:

godot/modules/mono/editor/GodotTools/GodotTools.ProjectEditor/ProjectExtensions.cs

Lines 37 to 59 in ce11f8f

	public static ProjectItemElement FindItemOrNullAbs(this ProjectRootElement root, string itemType, string include, bool noCondition = false)
	{
	string normalizedInclude = Path.GetFullPath(include).NormalizePath();
	foreach (var itemGroup in root.ItemGroups)
	{
	if (noCondition && itemGroup.Condition.Length != 0)
	continue;
	foreach (var item in itemGroup.Items)
	{
	if (item.ItemType != itemType)
	continue;
	var glob = MSBuildGlob.Parse(Path.GetFullPath(item.Include).NormalizePath());
	if (glob.IsMatch(normalizedInclude))
	return item;
	}
	}
	return null;
	}

This method iterates over the <Compile> elements and checks if its the right path, unfortunately since you are using globbing the path ./*.cs is a match for any script in your project so it thinks it found it and it tries to use that one.

I'm not sure what would be the proper way to fix this, but have you tried having that <Compile> element in a separate <ItemGroup> below? That way Godot may use the first one and never reach the second one:

<Project Sdk="Godot.NET.Sdk/3.3.0">
	<PropertyGroup>
		<TargetFramework>netstandard2.1</TargetFramework>
		<EnableDefaultCompileItems>false</EnableDefaultCompileItems>
		...
	</PropertyGroup>
	<ItemGroup>
		<Compile Include="new_script.cs"/>
		...
	</ItemGroup>
	<ItemGroup>
		<Compile Include="./*.cs" Exclude="script_templates/*.cs">
	</ItemGroup>
</Project>

Also there is the mono/project/auto_update_project project setting which you may be interested in, but it does not seem like it's being used when moving files.

[nobuyukinyuu]

Perhaps a better solution would be for Godot to have its includes handled in its own ItemGroup, or the last ItemGroup found. The behavior in 3.2.3 (the version I'm currently using) does not seem to mess with the existing one when creating a new script if the group it appears to want to modify has been touched or is formatted differently, it would seem, as I'm able to create new scripts easily without having to modify my project file to accomodate. The undesired behavior only occurs (as far as I can tell) under the specific circumstances I've outlined. When moving files, I simply do so outside of the editor, and I've found this to be a more reasonable workaround since right now, 3.2.3 doesn't mess around with my project file unless I move something using the editor.

Edit: For reference, here is a complete copy of my current project file:

<Project Sdk="Godot.NET.Sdk/3.2.3">
  <PropertyGroup>
    <TargetFramework>netstandard2.1</TargetFramework>
    <EnableDefaultCompileItems>false</EnableDefaultCompileItems>
    <!-- <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies>     -->
  </PropertyGroup>
  <ItemGroup>
    <!-- <Compile Include="**\*.cs" Exclude="script_templates/*.cs" /> -->
    <Compile Include="./*.cs" Exclude="script_templates/*.cs" />
    <Compile Include="FMCore/**/*.cs" />
  </ItemGroup>
</Project>

[raulsntos]

I agree, moving the file outside of Godot and editing the .csproj manually seems more convenient.

As I explained earlier, the behavior when moving is trying to find a <Compile> that applies to the moved file previous path to modify it with the new path and since your glob path satisfies that condition it uses that one. When adding a new script it uses the ProjectUtils.AddItemToProjectChecked method instead which will try to find a <Compile> that applies to the new script path and will only modify the .csproj if it can't find one, again since your glob path satisfies the condition it does not add anything to the .csproj.

I had an idea, what if on moving we checked if the new path is also covered by the found glob in which case we can assume it does not need to be modified. Although I'm not sure if that may bring other undesired side-effects.

[akien-mga]

Fixed by #54262.

[nobuyukinyuu]

This issue has returned as of 3.4.3 stable. The conditions to get it to occur are to simply create a new C# script file. I normally don't create new scripts from Godot unless I need to use a script template, so I didn't notice the issue before. I've had to set my csproj file to read-only as a temporary workaround. Doing this throws up the following message when creating a new script:

 modules/mono/mono_gd/gd_mono_utils.cpp:369 - System.UnauthorizedAccessException: Access to the path "D:\redacted\PhaseEngine.csproj" is denied.
 modules/mono/editor/csharp_project.cpp:64 - Method failed.

Don't know if that will help pinpoint the regression.

[raulsntos]

I have been unable to reproduce it in 3.4.3 stable.

The code for AddItemToProjectChecked that gets executed when a new script is created checks if there is an item in the csproj that includes the file to be added and if that's the case it won't modify the csproj (so if you have a glob path that already satisfies the path of the newly created script it won't be added to the csproj, otherwise its path will be added).

Also, the code in that file (ProjectUtils) hasn't really been modified since my PR that fixed this issue 5 months ago so it seems odd that it will break without any changes. This means the issue you are describing could be something that I missed or a new/different issue so my PR didn't fix it. Since I don't seem to be able to reproduce it or maybe I'm misunderstanding the steps to do so, could you describe once again the steps required to reproduce this issue in as much detail as you can?

In order to make reproducing the issue easier it'd be great to have a minimal reproduction project that represents the initial state of the project (a simple project with a csproj in its initial state that already contains the compile items with globbing), the csproj in its final state after Godot modifies it (the incorrect result) and the expected csproj (what you expected Godot to do after following the reproduction steps).

After following the steps (I assume it'd be something like adding a C# script in a specific folder that is not included by the glob path) I should get the same csproj as the one that was provided for the final state instead of the one provided as the expected csproj.

---

Having said all that, if all you need is to exclude some files have you considered adding them to the DefaultItemExcludes? That would allow you to keep EnableDefaultCompileItems enabled and still exclude certain scripts from the compilation. Here is an example:

<Project Sdk="Godot.NET.Sdk/3.3.0">

  <PropertyGroup>
    <TargetFramework>net472</TargetFramework>
  </PropertyGroup>

  <!-- Exclude templates -->
  <PropertyGroup>
    <DefaultItemExcludes>$(DefaultItemExcludes);script_templates/*.cs</DefaultItemExcludes>
  </PropertyGroup>

</Project>

[nobuyukinyuu]

MinimalCSharpProject.zip

Sincerest apologies, I'm currently suffering from some connectivity issues that will make it difficult to address things for the next few days. I'm attempting to attach a minimal reproduction project to this comment. I may try modifying my project to implement your suggestions, but solving this issue is still important to me.

This project should be set up correctly to replicate the issue. To reproduce it you can do one of two things, and the issues may not be the same but are definitely related. The first is to tried deleting new_script.cs from the editor in the icon view. This should delete line 8 of the project file.

The second way to reproduce the issue is also from the editors icon item list for the file system. Right click and select new script. Select language as c sharp, and use one of the project templates to create another script. This should replace line 8 with a compiler include reference to the script.

[raulsntos]

Thank you, I can reproduce the issue with the provided steps.

[raulsntos]

@nobuyukinyuu I believe #59521 should fix the issue for the two reproduction steps you mentioned.