Hi Josselin,
Sorry for taking so long to come back to you. I think this is great
work! I pushed it as ‘wip-posix-spawn’ so others can give it a try.
Josselin Poiret <d...@jpoiret.xyz> skribis:
> * libguile/posix.c (renumber_file_descriptor, start_child,
> scm_piped_process): Remove functions.
> (scm_port_to_fd_with_default): New helper function.
> (scm_system_star): Rewrite using scm_spawn_process.
> (scm_init_popen): Remove the definition of piped-process.
> (scm_init_posix): Now make popen available unconditionally.
>
> * module/ice-9/popen.scm (port-with-defaults): New helper procedure.
> (spawn): New procedure.
> (open-process): Rewrite using spawn.
> (pipeline): Rewrite using spawn*.
>
> * test-suite/tests/popen.test ("piped-process", "piped-process:
> with-output"): Removed tests.
> ("spawn", "spawn: with output"): Added tests.
> * test-suite/tests/posix.test ("http://bugs.gnu.org/13166";, "exit code
> for nonexistent file", "https://bugs.gnu.org/55596";): Remove obsolete
> tests.
> ("exception for nonexistent file"): Add test.
> ---
> libguile/posix.c | 218 +++---------------------------------
> module/ice-9/popen.scm | 83 ++++++++++----
> test-suite/tests/popen.test | 14 +--
> test-suite/tests/posix.test | 36 +++---
> 4 files changed, 102 insertions(+), 249 deletions(-)
More deletions than insertions. 👍
That scary-looking Gnulib update seems to have worked well.
I have mostly cosmetic/polishing comments and one issue with ‘system*’.
I can actually do that on your behalf if you’re unavailable these days;
let me know what you prefer.
> static SCM
> scm_spawn_process (SCM prog, SCM args, SCM scm_in, SCM scm_out, SCM scm_err)
> #define FUNC_NAME "spawn*"
For top-level functions, please add a comment above explaining what it
does.
I would call this one ‘primitive-spawn’ rather than ‘spawn*’ and keep it
private to (ice-9 popen).
> +(define* (spawn exec-file argv #:key
> + (in (current-input-port))
> + (out (current-output-port))
> + (err (current-error-port)))
Please add a docstring. It may also be worth documenting it in the
manual given that it’s public.
> + (let* ((in (port-with-defaults in "r"))
> + (out (port-with-defaults out "w"))
> + (err (port-with-defaults err "w"))
I’d make it “r0” and “w0” since we’re doing to throw the ports away
right after.
We could even avoid allocating a port when we’re going to use /dev/null
(and thus go with ‘open-fdes’ directly), but that’s a micro-optimization
we can keep for later.
> +++ b/test-suite/tests/posix.test
> @@ -236,24 +236,24 @@
>
> (with-test-prefix "system*"
>
> - (pass-if "http://bugs.gnu.org/13166";
> - ;; With Guile up to 2.0.7 included, the child process launched by
> - ;; `system*' would remain alive after an `execvp' failure.
> - (let ((me (getpid)))
> - (and (not (zero? (system* "something-that-does-not-exist")))
> - (= me (getpid)))))
I’d keep this one, I guess it doesn’t hurt?
> - (pass-if-equal "exit code for nonexistent file"
> - 127 ;aka. EX_NOTFOUND
> - (status:exit-val (system* "something-that-does-not-exist")))
It’s good that we have better error reporting thanks to ‘posix_spawn’.
However, I don’t think we can change that in 3.0. What about, for 3.0,
adding a layer around ‘spawn’ so that ‘system*’ still returns 127 when
‘spawn’ throws to ‘system-error’?
> - (pass-if-equal "https://bugs.gnu.org/55596";
> - 127
> - ;; The parameterization below used to cause 'start_child' to close
> - ;; fd 2 in the child process, which in turn would cause it to
> - ;; segfault, leading to a wrong exit code.
> - (parameterize ((current-output-port (current-error-port)))
> - (status:exit-val (system* "something-that-does-not-exist")))))
Likewise we should keep this one.
> + (pass-if-equal "exception for nonexistent file"
> + 2 ; ENOENT
> + (call-with-prompt 'escape
> + (lambda ()
> + (with-exception-handler
> + (lambda (exn)
> + (let* ((kind (exception-kind exn))
> + (errno (and (eq? kind 'system-error)
> + (car (car
> + (cdr (cdr (cdr (exception-args
> + exn)))))))))
> + (abort-to-prompt 'escape errno)))
> + (lambda ()
> + (status:exit-val (system*
> + "something-that-does-not-exist")))
> + #:unwind? #t))
> + (lambda (k arg)
> + arg))))
We’ll have to leave this change for the next major series of Guile.
Thanks!
Ludo’.
