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

Improve OS.execute() (run a callback with the line contents every time a line is printed) #216

Closed
NHodgesVFX opened this issue Nov 9, 2019 · 25 comments · May be fixed by godotengine/godot#66830
Milestone

Comments

@NHodgesVFX
Copy link

Describe the project you are working on:
A application that calls a console executable to encode video

Describe the problem or limitation you are having in your project:

When using os.execute you can get the data back using blocking mode or non-blocking when using threads but only when the application is closed/completed. At least as far as I know there is no way to get the data line by line while the application is running.

Describe how this feature / enhancement will help you overcome this problem or limitation:

It would be great if in either blocking or non blocking for every line printed by the exe call a function with the new line as its printed in realtime.

this would make it super easy to call and check progress on external executables

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:
for example it should work like

os.execute("res://example.exe", ["-w", "example"], true, "callback")

func callback(new_line):
    #called every time a new line is printed by the called exe in real time
     print(new_line)

Describe implementation detail for your proposal (in code), if possible:

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

no this would need to be implemented in core as it not something that can be changed in gdscript

Is there a reason why this should be core and not an add-on in the asset library?:
as noted above this is already a core feature that would be enhanced

@aabmets
Copy link

aabmets commented Nov 11, 2019

Have you tried creating a TCP server on Godot and then OS.execute a client (python) script which can then talk with godot through the server instance? I tried it and it works.

EDIT: Though, if you're not used to socket programming, then this might be difficult for you.

@NHodgesVFX
Copy link
Author

I am not to familer with socket programming but I'm sure you could probably also run a simple http server on the python side then post to it from godot. the point of this proposal, is at the moment it requires alot of extra work and work arounds to make a usable solution to robustly call an outside executable.

Thanks for the suggestion, it inspired using a http server . Which should be a good work around till this is hopefully improved upon.

@nobuyukinyuu
Copy link

nobuyukinyuu commented Nov 12, 2019

For some reason TCP is preferred over standard pipes, but as someone who barely knows how to use C++, communication with console apps written in this language becomes less than trivial, requiring argument processing (in my case, 3rd party string functions to handle splitting and type conversion) or a sockets implementation on the backend.

I support proposals that amend OS.execute() to have arguments for non blocking communication via stdin/stdout.

@aabmets
Copy link

aabmets commented Nov 17, 2019

Okay so I took a look at how, if even, it can be implemented and I have to be honest here, I could not find where the OS.execute is defined in the source code. You might think "ok", its probably in "OS" -> "os.cpp" file, but you'd be wrong. The header file "os.h" has a definition of "execute", which is not defined in the os.cpp" file. If the source code would make any fucking sense, then MAYBE I could contribute something myself.

@NHodgesVFX
Copy link
Author

I think its defined per platform. I found the windows version here https://github.com/godotengine/godot/blob/master/platform/windows/os_windows.cpp

@bruvzg
Copy link
Member

bruvzg commented Nov 17, 2019

It's platform specific, plus there is additional wrapper for the gdscript.

  • core/bind/core_bind.cpp (gdscript interface for C++ implementation, with the different arguments!).
  • core/os/os.h virtual definition of the C++ function.
  • drivers/unix/os_unix.cpp generic Unix implementation, used on Linux, Android, Haiku, iOS and macOS.
  • platform\windows\os_windows.cpp implementation for Windows.
  • platform\javascript\os_javascript.cpp and platform\uwp\os_uwp.cpp dummy implementations, execute is not supported on these platforms.

@aabmets
Copy link

aabmets commented Nov 17, 2019

Okay, thanks. That's the code I was looking for. Now that I've realized that I can't understand a single line of that gibberish, I'm dropping any attempt to improve Godot by myself.

@Calinou Calinou changed the title Improve OS.execute() Improve OS.execute() (run a callback with the line contents every time a line is printed) May 3, 2020
@neikeq
Copy link

neikeq commented Jun 9, 2020

There's godotengine/godot#8310 if any one wants to retake it.

@silverkorn
Copy link

For some reason TCP is preferred over standard pipes, but as someone who barely knows how to use C++, communication with console apps written in this language becomes less than trivial, requiring argument processing (in my case, 3rd party string functions to handle splitting and type conversion) or a sockets implementation on the backend.

I support proposals that amend OS.execute() to have arguments for non blocking communication via stdin/stdout.

+ stderr, and if possible, custom file descriptors for some kind of parallelism or message compartmentalization.

But at least stdin (write), stdout (read) & stderr (read) would be the must and it's pretty much the standard everywhere.

@quentincaffeino
Copy link

I would also love to see stdin/out/err support for os.execute.

Also would be nice allowing to read stdin of godot process.

@silverkorn
Copy link

As potential references/libraries for this feature request:

@nathanfranke
Copy link

I don't like the idea of adding a ton of unreadable parameters to OS.execute(). I think a process class is a much better idea (See godotengine/godot#8310).

@LinuxUserGD
Copy link

Workaround for printing a character every second while command is running:

func run_command() -> void:
    var thread = Thread.new()
    thread.start(_build)
    while(thread.is_alive()):
        OS.delay_msec(1000)
        printraw(".")
    thread.wait_to_finish()

func _build():
    var outArray: Array = []
    OS.execute('bash',['build'],outArray,true,false)

@lyuma
Copy link

lyuma commented Dec 8, 2023

Ran across this forum thread when trying to figure out how to make one of my slow OS.execute calls async.

The create_process function seems like it is entirely missing the ability to pass an input or output file, and execute does not give sufficiently fine-grained control over I/O. Specifically, it only permits reading output, not writing input; and there is no way to isolate output from stderr, just a boolean flag to entirely disable stderr.

I would recommend looking some at the python subprocess APIs which from personal experience are very nice to use and have most platform specific concerns abstracted away.

Godot's "execute" and "create_process" calls are very close to what we need, just missing a few features.
Should a separate proposal be opened for adding I/O callbacks / file descriptors for create_process?

@Calinou
Copy link
Member

Calinou commented Dec 8, 2023

Should a separate proposal be opened for adding I/O callbacks / file descriptors for create_process?

Yes, since this is quite different from what's originally asked here.

@silverkorn
Copy link

silverkorn commented Jan 15, 2024

@lyuma , there's a pending PR using Tiny Process Librairy (godotengine/godot#66830)
You might want to follow and/or link your proposal with it.

@time-killer-games
Copy link

I would like to add another approach instead of reading line-for-line would be to read all output but add an additional argument for limiting the buffer to a certain amount of characters to prevent buffer overflow, and this will allow more than a single line to be read at a time, if the buffer limit happens to be greater than the most recent line length.

Additionally, being able to write to standard input of the child process or to get the process id of the child would be other useful additions.

@time-killer-games
Copy link

time-killer-games commented Jan 16, 2024

It's platform specific, plus there is additional wrapper for the gdscript.

  • core/bind/core_bind.cpp (gdscript interface for C++ implementation, with the different arguments!).
  • core/os/os.h virtual definition of the C++ function.
  • drivers/unix/os_unix.cpp generic Unix implementation, used on Linux, Android, Haiku, iOS and macOS.
  • platform\windows\os_windows.cpp implementation for Windows.
  • platform\javascript\os_javascript.cpp and platform\uwp\os_uwp.cpp dummy implementations, execute is not supported on these platforms.

I would like to point out according to Microsoft Documentation, UWP supports CreateProcess() and CreatePipe(), (among many other things in that platform\windows\os_windows.cpp file), so it sounds more or less like the way this was implemented on Windows Desktop uses things which are specific to Windows Desktop and can't be used on UWP, but it can still technically be tweaked to get it working in UWP, unless I'm sadly mistaken. Just no one took the time to do it.

@bruvzg
Copy link
Member

bruvzg commented Jan 16, 2024

and can't be used on UWP, but it can still technically be tweaked to get it working in UWP

UWP support was dropped in 4.x, so it doesn't matter what is supported on this platform.

I would like to add another approach instead of reading line-for-line would be to read all output but add an additional argument for limiting the buffer to a certain amount of characters to prevent buffer overflow, and this will allow more than a single line to be read at a time, if the buffer limit happens to be greater than the most recent line length.

Most standard way of doing in is probably File like pipe interface to allow reading and writing on demand, without any arbitrary line/buffer size restrictions.

@time-killer-games
Copy link

I didn't know UWP was dropped; that makes sense then I guess.

Wouldn't what you suggest with on demand read/write be a risk for buffer overflow? That's why I mentioned it being optional to have a buffer limit for apps that need it.

@bruvzg
Copy link
Member

bruvzg commented Jan 16, 2024

Wouldn't what you suggest with on demand read/write be a risk for buffer overflow? That's why I mentioned it being optional to have a buffer limit for apps that need it.

No. Usually, a pipe like this is blocking (I think on Windows it's the only supported option), so if the internal pipe buffer is full, the app will be waiting for the other end to take some data out.

@time-killer-games
Copy link

time-killer-games commented Jan 16, 2024

@bruvzg I think I must be misunderstanding you somewhere right now, because I wrote my own process library, and Unix-likes and Windows alike it is not blocking, though I am using multi-threading to make it not block, (so I guess you mean blocks current thread? and not all threads? You said blocking is the only option on Windows which makes it sound like all threads will be blocked and I know this is not so, so maybe you mean the former and not the latter?) Just want to make sure I am understanding you correctly. Of course it blocks the calling thread on Windows, but it shouldn't block outside threads. Here's the library I wrote in case you were wondering: https://github.com/time-killer-games/xproc

I would like to help contribute to the OS.execute() overhaul, would create pull request. But it seems there are several users already doing that, and we only need one implementation and not several.

@bruvzg
Copy link
Member

bruvzg commented Jan 16, 2024

so I guess you mean blocks current thread?

Only output write calls (in the app) and pipe read calls (in the engine) are blocking. It's not blocking other threads or process in general (if it's not outputting anything).

@KoBeWi
Copy link
Member

KoBeWi commented Apr 7, 2024

With godotengine/godot#89206, I think this is resolved. I was able to use the new pipe API to achieve what is requested in this proposal. A callback can be implemented manually. Since I already have a nice wrapper, I could make it into an addon.

@Calinou
Copy link
Member

Calinou commented Apr 8, 2024

Equivalent functionality was implemented by godotengine/godot#89206, closing.

@Calinou Calinou closed this as completed Apr 8, 2024
@Calinou Calinou added this to the 4.3 milestone Apr 8, 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.