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

Exit MainLoop script with a custom error code #7557

Open
lonevox opened this issue Aug 25, 2023 · 12 comments
Open

Exit MainLoop script with a custom error code #7557

lonevox opened this issue Aug 25, 2023 · 12 comments
Labels
breaks compat Proposal will inevitably break compatibility topic:core

Comments

@lonevox
Copy link

lonevox commented Aug 25, 2023

Describe the project you are working on

I'm writing a C# game that requires a pre-build script to run to compile some assets, so I have added this to my .csproj file:

<Target Name="BuildPck" BeforeTargets="CoreCompile">                                                                   
    <Exec Command="%GODOT4% --display-driver headless -s BuildPck.gd" WorkingDirectory="$(ProjectDir)" />  
</Target>

I need to use a GDScript script for this, as I need access to PCKPacker as part of this build step. My script extends MainLoop instead of SceneTree, because SceneTree causes the script to repeatedly execute and not finish (I have no idea if this is a bug with msbuild or Godot, but it is a separate issue).

Describe the problem or limitation you are having in your project

Currently, returning true from MainLoop's _process() and _physics_process() functions exists the program with error code 1, causing my build to fail. It used to be possible to use OS.set_error_code() before exiting to change the exit code in Godot 3.1, but that was removed in Godot 3.2 with the addition of SceneTree.quit(0) where you could specify the exit code as an argument.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Instead of returning a boolean from MainLoop's _process() and _physics_process() functions, let them return an int instead which determines the error code.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Example MainLoop script:

extends MainLoop

func _process(delta):
    print("Happy console script :)")
    return 0

If this enhancement will not be used often, can it be worked around with a few lines of script?

This can't be worked around by anything in Godot.

In my case, I can ignore the error code by adding IgnoreExitCode="True" to my .csproj (but this isn't ideal):

<Target Name="BuildPck" BeforeTargets="CoreCompile">                                                                   
    <Exec Command="%GODOT4% --display-driver headless -s BuildPck.gd" WorkingDirectory="$(ProjectDir)" IgnoreExitCode="True" />  
</Target>

Is there a reason why this should be core and not an add-on in the asset library?

This is a very standard feature.

@dalexeev
Copy link
Member

dalexeev commented Aug 25, 2023

(Examples below are for 4.x; I haven't tested it in 3.x, but should work too.)

You can extend SceneTree instead of MainLoop, see "Command line tutorial - Running a script": 3.x docs, 4.x docs.

extends SceneTree

func _init() -> void:
    if error:
        printerr("Error message.")
        quit(1)
        return

    print("Success message.")
    quit(0)

If you don't like 3 lines of error output, then you can use void return:

extends SceneTree

func fail(message: String) -> void:
    printerr(message)
    quit(1)

func success(message: String) -> void:
    print(message)
    quit(0)

func _init() -> void:
    if error:
        return fail("Error message.")

    return success("Success message.")

@lonevox
Copy link
Author

lonevox commented Aug 25, 2023

I'm aware that you can extend SceneTree for this, but there seems to be a bug where Godot processes are repeatedly spawned forever and they never execute, which doesn't occur with MainLoop. I'm not sure how to describe that bug or what is happening there so I made this proposal, since it feels like exit code functionality should be a part of MainLoop anyway.

@dalexeev
Copy link
Member

dalexeev commented Aug 25, 2023

Currently, returning true from MainLoop's _process() and _physics_process() functions exists the program with error code 1, causing my build to fail.

Indeed:

// os.h
int _exit_code = EXIT_FAILURE; // unexpected exit is marked as failure

// scene_tree.cpp 
void SceneTree::quit(int p_exit_code) {
    _THREAD_SAFE_METHOD_

    OS::get_singleton()->set_exit_code(p_exit_code);
    _quit = true;
}

It used to be possible to use OS.set_error_code() before exiting to change the exit code in Godot 3.1, but that was removed in Godot 3.2

In 3.x there is OS.exit_code property. In 4.x, the exit code can only be set via SceneTree.quit().

Instead of returning a boolean from MainLoop's _process() and _physics_process() functions, let them return an int instead which determines the error code.

This breaks backward compatibility.

@dalexeev dalexeev added the breaks compat Proposal will inevitably break compatibility label Aug 25, 2023
@lonevox
Copy link
Author

lonevox commented Aug 25, 2023

How would one do an expected exit from MainLoop without this feature? It seems weird that the docs would encourage you to cause an "EXIT_FAILURE", based on the example here in which exiting is expected.

This breaks backward compatibility.

Definitely.

@dalexeev
Copy link
Member

It seems weird that the docs would encourage you to cause an "EXIT_FAILURE", based on the example here in which exiting is expected.

I agree that the current API looks weird since you can't set the exit code via MainLoop, only via SceneTree. But this change (EXIT_FAILURE by default) has a reason, see godotengine/godot#30207 and godotengine/godot#38929.

We probably could restore the OS.exit_code property (or only methods), but we need to keep two things in mind:

  1. When the engine crashes, there must be a non-zero exit code.
  2. Calling SceneTree.quit() (even without the argument) will overwrite the previously set exit code.

Or we can add a new API exclusively to the MainLoop, without adding confusing API to the OS class (which is used by more users). Or maybe break compatibility (highly undesirable) in the MainLoop API, since it is for advanced use.

@dalexeev
Copy link
Member

Perhaps we could do the following:

  1. restore OS.exit_code property (0 by default), like in 3.x;
  2. change SceneTree.quit() default argument value from 0 to -1, like in 3.x (optional, but makes more sense);
  3. in case of a crash OS.exit_code should be ignored and the process should exit with code 1?

I think that would solve all the problems at a small cost:

  • This will fix the MainLoop API (exit code 0 by default, can be changed via OS.exit_code if needed).
  • Changing the value of an optional argument is less compatibility break than changing the return type of a method. In addition, if the OS.exit_code setter has never been called in the project, then calling SceneTree.quit() without an argument will give the same result, since OS.exit_code is 0 by default,
  • I'm not familiar with operating system processes and I'm not sure if the crash handler is always called or if we should set an exit code before the crash (or if the process received SIGKILL?). But even in this case I think that we can use two variables: one for the effective exit code (1 by default, in case of a crash), and the second for OS.exit_code (0 by default, can be changed by user, used when the process is normally terminated).

CC @Calinou @touilleMan

@Calinou
Copy link
Member

Calinou commented Aug 25, 2023

I'm not familiar with operating system processes and I'm not sure if the crash handler is always called or if we should set an exit code before the crash (or if the process received SIGKILL?).

The crash handler is called regardless of the assigned exit code. Remember that exiting on an unclean signal such as SIGSEGV or SIGFPE will modify the process' exit code (128 + signal number).

@geekley
Copy link

geekley commented Nov 20, 2023

Method quit() should be in MainLoop too, not just SceneTree. SceneTree should just inherit it.
Alternatively, you could make quit(code) a static method that could be called from anywhere.

Specifying your exit code from a console program is such a basic feature!
Is there really no workaround without having to make the script a SceneTree with all overhead and whatnot?

@geekley
Copy link

geekley commented Nov 20, 2023

OK, so at least it looks like this iffy workaround works for MainLoop:

func quit(code: int) -> void:
	var s := SceneTree.new()
	s.quit(code)
	s.free()

But I really don't like it. quit(code) should just be available in MainLoop.

@Calinou
Copy link
Member

Calinou commented Nov 21, 2023

Readding the OS.exit_code property would restore 3.x behavior without having to break GDExtension compatibility by moving the SceneTree quit() method in the inheritance chain. It also allows for a global way to set exit code without having to pass variables around, which is sometimes desired. You sometimes have a large program where parts of the program should be able to set exit code for later, even if exiting the project actually happens in a completely unrelated location.

As for having exit_code in OS or MainLoop, I'd put it in the location where it's guaranteed to be unique. As I understand it, some people were looking to have a way to run multiple MainLoops at once?

in case of a crash OS.exit_code should be ignored and the process should exit with code 1?

Crashing should exit with the signal 128 + <signal number that caused the crash>. For instance, a SIGSEGV (segmentation fault) is number 11, so it would exit with code 139.

This appears to already be the case… mostly. Here, I'm using CRASH_NOW_MSG("test") in the project manager initialization:

$ bin/godot.linuxbsd.editor.x86_64                                                    
Godot Engine v4.2.rc.custom_build.fa259a77c - https://godotengine.org
OpenGL API 3.3.0 NVIDIA 535.129.03 - Compatibility - Using Device: NVIDIA - NVIDIA GeForce RTX 4090
 
ERROR: test
   at: ProjectManager (./editor/project_manager.cpp:2794)

================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.2.rc.custom_build (fa259a77cd9ea725f22ccfd52d5c228e10358e1d)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib64/libc.so.6(+0x3e9a0) [0x7f7c9437b9a0] (??:0)
[2] bin/godot.linuxbsd.editor.x86_64() [0x464f046] (/home/hugo/Documents/Git/godotengine/godot/./editor/project_manager.cpp:2794 (discriminator 2))
[3] bin/godot.linuxbsd.editor.x86_64() [0x286acd5] (/home/hugo/Documents/Git/godotengine/godot/main/main.cpp:3470 (discriminator 1))
[4] bin/godot.linuxbsd.editor.x86_64() [0x27c5169] (/home/hugo/Documents/Git/godotengine/godot/platform/linuxbsd/godot_linuxbsd.cpp:72 (discriminator 1))
[5] /lib64/libc.so.6(+0x2814a) [0x7f7c9436514a] (??:0)
[6] /lib64/libc.so.6(__libc_start_main+0x8b) [0x7f7c9436520b] (??:0)
[7] bin/godot.linuxbsd.editor.x86_64() [0x27c4f75] (??:?)
-- END OF BACKTRACE --
================================================================
[1]    10433 IOT instruction (core dumped)  bin/godot.linuxbsd.editor.x86_64

$ echo $?          
134

I'd expect it to be 132 in this case instead of 134, as 128 + 4 = 132. Am I missing something?

@akien-mga
Copy link
Member

I agree that there's a missing feature now to implement a MainLoop with a custom exit code.

Moving SceneTree::quit() to MainLoop as suggested may be an option. At least the assumption from godotengine/godot#38929 was that users would always quit with get_tree().quit(), forgetting that Godot can be used without SceneTree.

@dalexeev's proposal in #7557 (comment) also makes sense to me, and may be better than introducing a fairly high level quit() in MainLoop, when the rest of the functionality is very barebones (it's just calling virtual methods if they exist).

godotengine/godot#89229 evaluates going back to EXIT_SUCCESS exit code as the default, which mitigates the need for being able to set the exit code in the OP's use case, but this need would still exist for people who want their MainLoop script to actually report failure.


Crashing should exit with the signal 128 + <signal number that caused the crash>. For instance, a SIGSEGV (segmentation fault) is number 11, so it would exit with code 139.
...
I'd expect it to be 132 in this case instead of 134, as 128 + 4 = 132. Am I missing something?

Slight off topic, but I believe the explanation here is:

  • CRASH_NOW sends SIGILL (4) which is intercepted by the crash handler to dump the backtrace
  • Once done, the crash handler terminates by calling abort(), which means sending SIGABRT (6). That's why you get 10433 IOT instruction (core dumped), and an error code of 132.

@dbnicholson
Copy link

dbnicholson commented Nov 18, 2024

Perhaps we could do the following:

  1. restore OS.exit_code property (0 by default), like in 3.x;
  2. change SceneTree.quit() default argument value from 0 to -1, like in 3.x (optional, but makes more sense);
  3. in case of a crash OS.exit_code should be ignored and the process should exit with code 1?

I worked on 3 in godotengine/godot#99254. Technically that's about any detected errors and not crashes. It's guarded by a --fail-on-error CLI option.

While doing that I came to the same conclusion as 2. I.e., SceneTree.quit() should support and default to not setting an exit code by ignoring negative exit codes. With the current SceneTree.quit() API, you can't determine what the caller intended. Having it default to not setting the exit code would remove one ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks compat Proposal will inevitably break compatibility topic:core
Projects
None yet
Development

No branches or pull requests

6 participants