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

godot4 --headless --export-release should output a non-zero exit_code on errors #83042

Closed
superherointj opened this issue Oct 9, 2023 · 10 comments · Fixed by #89234
Closed

Comments

@superherointj
Copy link

superherointj commented Oct 9, 2023

Godot version

4.1.2

System information

Godot v4.1.2.stable (399c9dc) - NixOS 23.11 (Tapir) - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3090 (nvidia; 535.113.01) - AMD Ryzen 9 5950X 16-Core Processor (32 Threads)

Issue description

On --export-release failing for any reason, godot4 application should output a non-zero exit_code.

It causes CI to not block pipelines.

Desired behavior: In the presence of errors in logs, Godot process should return a non-zero exit signal.

Steps to reproduce

Start any new project. Just run:

godot4 --headless --export-release "Linux/X11" junk/

Error output:

Godot Engine v4.1.2.stable.nixpkgs.399c9dc39 - https://godotengine.org

WARNING: Custom cursor shape not supported by this display server.
at: cursor_set_custom_image (servers/display_server.cpp:480)
ERROR: This project doesn't have an export_presets.cfg file at its root.
Create an export preset from the "Project > Export" dialog and try again.
at: _fs_changed (editor/editor_node.cpp:1003)

Then, check for signal error:

echo $

0

Minimal reproduction project

This error replicates with any new empty project. (So it is not necessary to provide a zip!)
I'm including a repo and a zip. But it is pointless. You can start a new project and works the same.

https://github.com/superherointj/godot-error-signal

empty-project.zip

@AThousandShips
Copy link
Member

On --export-release failing for any reason, godot4 application should output a non-zero error number.

Is this documented as a feature that's guaranteed?

@superherointj
Copy link
Author

superherointj commented Oct 9, 2023

Is this documented as a feature that's guaranteed?

This is a OS standard behavior for applications. All applications should comply. It's pretty basic. I think of this being a bug and not a feature. All applications return an error number. But godot is not returning a non-zero error number for failure. Failure in Godot currently is silent. Which breaks anything that relies on the exit signal. Such as CI, build scripts, shell pipelines etc.

@AThousandShips
Copy link
Member

There are plenty of ways to respond to a particular event, that's what needs to be discussed and how to approach it

To confirm, the exporting here completely fails yes? No results are exported?

@superherointj superherointj changed the title godot4 --headless --export-release does not output a non-zero error number in Linux on errors godot4 --headless --export-release should not output a zero signal on errors Oct 9, 2023
@superherointj
Copy link
Author

Sorry if I'm being repetitive. Communication is hard.

There are plenty of ways to respond to a particular event, that's what needs to be discussed and how to approach it

Actually there is only one way to do process signalling because it's standardized behavior for all processes. Processes should signal a non-zero error when execution of application has failed. As Godot logs have errors, it should signal a non-zero error. This is standard behavior for process communication.

To confirm, the exporting here completely fails yes? No results are exported?

The issue is not the output of export but the process signalling of export when there are errors.
Still, as there are errors, I'd expect export artifacts to be deemed invalid or irrelevant. [Even if they are not.]
This behavior I'm pointing out should be valid for any output that has errors. In the presence of any error output, Godot should signal a non-zero error number as process signal exit number.

On Godot implementation side. Would be valid to assume that anytime any error has been logged, Godot process should return a non-zero number. Then, there is the question of which one to output. [I'm trying to clarify the number atm, but regardless of the specific number, they key aspect is to output non-zero.]

@AThousandShips I'd prefer having this issue labeled as bug and not enhancement. Would you be okay with changing that? Thanks!

@AThousandShips
Copy link
Member

AThousandShips commented Oct 9, 2023

I'll let others make the judgement on that, I don't consider this a bug but a missing feature, but that's just my view, and changing the return value does change the behavior so that means that the distinction between bug/enhancement matters

@superherointj superherointj changed the title godot4 --headless --export-release should not output a zero signal on errors godot4 --headless --export-release should output a non-zero signal on errors Oct 9, 2023
@superherointj superherointj changed the title godot4 --headless --export-release should output a non-zero signal on errors godot4 --headless --export-release should output a non-zero exit_code on errors Oct 9, 2023
@Calinou
Copy link
Member

Calinou commented Oct 9, 2023

This can likely be implemented by using OS::get_singleton()->set_exit_code(EXIT_FAILURE); just before an export-related error is printed (you'll need to modify individual instances where these messages are printed).

Would be valid to assume that anytime any error has been logged, Godot process should return a non-zero number.

Godot regularly prints errors due to internal issues (such as engine bugs or user scripts doing something invalid), so I don't think this makes sense. Non-zero exit code should only be returned if the main process failed in some way (such as exporting the project in your case).

[I'm trying to clarify the number atm, but regardless of the specific number, they key aspect is to output non-zero.]

I'd just exit with 1, which is the value of the EXIT_FAILURE C constant.

@kxn
Copy link

kxn commented Oct 11, 2023

+1 for this one. Call it a bug or a feature, whatever is ok. it is badly needed by projects depending on CI workflows. There is no way to find errors that prevent building currently. If other people need current exit code behaviour and changing may break them, please at least provide something like --return-non-zero-on-any-errors.

Thanks

@m21-cerutti
Copy link

This is clearly a bug, as it prevents CI systems from operating reliably.

Seems exit code is very random, I got error code -1073741819 when I export with a workaround to this error #77478, where I add global_script_class_cache.cfg to non ignored files. Make export work if we ignored this weird error coe.

image

On second run however no code error anymore ^^'.
image

It's really painfull we can't rely on exit code.

@superherointj
Copy link
Author

superherointj commented Feb 16, 2024

To export a Godot project in CI, the workaround I have been using to check for errors is:

    # 'command' is required to forward stderr to stdin.
    # 'unbuffer' is required to keep colors in logs.
    command 2>&1 ${expect}/bin/unbuffer godot4 --verbose --headless --export-release "Linux/X11" $out/bin/project_name | tee $TMPDIR/godot.logs

    # Manually check for errors
    [ `grep "ERROR:" $TMPDIR/godot.logs | wc -l` -eq 0 ]

I happen to use Nix to do the build.

  • $out/bin/ is from Nix. You don't need this part.
  • ${expect} is the package name from nixpkgs. Replace it with unbuffer in your host OS.

The idea is straightforward. Just adapt it.

@akien-mga
Copy link
Member

#89234 should fix this, turns out it was (likely) a misleading assumption that a function named _exit_editor would abort right away and not continue running the code that makes it exit with a success code.

akien-mga added a commit to akien-mga/godot that referenced this issue Mar 11, 2024
akien-mga added a commit to akien-mga/godot that referenced this issue Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants