Created attachment 288075 [details] [review]
Skip GSpawnChildSetupFunc closures in introspection

It's not certain that the runtime of any bound language using the
introspection supports running in a process forked by a foreign
library, so that a closure programmed in that language would work
safely.

Any programming environment supporting that would probably have
its own advanced facilities for process spawning, or be able
to access the GLib spawning APIs via raw C bindings (still
represented in the introspection, (skip) only adds a flag)
and do any low-level preparatory dances as necessary for the
forked runtime.

Also properly annotated the closures, for good measure.

(In reply to comment #1)
> Created an attachment (id=288075) [details] [review]
> Skip GSpawnChildSetupFunc closures in introspection

Note that bug #738171 would cause gobject-introspection build to fail if this is applied before that bug is fixed.

Review of attachment 288075 [details] [review]:

::: gio/gdesktopappinfo.c
@@ -2991,3 @@
  * @launch_context: (allow-none): a #GAppLaunchContext
  * @spawn_flags: #GSpawnFlags, used for each process
- * @user_setup: (scope call) (allow-none): a #GSpawnChildSetupFunc, used once

See comments to the gspawn.c patch.

::: gio/gsubprocesslauncher.c
@@ -633,2 @@
 /**
- * g_subprocess_launcher_set_child_setup:

It does make sense to skip this one, probably.

::: glib/gspawn.c
@@ -110,3 @@
  * @envp: (array zero-terminated=1) (allow-none): child's environment, or %NULL to inherit parent's
  * @flags: flags from #GSpawnFlags
- * @child_setup: (scope async) (allow-none): function to run in the child just before exec()

I'm afraid we can't really do this.  Some people may be using this API from bindings and passing NULL/None/whatever.  Adding (skip) here would effectively change the API.

::: glib/gspawn.h
@@ +107,3 @@
 /**
  * GSpawnChildSetupFunc:
+ * @user_data: (closure): user data to pass to the function.

Why are you aiming to make this more usable from bindings while declaring it unusable from bindings?

(In reply to Ryan Lortie (desrt) from comment #3)
> @@ +107,3 @@
>  /**
>   * GSpawnChildSetupFunc:
> + * @user_data: (closure): user data to pass to the function.
> 
> Why are you aiming to make this more usable from bindings while declaring it
> unusable from bindings?

It still makes sense to annotate things correctly, because it all ends up there in GIR, and some binding projects might decide to ignore the skip flag and generate the binding anyway because their C support is exceptionally good, or something.

Created attachment 312580 [details] [review]
Fix up closure annotations for GSpawnChildSetupFunc

Created attachment 312581 [details] [review]
Skip g_subprocess_launcher_set_child_setup() in introspection

It's not likely that the runtime of a bound language using the
introspection supports running in a process forked by a foreign
library, so that a closure programmed in that language would work
safely.

Any programming environment supporting that would probably have
its own advanced facilities for process spawning, or be able
to access the GLib spawning APIs via raw C bindings (still
represented in the introspection, (skip) only adds a flag)
and do any low-level preparatory dances as necessary for the
forked runtime.

Note that there are other APIs making use of GSpawnChildSetupFunc,
but they are usable with the closure nullified, and we cannot annotate
the closure parameters away because that would break the annotated API
for bindings; accordingly to bug #738176 comment #3, the current bindings'
users are expected to pass null.

Created attachment 312582 [details] [review]
gspawn, gdesktopappinfo: Replace (allow-none) with modern annotations

Review of attachment 312580 [details] [review]:

++

Review of attachment 312581 [details] [review]:

++

Review of attachment 312582 [details] [review]:

This was already done as commit 18a33f72db.

Rebased (trivial rebase conflicts) and pushed.

Attachment 312580 [details] pushed as 48cf1d3 - Fix up closure annotations for GSpawnChildSetupFunc
Attachment 312581 [details] pushed as e5db8ec - Skip g_subprocess_launcher_set_child_setup() in introspection