C#: Discontinue GodotNuGetFallbackFolder

[RedworkDE]

There is no real need extract the nupkgs, NuGet will do this automatically, as when using a folder with the nupkgs as a source.
This will also removes the copying offline packages build phase and stop some cache issues with local builds.

While testing the first commit, I found the way sources were added to just no work at all, so switched to using the dotnet nuget add/remove source commands to manage the package source in the third commit and in the process I ended up refactoring the way the dotnet command is called to deduplicate that code.

Get rid of the GodotNuGetFallbackFolder and rely on packages distributed via Nuget.org instead (or the --push-nupkgs-local option for source builds)

Atm this PR doesn't try to remove the GodotSharp/Tools/nupks folder to avoid breaking the build system. (also it should be possible to remove the GodotSharp/Api folder as well, but just doing that breaks the editor)

[raulsntos]

I don't think we want to get rid of BuildAsync. The original plan was to eventually implement building C# projects in the background (see godotengine/godot-proposals#849).

About the argumentList, we were using Collection so we can add arguments directly to ProcessStartInfo.ArgumentList without having to allocate a temporary list. But now you are allocating a Collection only to convert it to an array later.

I feel like it's a bit weird to have a BuildArguments method that is constructing arguments for a Process, and then ExecuteDotNet which is a wrapper of dotnet CLI that hides the Process inside. I think we should either revert to the Build/LaunchBuild methods that takes the constructed BuildInfo and just start the Process, or fully commit to creating a dotnet CLI wrapper (but not sure if it's worth going that far).

As for the NuGet source, I don't think this is the path we want to take. The NuGet packages should be available in NuGet.org and we plan to stop distributing the NuGet packages with the Godot binary in the future (when we implement Editor unification in a future 4.x release). I think it's good to remove the GodotNuGetFallbackFolder and Godot.Offline.Config though, and I was planning to do it as part of Editor unification.

[RedworkDE]

The issue with removing the nuget fallback entirely is that this means that dev builds without push-nupkgs-local are broken and that official builds must push the packages to nuget immediately, and i wasn't sure if this was the direction things were suppose to go.

re: the whole refactor, if we don't add more uses of the dotnet command that whole refactor isn't really necessary for this PR, tho I think that code still deserves some cleanup in an other PR (at least deduplicate the LaunchBuild & LaunchPublish methods).

If the plan is to entirely rely on the online packages this whole PR becomes a lot simpler.

[neikeq]

I'm fine with this change for 3.x, but for 4.x I would prefer to just get rid of this entirely and not distribute the nupkgs with Godot.

The issue with removing the nuget fallback entirely is that this means that dev builds without push-nupkgs-local are broken

That's the workflow we recommend, and it's the reason I added --push-nupkgs-local. Even with the nupkgs fallback folder, old packages could still be fetched from the NuGet cache, which --push-nupkgs-local takes care of clearing.

[neikeq]

... and we plan to stop distributing the NuGet packages with the Godot binary in the future (when we implement Editor unification in a future 4.x release).

Editor unification is not required for this. I don't know if our setup logic for the fallback folder requires the nupkgs to be available. If not, we could stop distributing the nupkgs right away.

[raulsntos]

... and we plan to stop distributing the NuGet packages with the Godot binary in the future (when we implement Editor unification in a future 4.x release).

Editor unification is not required for this.

Sorry if I made it sound like it was. What I meant was that, we would have to stop distributing the NuGet packages when that happens, since users that don't use C# won't need them. So it fits well together IMO, but it should be possible to do it earlier.

[RedworkDE]

I don't know if our setup logic for the fallback folder requires the nupkgs to be available. If not, we could stop distributing the nupkgs right away.

Not sure which logic exactly you are referring to, but the editor code that populated the GodotNuGetFallbackFolder used it, the --push-nupkgs-local option does not. not sure how the release team handles publishing the packages, so I left them for now, but stopping generation of them is just deleting another 8 lines.

[raulsntos]

Maybe referring to Godot.Offline.Config. In the past we've had issues where if the directory GodotNuGetFallbackFolder didn't exist, doing a dotnet restore would just fail resulting in C# projects failing to build (not just Godot projects).

But I think as long as the directory exists, dotnet restore will work. If the nupkgs are not found in the directory it should continue looking at other NuGet sources to try and find the packages.

---

The --push-nupkgs-local should clear the nupkgs before pushing them:

godot/modules/mono/build_scripts/build_assemblies.py

Lines 320 to 321 in 30884ee

	if push_nupkgs_local:
	args += ["/p:ClearNuGetLocalCache=true", "/p:PushNuGetToLocalSource=" + push_nupkgs_local]

Also take a look at Directory.Build.targets.

---

not sure how the release team handles publishing the packages

We build Godot as described in Compiling with .NET but maybe with a few different flags or environment variables to set the Godot version, then the resulting nupkgs are published to NuGet.org by the build_release.sh script.

[neikeq]

I don't know if our setup logic for the fallback folder requires the nupkgs to be available. If not, we could stop distributing the nupkgs right away.

Not sure which logic exactly you are referring to ...

I meant this:

godot/modules/mono/editor/GodotTools/GodotTools/Build/NuGetUtils.cs

Lines 235 to 251 in 0cd1483

	void AddPackage(string packageId, string packageVersion)
	{
	string nupkgPath = Path.Combine(nupkgsLocation, $"{packageId}.{packageVersion}.nupkg");
	AddPackageToFallbackFolder(fallbackFolder, nupkgPath, packageId, packageVersion);
	}
	foreach (var (packageId, packageVersion) in PackagesToAdd)
	AddPackage(packageId, packageVersion);
	}
	private static readonly (string packageId, string packageVersion)[] PackagesToAdd =
	{
	("Godot.NET.Sdk", GeneratedGodotNupkgsVersions.GodotNETSdk),
	("Godot.SourceGenerators", GeneratedGodotNupkgsVersions.GodotSourceGenerators),
	("GodotSharp", GeneratedGodotNupkgsVersions.GodotSharp),
	("GodotSharpEditor", GeneratedGodotNupkgsVersions.GodotSharp),
	};

It does indeed seem to require the files to exist. We don't check if the file exists, so AddPackageToFallbackFolder should throw FileNotFoundException.

This PR solves that as it get rids of all of that.

[neikeq]

(also it should be possible to remove the GodotSharp/Api folder as well, but just doing that breaks the editor)

No, we can't do that. That folder contains the C# loader (GodotPlugins) and API assemblies, both of which the Godot editor relies on.

[YuriSizov]

Thanks!