Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Fix editor crashing without a message when .NET is not installed #73815

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

RedworkDE
Copy link
Member

@RedworkDE RedworkDE commented Feb 23, 2023

This only fixes the without a message part, the editor will still crash.

TODO: Verify that OS::alert() is either blocking or the message survives a crash on all supported platforms.

@RedworkDE RedworkDE requested a review from a team as a code owner February 23, 2023 11:20
@akien-mga akien-mga added this to the 4.0 milestone Feb 23, 2023
@akien-mga
Copy link
Member

akien-mga commented Feb 23, 2023

It would be great if we could exit gracefully instead of crashing, but I agree that as a short term improvement this is better than nothing (and exiting gracefully from an arbitrary place in the codebase is not trivial).

I can test on Linux in a minute.

@RedworkDE
Copy link
Member Author

A somewhat graceful exit was the plan when I originally wrote this for #72333, but there is no OS::exit, and in the end it doesn't really make a difference for the user, so I just went for a crash instead.

I am currently checking on macOS, but my Ubuntu doesn't feel like starting atm, so you checking that would be great.

@akien-mga
Copy link
Member

akien-mga commented Feb 23, 2023

Doesn't seem to work for me on Linux (Mageia 9, KDE Plasma on KWin/X11).

The project manager runs fine and the console gets those errors:

ERROR: The host fxr folder does not exist: /usr/share/dotnet/host/fxr
   at: try_get_path_from_dotnet_root (modules/mono/editor/hostfxr_resolver.cpp:323)
ERROR: .NET: One of the dependent libraries is missing. Typically when the `hostfxr`, `hostpolicy` or `coreclr` dynamic libraries are not present in the expected locations.
   at: find_hostfxr (modules/mono/mono_gd/gd_mono.cpp:126)
ERROR: .NET: Failed to load hostfxr
   at: initialize (modules/mono/mono_gd/gd_mono.cpp:402)

Then trying to edit an existing C# project, I get a crash with no OS dialog, and the same errors in the console:

ERROR: The host fxr folder does not exist: /usr/share/dotnet/host/fxr
   at: try_get_path_from_dotnet_root (modules/mono/editor/hostfxr_resolver.cpp:323)
ERROR: .NET: One of the dependent libraries is missing. Typically when the `hostfxr`, `hostpolicy` or `coreclr` dynamic libraries are not present in the expected locations.
   at: find_hostfxr (modules/mono/mono_gd/gd_mono.cpp:126)
ERROR: .NET: Failed to load hostfxr
   at: initialize (modules/mono/mono_gd/gd_mono.cpp:402)

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.0.rc.mono.custom_build (f485bf46ce397ec205e69d2002c3505dc4398041)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib64/libc.so.6(+0x36940) [0x7f517ffc6940] (??:0)
-- END OF BACKTRACE --
================================================================

I don't seem to hit the condition you're modifying, it crashes earlier on. It should probably raise an error when load_hostfxr() fails too.

Since the project manager seems to be doing those checks too, but without crashing, maybe it could already show an error dialog (from Godot itself, not necessarily OS.alert()) with information about what's missing.

But for users editing a project directly without going through the project manager we'd still want to at least show the OS.alert() dialog.

@RedworkDE
Copy link
Member Author

Yeah, no sure how this can work on windows, but on mac that condition can't be hit either, so back to the drawing board.

@RedworkDE RedworkDE marked this pull request as draft February 23, 2023 12:10
@akien-mga
Copy link
Member

akien-mga commented Feb 23, 2023

This works for me on Linux:

diff --git a/modules/mono/mono_gd/gd_mono.cpp b/modules/mono/mono_gd/gd_mono.cpp
index e277d887fd..27c21693d5 100644
--- a/modules/mono/mono_gd/gd_mono.cpp
+++ b/modules/mono/mono_gd/gd_mono.cpp
@@ -399,6 +399,9 @@ void GDMono::initialize() {
 			ERR_FAIL_MSG(".NET: Failed to load hostfxr");
 		}
 #else
+
+		// Show a message box to the user to make the problem explicit (and explain a potential crash).
+		OS::get_singleton()->alert(TTR("Unable to load .NET runtime, specifically hostfxr.\nAttempting to create/edit a project will lead to a crash.\n\nPlease install the .NET SDK 6.0 or later from https://dotnet.microsoft.com/en-us/download and restart Godot."), TTR("Failed to load .NET runtime"));
 		ERR_FAIL_MSG(".NET: Failed to load hostfxr");
 #endif
 	}

This gets shown when opening the project manager, so I didn't make it crash, but pointed out in the error that it would if the user attempts to create or edit a project. So they get to close the dialog, and then close the project manager.

image

(don't mind the garbage in the window, the dialog is shown so early that the Project Manager hasn't finished rendering)

@RedworkDE RedworkDE force-pushed the net-missing-editor-message branch from 276a86f to 3b3e3b6 Compare February 23, 2023 12:32
@RedworkDE
Copy link
Member Author

Found the same point as well, it works on macOS and windows as well.

@akien-mga
Copy link
Member

WDYT of my improved message? I think it's good to give clear expectations for what might happen once they click "ok" :)

@RedworkDE RedworkDE force-pushed the net-missing-editor-message branch from 3b3e3b6 to 30d0f34 Compare February 23, 2023 12:38
@akien-mga
Copy link
Member

I think it's ready for review now? I'd like to merge it now for 4.0 RC 4 :)

@RedworkDE RedworkDE force-pushed the net-missing-editor-message branch from 30d0f34 to 6b050a3 Compare February 23, 2023 12:53
@RedworkDE RedworkDE marked this pull request as ready for review February 23, 2023 12:53
@RedworkDE
Copy link
Member Author

I added the same message (without the crash_now) to the original location as well. because I wasn't completely confused when I put it there originally. It is necessary there, if some .net is installed but it is too old.

@RedworkDE
Copy link
Member Author

Also for completeness: On mac and windows the splash screen is rendered correctly before the message box pops up, (which is blocking on both) than when you confirm it, either the project manager comes up normally or the editor crashes when editing a project.

@akien-mga akien-mga merged commit e0de357 into godotengine:master Feb 23, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants