godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.23k stars 21.22k forks source link

When building projects, Godot doesnt consistently use the same dotnet executable from c++ and the c#, specifically on mac #97501

Open zynga-jpetersen opened 1 month ago

zynga-jpetersen commented 1 month ago

Tested versions

Looks to be reproducible back to 4.0 based on the commits. I repro'ed it in 4.3

System information

Godot v4.3.stable.mono - macOS 14.5.0 - Vulkan (Mobile) - integrated Apple M2 Max - Apple M2 Max (12 Threads)

Issue description

Dotnet isn't generally required to be in a specific location, it has various mechanism to allow other projects/executables to find it, through environment variables, text file redirects and other methods. See https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-environment-variables#dotnet_root-dotnet_rootx86-dotnet_root_x86-dotnet_root_x64

But Godot has 2 different methods of looking up the dotnet executable, in c# and from the c++ and they don't match.

C# is only looking for the executable in 2 locations: "/usr/local/share/dotnet/dotnet"; and "/usr/local/share/dotnet/x64/dotnet"

The c++ is correctly using the common c# environment variables to find the right version, first checking environment variables like DOTNET_ROOT_ARM64 and then DOTNET_ROOT (it could maybe be slightly better by searching the path): https://github.com/godotengine/godot/blob/master/modules/mono/editor/hostfxr_resolver.cpp#L335

In my tests, I was hoping that DOTNET_HOST_PATH would have been set by the pipeline out of the box, but its not at that code location in DotNetFinder.cs.

I think the solution would end up looking maybe like this (but maybe there is a real library function we could be calling): https://github.com/dotnet/roslyn/blob/main/src/Compilers/Shared/RuntimeHostInfo.cs#L59

Steps to reproduce

On mac, install dotnet or the sdks into a non-standard location (like say maybe you might do on a build machine).

Then set DOTNET_ROOT to point at that location as the dotnet documentation suggests: https://learn.microsoft.com/en-us/dotnet/core/install/macos#set-environment-variables-system-wide

Then build for Android and the c# will not be able to find the right dotnet executable.

Minimal reproduction project (MRP)

Since the bug comes from the dotnet installation location variations, not the project, any project will show the problem.

raulsntos commented 1 month ago

According to the documentation you linked:

This is used by the .NET SDK to help tools that run during .NET SDK commands ensure they use the same dotnet runtime for any child dotnet processes they create for the duration of the command.

[!NOTE] DOTNET_HOST_PATH is not a general solution for locating the dotnet host. It is only intended to be used by tools that are invoked by the .NET SDK.

The Godot editor is not invoked by the .NET SDK, so it doesn't apply. The DOTNET_HOST_PATH environment variable is set by the .NET SDK, so since Godot is not running under the .NET SDK it won't be set.

Also, specifically for macOS, as I understand it app bundles aren't able to read environment variables so we may not be able to read DOTNET_HOST_PATH anyway.

zynga-jpetersen commented 1 month ago

Yeah, it makes sense that DOTNET_HOST_PATH is not currently set in the path, especially since reading the source code more in dotnet, there are several places in their toolchain where they are explicitly setting it before making calls to the tools. I wasnt sure if that was the right direction or matching the RuntimeHostInfo code from roslyn.

Some examples: https://github.com/dotnet/msbuild/blob/main/eng/cibuild_bootstrapped_msbuild.sh#L80 https://github.com/dotnet/sdk/blob/0dbf5441b8b741e610103e2a57525fad740b410e/src/Cli/dotnet/commands/dotnet-nuget/Program.cs#L44

So we might be able to do something similar when we launch into c# so we are passing along the same host as that environment variable and not re-looking it up (or look it up from that environment variable first).

As for "app bundles aren't able to read environment variables". I've locally changed FindDotNetExe to print the environment variables and I do see it printing.

            Console.WriteLine($"FindDotNetExe -- All Environment Variables");
            foreach(DictionaryEntry e in Environment.GetEnvironmentVariables())
            {
                Console.WriteLine(e.Key  + ":" + e.Value);
            }

So I believe I could change it to match this function, or at least take the environment variable lookups its doing and then fall into the case that this change was originally meant to handle.

zynga-jpetersen commented 1 month ago

Another alternative, is that I could create another environment variable called GODOT_DOTNET_EXECUTABLE and access it in both and in that way ensure they are consistent?

https://github.com/godotengine/godot/blob/master/modules/mono/editor/hostfxr_resolver.cpp#L335 and https://github.com/godotengine/godot/blob/master/modules/mono/editor/GodotTools/GodotTools/Build/DotNetFinder.cs#L14

raulsntos commented 1 month ago

As for "app bundles aren't able to read environment variables". I've locally changed FindDotNetExe to print the environment variables and I do see it printing.

But are you building an app bundle or just an executable?


Anyway, if we were to add a way to set the path to the dotnet executable, I don't think we would do it through an environment variable. I don't think Godot uses environment variables for any configuration, usually we use EditorSettings.

But first, I'd like to know more about your use case. Why do you need to specify the path to a dotnet executable different from the one in PATH? And why can't you modify PATH to point to the right dotnet executable?

This sounds more like a proposal than a bug report, there's a similar proposal that may already fulfill your use case:

zynga-jpetersen commented 1 month ago

First let me say, thanks for the reply.

As for "app bundles aren't able to read environment variables". I've locally changed FindDotNetExe to print the environment variables and I do see it printing.

But are you building an app bundle or just an executable?

Reading more tickets, I'm probably able to access the environment variables because I am compiling the code myself, so I'm probably not seeing the sandboxed problem that some other tickets have described. Though this might be OS version issue as well that I'm just not triggering.

But first, I'd like to know more about your use case. Why do you need to specify the path to a dotnet executable different from the one in PATH? And why can't you modify PATH to point to the right dotnet executable?

I do have a dotnet in my PATH, but DotnetFinder.cs is not checking the PATH, instead its looking for specific files instead of relying on the PATH first: https://github.com/godotengine/godot/blob/master/modules/mono/editor/GodotTools/GodotTools/Build/DotNetFinder.cs#L19

Which doesnt match the behavior in the c++ where first environment variables are checked first: https://github.com/godotengine/godot/blob/master/modules/mono/editor/hostfxr_resolver.cpp#L335

I've written a working solution adding support for DOTNET_ROOT in DotnetFinder, so the c++ and c# are implemented more closely together (though not identical since I don't have a method of testing all the different architecture versions of DOTNETROOT{arch}). I'm hoping to make a PR request later today.

As for the referenced proposal:

I could additional checks for it to the EditorSettings, if that is more likely to get through the approval process. My concerns about that solution though is that it doesn't closely match the current behavior, does not match C# tool conventions, and was proposed in 2020 and not blessed as an "good" idea then?

zynga-jpetersen commented 1 month ago

I'll write the settings version as well for completeness since given the environment variable issue with mac and sandboxed editors, it would be good for them to have a consistent method of overriding it.

I'm calling the editor setting "dotnet/build/override_dotnet_executable", let me know if you would prefer a different name.

The editor settings werent initialized in time, so I lazy initializing the Editor Settings like they do in EditorNode, so if it not there, we create it.

My setting changes look like:

const StringName get_dotnet_editor_settings_name() { return StringName("dotnet/build/override_dotnet_executable"); }

bool get_dotnet_exe_from_settings(String &r_dotnet_exe) { const EditorSettings* editor_settings = EditorSettings::get_singleton(); if (editor_settings == nullptr) { // load editor settings. EditorSettings::create(); editor_settings = EditorSettings::get_singleton(); if (editor_settings == nullptr) { return false; } }

const StringName editor_settings_name = get_dotnet_editor_settings_name(); bool valid = false; // Use EDITOR_DEF to default it to "", if not found. const Variant property_val = EDITOR_DEF(editor_settings_name, ""); if (property_val.is_string() == false) { if (OS::get_singleton()->is_stdout_verbose()) { ERR_PRINT("The editor setting (" + editor_settings_name + ") is not referencing a valid string path."); } return false; }

String dotnet_exe = property_val; if (dotnet_exe.is_empty()) { return false; }

r_dotnet_exe = path::abspath(path::realpath(dotnet_exe));

return true; }

bool get_dotnet_root_from_settings(String &r_dotnet_root) { String dotnet_exe;

if (!get_dotnet_exe_from_settings(dotnet_exe)) { return false; }

r_dotnet_root = dotnet_exe.get_base_dir();

if (!DirAccess::exists(r_dotnet_root)) { if (OS::get_singleton()->is_stdout_verbose()) { ERR_PRINT("The editor setting (" + get_dotnet_editor_settings_name() + ") is not referencing a valid string path (" + r_dotnet_root + ")."); }

  return false;

}

return true; }

Then called from

bool godotsharp::hostfxr_resolver::try_get_path(String &r_dotnet_root, String &r_fxr_path) { if (!get_dotnet_root_from_settings(r_dotnet_root) && !get_dotnet_root_from_env(r_dotnet_root) && !get_dotnet_self_registered_dir(r_dotnet_root) && !get_default_installation_dir(r_dotnet_root)) { return false; }

  return try_get_path_from_dotnet_root(r_dotnet_root, r_fxr_path);

}

zynga-jpetersen commented 1 month ago

Sorry for the delay. I opened a PR to fix this https://github.com/godotengine/godot/pull/98073. let me know if you have feedback. Thanks