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

Headless builds: No support for error popups/dialogs #57016

Open
mhilbrunner opened this issue Jan 20, 2022 · 9 comments
Open

Headless builds: No support for error popups/dialogs #57016

mhilbrunner opened this issue Jan 20, 2022 · 9 comments

Comments

@mhilbrunner
Copy link
Member

mhilbrunner commented Jan 20, 2022

Things like EditorNode::immediate_confirmation_dialog:

bool EditorNode::immediate_confirmation_dialog(const String &p_text, const String &p_ok_text, const String &p_cancel_text) {

...currently don't handle headless mode at all, which is problematic as they are in some cases used for critical information, errors and even to confirm required editor restarts.

Currently, in headless mode, this just leads to that information being quietly supressed.

(One example is the popup for restarting/quitting the editor if new GDExtensions are detected: #53163)

@Calinou
Copy link
Member

Calinou commented Jan 20, 2022

Should a compile-time define be able to handle this by using ERR_PRINT() instead of displaying a message in the editor?

One open question is what to do when there are multiple choices possible. I think the dialog should always be confirmed in headless mode. This is likely done by pressing the get_ok() button by code.

@akien-mga
Copy link
Member

akien-mga commented Jan 20, 2022

Should a compile-time define be able to handle this by using ERR_PRINT() instead of displaying a message in the editor?

Headless builds are no longer separate builds in 4.0, it's just a --headless flag. So a compile-time define wouldn't be so nice, it should instead rely on something like Engine::get_singleton()->is_headless() (or an equivalent check via DisplayServer?).

@mhilbrunner
Copy link
Member Author

mhilbrunner commented Jan 20, 2022

One open question is what to do when there are multiple choices possible. I think the dialog should always be confirmed in headless mode. This is likely done by pressing the get_ok() button by code.

I wonder whether we should do that by default or exit with an error unless an additional CLI arg is given (--headless-confirm-all?) to avoid nasty surprises?

I.e. then the first time you run into this you are very obviously confronted with the decision to confirm all prompts by default (the error could even point out the CLI arg).

I guess we could also just do it and assume all users who use headless know what they are doing and at least have VCS enabled so possible destructive operations being confirmed by default isn't the end of the world, but... hm.

@fire
Copy link
Member

fire commented Jan 21, 2022

I am concerned about adding a different mode that needs to be tested.

Headless is a separate mode and gets less amounts of testing already.

@d6e
Copy link

d6e commented Jan 5, 2024

This issue bit me today.

I spent several hours trying to figure out how to accept a dialog prompt when running in headless mode in CI (otherwise it hangs indefinitely). Eventually I tracked it down to the .godot/extension_list.cfg being missing (adding it works around the dialog prompt issue). It would be nice if there were some sort of "accept-all" option to go with headless mode or if it were implicit in headless mode (there could be a logged message about the acceptance for traceability). Otherwise, every new godot dev is going to stumble into this pitfall.

@hello-adam
Copy link

hello-adam commented Jan 17, 2024

I think this is causing an issue with a --headless godot export I'm doing in a CI flow. The main thread seems to be blocked on this dialog:

(gdb) bt
#0  0x0000746881607385 in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=0x7ffcca642640, rem=0x7ffcca642650) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:48
#1  0x000074688160bc93 in __GI___nanosleep (req=<optimized out>, rem=<optimized out>) at ../sysdeps/unix/sysv/linux/nanosleep.c:25
#2  0x00005eb47b367f90 in OS_Unix::delay_usec (this=<optimized out>, p_usec=<optimized out>) at drivers/unix/os_unix.cpp:283
#3  0x00005eb47dd87a3e in OS::add_frame_delay (this=this@entry=0x7ffcca642c60, p_can_draw=<optimized out>) at core/os/os.cpp:591
#4  0x00005eb47a59a9a2 in Main::iteration () at main/main.cpp:3717
#5  0x00005eb47b616b88 in EditorNode::immediate_confirmation_dialog (p_text=..., p_ok_text=..., p_cancel_text=..., p_wrap_width=p_wrap_width@entry=0) at editor/editor_node.cpp:5589
#6  0x00005eb47b572ffe in EditorFileSystem::_update_scan_actions (this=this@entry=0x5eb481d81db0) at editor/editor_file_system.cpp:675
#7  0x00005eb47b574114 in EditorFileSystem::_notification (this=0x5eb481d81db0, p_what=<optimized out>) at editor/editor_file_system.cpp:1282
#8  0x00005eb47e2440a0 in Object::notification (this=0x5eb481d81db0, p_notification=17, p_reversed=<optimized out>) at core/object/object.cpp:837
#9  0x00005eb47c495bbf in SceneTree::_process_group (this=0x5eb481cabe70, p_group=0x5eb481cac0c8, p_physics=false) at scene/main/scene_tree.cpp:951
#10 0x00005eb47c4962c8 in SceneTree::_process (this=this@entry=0x5eb481cabe70, p_physics=p_physics@entry=false) at scene/main/scene_tree.cpp:1028
#11 0x00005eb47c4968c2 in SceneTree::process (this=0x5eb481cabe70, p_time=0.0055555555555555549) at scene/main/scene_tree.cpp:508
#12 0x00005eb47a59a5d5 in Main::iteration () at main/main.cpp:3634
#13 0x00005eb47a530131 in OS_LinuxBSD::run (this=this@entry=0x7ffcca642c60) at platform/linuxbsd/os_linuxbsd.cpp:933
#14 0x00005eb47a51f78b in main (argc=<optimized out>, argv=0x7ffcca643238) at platform/linuxbsd/godot_linuxbsd.cpp:74

Update:
looks like it's extension-related in this case as well. I'll see if I can do d6e's workaround in this case.

(gdb) info args
p_text = @0x7ffcca642840: {_cowdata = {_ptr = 0x5eb495d5e8e0 U"Some extensions need the editor to restart to take effect."}, static _null = 0 U'\000', static _replacement_char = 65533 U'\375\377\000\000'}
p_ok_text = @0x7ffcca642868: {_cowdata = {_ptr = 0x5eb4967acb80 U"Restart"}, static _null = 0 U'\000', static _replacement_char = 65533 U'\375\377\000\000'}
p_cancel_text = @0x7ffcca6428b0: {_cowdata = {_ptr = 0x5eb490b0c5e0 U"Continue"}, static _null = 0 U'\000', static _replacement_char = 65533 U'\375\377\000\000'}
p_wrap_width = 0

@pkdawson
Copy link
Contributor

The trouble with auto-confirming a restart prompt is that in a typical headless scenario like a CI script, there's now a new orphan editor process you have to find and wait for, which is pretty awkward (unless there's an easy solution I'm missing).

It might be better to simply exit with a certain error code rather than restarting when headless.

@Calinou
Copy link
Member

Calinou commented Jun 20, 2024

It might be better to simply exit with a certain error code rather than restarting when headless.

The issue is, we need to define an universal solution for all AcceptDialogs and ConfirmationDialogs. I don't see a way to provide more granular behavior that would allow simply exiting the editor (without restarting it) when there's a ConfirmationDialog that proposes quitting and restarting the editor.

@VrIgHtEr
Copy link

Agreed, currently I have to handle a fresh git clone of the project in the build script by running godot-mono --headless --import-only, waiting 60 seconds, sending it a SIGTERM, and writing a marker file (to check on subsequent runs to avoid doing this each time). Then the script carries out the export as usual. This will give the server enough time to "show" the confirmation dialog, at which point it has already stored the necessary information to start up properly next time, so killing it and restarting manually (even without clicking the dialog) works.

This is obviously not a desired workflow...

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

No branches or pull requests

8 participants