Re: guile pipeline do-over

[Ludovic Courtès]

Hi Rutger!

Rutger van Beusekom <address@hidden> skribis:

> This patch replaces open-process with piped-process in posix.c and
> reimplement open-process with piped-process in popen.scm. This allows
> setting up a pipeline in guile scheme using the new pipeline procedure
> in popen.scm and enables its use on operating systems which happen to
> lack the capability to fork, but do offer the capability captured by
> start_child (see posix.c).

Nice!  That’s definitely very useful to have.  We’ll need to check what
Andy thinks, but I think it can be added in the 3.0 series.

> From 3c8f5d534419418234cfe7d3eda8227951bc208a Mon Sep 17 00:00:00 2001
> From: Rutger van Beusekom <address@hidden>
> Date: Mon, 2 Mar 2020 10:38:57 +0100
> Subject: [PATCH] Allow client code to create pipe pairs when opening a
>  process.
>
> * libguile/posix.c (scm_piped_process): Replace open_process by piped_process.

Could you mention functions renamed/removed here?  The ChangeLog format
is about boringly listing all the language-entity-level changes.  :-)

> * module/ice-9/popen.scm (pipe->fdes): Convert pipe pair to fdes pair.
> (open-process): Implement open-process with piped-process.
> (pipeline): Implement a pipeline with piped-process.
>  static SCM
> -scm_open_process (SCM mode, SCM prog, SCM args)
> -#define FUNC_NAME "open-process"
> +scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
> +#define FUNC_NAME "piped-process"
> +/* SCM_DEFINE (scm_piped_process, "piped-process", 2, 2, 0, */
> +/*            (SCM prog, SCM args, SCM from, SCM to), */
> +/* "Execute the command indicated by @var{prog} with arguments 
> @var(args),\n" */
> +/* "optionally connected by an input and an output pipe.\n" */
> +/* "@var(from) and @var(to) are either #f or a valid file descriptor\n" */
> +/* "of an input and an output pipe, respectively.\n" */
> +/* "\n" */
> +/* "This function returns the PID of the process executing @var(prog)." */
> +/* "\n" */
> +/* "Example:\n" */
> +/* "(let ((p (pipe)))\n" */
> +/* "  (piped-process \"echo\" '(\"foo\" \"bar\")\n" */
> +/* "                 (cons (port->fdes (car p))\n" */
> +/* "                       (port->fdes (cdr p))))\n" */
> +/* "  (display (read-string (car p))))\n" */
> +/* "(let ((p (pipe)))\n" */
> +/* "  (read-string (piped-process \"echo\" '(\"foo\" \"bar\")\n" */
> +/* "                              (port->fdes (car p)))))\n") */
> +/* #define FUNC_NAME scm_piped_process */

I guess you can remove the commented-out bits…

> -  int c2p[2]; /* Child to parent.  */
> -  int p2c[2]; /* Parent to child.  */
> +  int c2p[2] = {}; /* Child to parent.  */
> +  int p2c[2] = {}; /* Parent to child.  */

… and this hunk, to minimize change.

> +++ b/module/ice-9/popen.scm
> @@ -22,9 +22,10 @@
>    #:use-module (rnrs bytevectors)
>    #:use-module (ice-9 binary-ports)
>    #:use-module (ice-9 threads)
> +  #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-9)
>    #:export (port/pid-table open-pipe* open-pipe close-pipe open-input-pipe
> -            open-output-pipe open-input-output-pipe))
> +            open-output-pipe open-input-output-pipe pipe->fdes piped-process 
> pipeline))

I would not export ‘pipe->fdes’.  I’m not sure about exporting
‘piped-process’: it’s a bit low-level and we might want to reserve
ourselves the possibility to change it, like this patch does actually.

WDYT?

> +(define (open-process mode command . args)
> +  (let* ((from (and (or (equal? mode OPEN_READ) (equal? mode OPEN_BOTH)) 
> (pipe->fdes)))
> +         (to (and (or (equal? mode OPEN_WRITE) (equal? mode OPEN_BOTH)) 
> (pipe->fdes)))
> +         (pid (piped-process command args from to)))
> +    (values (and from (fdes->inport (car from))) (and to (fdes->outport (cdr 
> to))) pid)))

Please wrap lines to 80 chars.

I suggest using ‘string=?’ above instead of ‘equal?’.  Also, could you
add a docstring?

> +(define (pipeline procs)
> +  "Execute a pipeline of @code(procs) -- where a proc is a list of a
> +command and its arguments as strings -- returning an input port to the
> +end of the pipeline, an output port to the beginning of the pipeline and
> +a list of PIDs of the @code(procs)"

Perhaps s/procs/commands/ would be clearer?  Also, @var{commands}
instead of @code.

Could you also add an entry in doc/ref/*.texi, in the “Pipes” node,
perhaps with one of the examples you gave?

> +++ b/test-suite/tests/popen.test
> @@ -211,3 +211,37 @@ exec 2>~a; read REPLY"
>       (let ((st (close-pipe (open-output-pipe "exit 1"))))
>         (and (status:exit-val st)
>              (= 1 (status:exit-val st)))))))
> +
> +
> +;;
> +;; pipeline related tests
> +;;
> +
> +(use-modules (ice-9 receive))
> +(use-modules (ice-9 rdelim))

Please move these to the top-level ‘define-module’ form.

One last thing: we’d need you to assign copyright to the FSF for this.
We can discuss it off-line if you want.

Thank you for this great and long overdue addition!

Ludo’.