godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 96 forks source link

OS.execute output stdout and stderr seperately. #8543

Closed PastMoments closed 6 months ago

PastMoments commented 10 months ago

Describe the project you are working on

A game where we call certain OS commands to get system info for debug purposes.

Describe the problem or limitation you are having in your project

As mentioned above, we execute certain OS commands to obtain hardware info for our game. We then process the standard out, and standard error separately before handling it. However the engine provided OS.execute does not allow us to do this, as stdout and stderr is just combined together.

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

Allowing us to seperate stdout and stderr would solve these issues.

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

I'm proposing to change the signature from int execute (String path, PoolStringArray arguments, bool blocking=true, Array output=[ ], bool read_stderr=false, bool open_console=false) to int execute (String path, PoolStringArray arguments, bool blocking=true, Array output=[ ], Array error=[ ], bool open_console=false)

Users that wish to have the previous behaviour of combining the stdout and stderr could provide the same argument into output and error. I understand that this function has a great deal of complexity already, so I'm hoping this proposal would be able to not add yet another argument. I'm not sure if this proposal will be able to maintain backwards compatibility this way, though.

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

There's some potential OS-specific workarounds involving temporary files and piping things around, but that then introduces some issues with different environments handling things differently. Otherwise can't be fixed by gdscript.

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

We have a GDNative extension (and in the future a GDExtension) to replace OS.execute for this specific use case. However adding an entire C build dependency for this minor use case is quite annoying, and can introduce instability in our project.

Calinou commented 10 months ago

As mentioned above, we execute certain OS commands to obtain hardware info for our game.

Out of curiosity, which hardware information are you gathering this way? Maybe it's available using built-in methods.

I'm not sure if this proposal will be able to maintain backwards compatibility this way, though.

This can be made backwards-compatible by adding two parameters at the end:

Alternatively, we can add a separate method OS.execute_split_stderr().

mcmacker4 commented 6 months ago

I just encountered the same problem. My use case is I want to execute ffprobe like so:

ffprobe -loglevel error -print_format json -show_format -show_streams <some random file path>

This command will always spit out a JSON object into STDOUT, and will log any errors encountered into STDERR. This is standard for most CLI tools. When specifying read_stderr=true Godot will join those two outputs into a single String, forcing the programmer to find an ugly way to split the string somehow. It would've been more convenient to have two different strings for STDOUT and STDERR.

This can be made backwards-compatible by adding two parameters at the end:

  • bool split_stderr = false
  • Array error=[] (return value), only filled if split_stderr is true

You could even reuse the output array. If split_stderr=true is specified, STDOUT and STDERR are split into two different Strings and both Strings are appended to the output array like so:

Most OS already treat these two as different streams, allowing you to pipe them separately. I'm not sure why it was decided to join them into a single string.

@AThousandShips Are we sure that "This proposal will inevitably break compatibility" as specified by the https://github.com/godotengine/godot-proposals/labels/breaks%20compat label?

AThousandShips commented 6 months ago

Yes, changing a function signature always breaks compatibility

mcmacker4 commented 6 months ago

After some investigation, I believe that this proposal may have been solved in commit https://github.com/godotengine/godot/commit/082b420c0ab6ddb7cc658b929124893ee2ad72b2 with the introduction of OS.execute_with_pipe()

Documentation is available at docs.godotengine.org in the latest revision

Calinou commented 6 months ago

Equivalent functionality was implemented by https://github.com/godotengine/godot/pull/89206, closing.