pygobject / pgi-docgen

API Documentation Generator for PyGObject
https://lazka.github.io/pgi-docs/
GNU Lesser General Public License v2.1
127 stars 36 forks source link

GLib.spawn_async documentation is for the C api not the static implementation from pygobject #137

Closed infirit closed 7 years ago

infirit commented 7 years ago

From https://mail.gnome.org/archives/python-hackers-list/2017-January/msg00001.html

There appears to be a big difference between what the pygi-docs [1] documentation says how these should word compared to how they actually work. Basically as I see it, it works more like the old pygobject version [2], actually exactly like that. For example it return 4 values [3] while the current docs follow the C api and returns 2 [4].

This confused the hell out of me until I looked for some examples on the internet. I found in the GLib override that g_spawn_async is reassigned with gi._gi._glib.spawn_async which is my guess the reason it is t is different.

I since learned this is one few remaining static bindings in pygobject, see gnome bug 735213

edit: Did a pure python reimplementation using GLib.spawn_async_with_pipes [1]. Maybe useful for some ppl.

[1] https://gist.github.com/infirit/42bffcaf7f707b7d8c2b0c661fe971e7

lazka commented 7 years ago

Thanks

lazka commented 7 years ago

What about now? https://lazka.github.io/pgi-docs/GLib-2.0/functions.html#GLib.spawn_async

infirit commented 7 years ago

Looks good to me 👍

lazka commented 7 years ago

Thanks. If you find anything else please file a bug.

https://github.com/pygobject/pgi/commit/20145a45238633670effa774e1317c4a4c82592b

lazka commented 7 years ago

Regarding your Python implementation:

infirit commented 7 years ago

If you find anything else please file a bug.

Will do, thanks for the quick fix 👍

In case standard_output is False it still gets redirected so it's not the same as the C implementation

I have a hard time finding where this is different in the C module here. To me the code in the C module does nothing to change this, but this may be my lack of C knowledge 😄.

The returned fds don't get closed in case they don't get returned

I am not supposed to and looking at the C module I don't see where this is done. If I read the docs properly the file descriptors from GLib.spawn_async_with_pipes are automatically closed unless the LEAVE_DESCRIPTORS_OPEN is added.

I may be wrong because of my mentioned lack of C skills. If this has become to off-topic for here I can move this to the mailing-list 👍

lazka commented 7 years ago

In case standard_output is False it still gets redirected so it's not the same as the C implementation

I have a hard time finding where this is different in the C module here. To me the code in the C module does nothing to change this, but this may be my lack of C knowledge 😄.

In case standard_input is False, NULL will be passed to g_spawn_async_with_pipes which prevents it from creating a pipe. GLib.spawn_async_with_pipes will never pass NULL for those parameters, so always returning a fd and redirecting stdin.

The returned fds don't get closed in case they don't get returned

I am not supposed to and looking at the C module I don't see where this is done. If I read the docs properly the file descriptors from GLib.spawn_async_with_pipes are automatically closed unless the LEAVE_DESCRIPTORS_OPEN is added.

This talks about the file descriptors of the parent before fork, not the ones returned. In your code you simply ignore the returned fds which makes them leak.

infirit commented 7 years ago

GLib.spawn_async_with_pipes will never pass NULL for those parameters, so always returning a fd and redirecting stdin.

That sound like a bug in the binding, why would None not translate to NULL in pygobject 😕.

In your code you simply ignore the returned fds which makes them leak.

Which is caused by the problem above, oh well. I updated the code for the FD to be closed using GLib.close.

Due to the None/NULL problem the pure python version is broken(ish), which is a shame, anywayz..

thanks for the feedback and explanation 👍

lazka commented 7 years ago

That sound like a bug in the binding, why would None not translate to NULL in pygobject 😕.

spawn_async_with_pipes does not take a Python argument for this, it gets automatically provided by PyGObject.

GI has an annotation for passing NULL to "out" arguments called "(optional)" if you don't care about a specific value, but it is defined that passing NULL does not change the behavior of the function (which happens here) and is not used/exposed in PyGObject.