C#: GodotNETSdk should also suppress AppendRuntimeIdentifierToOutputPath.

[invino4]

Tested versions

Repo: 4.4.0+
(Untested on previous versions, but unlikely a regression)

System information

Windows 10 - Godot Engine v4.4.stable.mono.official.4c311cbee

Issue description

Description

The GodotNETSdk includes the following line in Sdk.props:

<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>

This appears to be intended to force all output paths to be exactly .godot\mono\temp\bin instead of .godot\mono\temp\bin\net8.0. However, if the csproj file also includes a <RuntimeIdentifier> then the output paths are still altered by NetSDK leading to something like .godot\mono\temp\bin\win-x64 (or the like). Perhaps Sdk.props should read:

<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
<AppendRuntimeIdentifierToOutputPath>false</AppendRuntimeIdentifierToOutputPath>

This appears to resolve the issue and is in line with the original intent.

Discussion

This begs the question though: why override the output (and intermediary) paths at all? Would it not be better to acquire the output paths from msbuild and then pass those to the Godot runtime for dynamic loading (or have the runtime/Editor invoke msbuild to evaluate $(OutputPath))? This would avoid the necessity to override the output paths to begin with and might make the Godot SDK and Editor more robust to future NetSDK changes that impact output paths. Is there another reason for hard coding the paths?

Steps to reproduce

Add <RuntimeIdentifier>win-x64</RuntimeIdentifier> to the csproj file for the targeted Godot project. Rebuild.

Minimal reproduction project (MRP)

Any project will work.

[raulsntos]

We can invoke MSBuild to evaluate $(OutputPath) and we do that in the new dotnet module, because I want C# projects to be more flexible. Users could define their own output path or enable artifacts.

However, changing the output path for existing projects is a bit delicate because the moment the projects update to the new SDK (which happens when you open a project with a new Godot version), the path changes and Godot won't be able to find the previously built DLLs. This means the inspector will be outdated until you rebuild the project, and your scenes/resources may lose data if you save them before building the project¹.

Because of this, we'd want to keep the current output path as the default. However, I don't think we can allow changing the output path, we set it in Sdk.props which is evaluated very early so it propagates to subsequent MSBuild tasks and targets.

Also, to better understand your scenario. Can you explain when you use the RuntimeIdentifier property and for what purpose? What kind of issues are you running into? For example, I imagine the inspector is using an outdated build from before you set the RuntimeIdentifier property. But exporting should work, since the command we use specifies a custom output path.

Footnotes

1. This problem already exists in some scenarios: https://github.com/godotengine/godot/issues/90755. But changing the path would make it worse. ↩

[invino4]

I am setting RuntimeIdentifier because I am taking a package dependency on a NuGet package with RID-specific (native) bits. To import the package the build system requires that you specify one or more RIDs in RuntimeIdentifier(s) that match those RID exported by the NuGet. In this case the RID I'm interested in is win-x64 which is the Export RID for Godot anyway, so no problem. Except... when you add RuntimeIdentifier to a csproj normally NetSDK adds the RID to the output path. So, by default instead of bin\Debug\net8.0 you get bin\Debug\net8.0\win-x64.

This is all fine. And, as you said, this doesn't affect exporting, which works fine. But, the editor doesn't work at all after this change.

The Sdk.props does set both BaseOutputPath and OutputPath, but I suspect that the setting of OutputPath really has no effect because that value is recomputed by the Build target. The Build target recomputes it as $(BaseOutputPath)\$(Configuration)\$(TargetFramework)\. However, there is an extensibility property AppendTargetFrameworkToOutputPath which controls whether $(TargetFramework) is actually appended. This is also set to false by Sdk.props which causes the recomputation of OutputPath to be equal to $(BaseOutputPath)\$(Configuration)\ instead, which happens to be exactly what Sdk.props set it to in the first place. So, it appears like the setting of OutputPath actually did something (even if it didn't).

It turns out that the real recomputation for OutputPath is even longer. It is actually $(BaseOutputPath)\$(Configuration)\$(TargetFramework)\$(RuntimeIdentifier) BUT, if RuntimeIdentifier is empty this becomes $(BaseOutputPath)\$(Configuration)\$(TargetFramework)\\ which is effectively the same as $(BaseOutputPath)\$(Configuration)\$(TargetFramework)\ (at least to msbuild). When RuntimeIdentifier is NOT empty, however, then setting AppendTargetFrameworkToOutputPath (as Sdk.props does) causes the recomputation to become $(BaseOutputPath)\$(Configuration)\$(RuntimeIdentifier). When Sdk.props sets BaseOutputPath to .godot\mono\temp\bin\ this leads to a value of .godot\mono\temp\bin\win-x64. Everything in msbuild-land still builds fine here, but the binaries are binplaced to .godot\mono\temp\bin\win-x64.

But, as far as I can tell, the Editor NEVER looks in that directory. I'm guessing it is hardcoded to look in .godot\mono\temp\bin\, is that true? I tried restarting, rebuilding, cleaning, etc. everything... but could find no way to get the Editor to look in .godot\mono\temp\bin\win-x64.

As a hack, I also set AppendRuntimeIdentifierToOutputPath to false which, as with AppendTargetFrameworkToOutputPath causes the recomputation to become $(BaseOutputPath)\$(Configuration) once again, and then the Editor is happy. Hence, the suggested fix above.

But... it feels ugly and brittle. It would be nicer if the Editor just did the right thing. If it detected the default output path that NetSDK comes up with (whatever it happens to be) and just uses it. If the output path changes (say because RuntimeIdentifier is set, or TargetPlatform is changed) then the Editor just rebuilds everything. That is what the normal msbuild workflow would do. I'm not asking for it to work while the Editor is running, but on a clean rebuild/reopen maybe it should work?

[raulsntos]

Thank so much you for the detailed explanation.

But, as far as I can tell, the Editor NEVER looks in that directory. I'm guessing it is hardcoded to look in .godot\mono\temp\bin\, is that true?

Yes, right here:

godot/modules/mono/godotsharp_dirs.cpp

Lines 141 to 142 in 987832b

	// TODO use paths from csproj
	res_temp_assemblies_dir = res_data_dir.path_join("temp").path_join("bin").path_join(_get_expected_build_config());

Sorry if my previous comment was confusing, when I said the new dotnet module is using MSBuild to evaluate $(OutputPath) I was referring to an unreleased product. The current module is still using a hardcoded path.

If the output path changes (say because RuntimeIdentifier is set, or TargetPlatform is changed) then the Editor just rebuilds everything.

This would require implementing this proposal:

• Auto (re)build project when C# script changes godot-proposals#2920

And keep in mind a lot of users would still prefer to keep that disabled.

The Sdk.props does set both BaseOutputPath and OutputPath, but I suspect that the setting of OutputPath really has no effect because that value is recomputed by the Build target.

I think this is what you are referring to:

https://github.com/dotnet/sdk/blob/3c21adb924c3c33682255711fd42a714210d8075/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets#L338-L355

<OutputPath Condition="'$(UseArtifactsOutput)' != 'true'">$(OutputPath)$(RuntimeIdentifier)\</OutputPath>

It looks like it just appends to our OutputPath value. But it may be true that we don't need to specifically set the OutputPath property.

[invino4]

The Build target basically expects to "cons-up" the OutputPath. This has been made complicated by the distributed nature of the append actions and the introduction of the "artifacts" feature. But you can see where OutputPath would normally get set here:

https://github.com/dotnet/sdk/blob/3c21adb924c3c33682255711fd42a714210d8075/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.DefaultOutputPaths.targets#L99C5-L99C130

<OutputPath Condition="'$(OutputPath)' == ''">$(BaseOutputPath)$(_PlatformToAppendToOutputPath)$(Configuration)\</OutputPath>

Then subsequent rules just append stuff to the end. The logic in NetSDK doesn't really keep track of whether you manually set OutputPath after the line above. (Maybe it should? Seems like a bug.) But then it just appends segments. The TargetPlatform and RuntimeIdentifier get so appended at later points in the evaluation (oblivious to the fact that OutputPath didn't actually get set by the above because it was manually overridden).

My understanding is that the overall goal here is to allow multi-platform targeting out of the same enlistment. I.e. you can build both win-x64, linux-x64, and android all from the same directory tree without the outputs conflicting (by having to write on top of each other). You can build all platforms at the same time with the same msbuild execution. This requires, however, that all outputs that might conflict be destined to land in different directories. I think this makes for a much more efficient build system and works well with automation and publishing. Given that Godot really wants to support multi-targeting for projects it would be great if it played well in this world. If Godot could dock to the existing conventions used/enforced by NetSDK then it is much less likely to run into friction in the future. (e.g. There really shouldn't be the need for ExportDebug or ExportRelease configurations as these concepts are already manifested by the NetSDK complex nonsense around platform\rid\publish conventions.)

---

As to keeping the code/data loaded in the Editor in sync with the code/data on disk... This problem seems bigger and has more widespread implications. I would have to think more about this problem to have anything actually intelligent to say. My first impressions are that this isn't really about automatically recompiling the code. It's really a serialization synchronization problem that requires code and data to evolve along the same linear ordering.

Warning

Random musing follows...

At first blush there are at least four parties that need to stay in sync:

1. The data serialization on disk (e.g. a tscn file that encodes the values of Exports).
2. The C# source on disk.
3. The data serialization active in memory in the Editor.
4. The assembly bits loaded into the Editor.

Let's assume a serialization regime with partial materialization (i.e. unknown properties are ignored during materialization) and partial materialization does NOT support round-tripping unknown data (i.e. non-materialized data is lost during reserialization). This means that (1) can be downlevel from (2), but as soon as (2) round-trips (1) it will write it back as (1`) at the current version of (2). If (1) is uplevel from (2) then new additive data will be misinterpreted as downlevel data that was dropped during evolution. A version of (2) older than (1) should therefore NEVER be run on (1).

This is generally a pretty stable environment as long as (1) and (2) moved forward in lockstep. (1) and (2) represent the persistent state of the system. (2) will be used to materialize (1). (1) and (2) can change together like when source control does a sync, but this doesn't break the lockstep property.

The same is mostly true for (3) and (4) as well.

The tricky part is when (1) and (2) both have changed, and the Editor detects it. By the above logic it is "unsafe" to reload (3) without first reloading (4), because doing so would lead to a downlevel materialization of uplevel data. Instead, the Editor should refuse to reload (3) until it can reload (4). That doesn't mean it has to force a build of (2) to produce (4). But it does mean that it shouldn't try to load (3) and display it. Maybe it should freeze the scene editor window and warn the user that a recompilation is necessary to unfreeze. The user can then hit Build or switch back to the IDE for more editing. This is particularly import if the Build is going to fail (due to a code bug).

The key issue is detecting that (1) is uplevel with respect to (4). The relationship between (1) and (4) needs to be present within (1) and (3). As a straw man, what if we consider the case where (1) includes a hash of the source (2) that produce the version of (4) that wrote (1). If the hash of (2) that produced (1) doesn't match the hash of (2) that produced (4) then (3) should be frozen until (2) is recompiled. I'm not sure exactly what "hash (2)" should really mean but we can think on that separately.

At any rate. If the relationship between the (4) and (2) was about linear ordering instead, then it doesn't matter what folder (4) lives in. When it is time to recompile then just do a build to produce a new (4). Then load that new (4) in its entirety. Then reload (3) from (1). All is good.