Re: [PATCH] utils: Allow wrap-program to be called multiple times.

[Ludovic Courtès]

Eric Bavier <address@hidden> skribis:

> Currently, if (@ (guix build utils) wrap-program) is called multiple
> times for the same file, the original file ends up being overwritten.

OK, you’ve convinced me that this improvement is welcome.

Some comments on the patch:

> From 231130db4444685d8f3264e61d680634eaead9fb Mon Sep 17 00:00:00 2001
> From: Eric Bavier <address@hidden>
> Date: Tue, 9 Sep 2014 17:47:31 -0500
> Subject: [PATCH] utils: Allow wrap-program to be called multiple times.
>
> * guix/build/utils.scm (wrap-program): Multiple invocations of
>   wrap-program for the same file create successive wrappers.
> ---
>  guix/build/utils.scm |   26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/guix/build/utils.scm b/guix/build/utils.scm
> index 2f3dc9c..d4435b4 100644
> --- a/guix/build/utils.scm
> +++ b/guix/build/utils.scm
> @@ -711,8 +711,24 @@ contents:
>  This is useful for scripts that expect particular programs to be in $PATH, 
> for
>  programs that expect particular shared libraries to be in $LD_LIBRARY_PATH, 
> or
>  modules in $GUILE_LOAD_PATH, etc."
> -  (let ((prog-real (string-append (dirname prog) "/." (basename prog) 
> "-real"))
> -        (prog-tmp  (string-append (dirname prog) "/." (basename prog) 
> "-tmp")))
> +  (define (wrapper-path num)
> +    (format #f "~a/.~a-wrap-~2'0d" (dirname prog) (basename prog) num))

Make it ‘wrapper-file-name’ (in GNU, “path” means “search path”.)

> +  (let* ((current-wrappers
> +          (find-files (dirname prog)
> +                      (string-append "\\." (basename prog) "-wrap-.*")))
> +         (wrapper-num (if (null? current-wrappers)
> +                          0
> +                          (string->number
> +                           (string-take-right (last current-wrappers) 2))))

These two could be factorized as a local ‘next-wrapper-number’
procedure.

For local variables, it’s better to use shorter names, such as
‘wrappers’ and ‘number’ here.

> +         (wrapper-tgt (if (zero? wrapper-num)
> +                          (let ((prog-real (string-append
> +                                            (dirname prog) "/."
> +                                            (basename prog) "-real")))
> +                            (copy-file prog prog-real)
> +                            prog-real)
> +                          (wrapper-path wrapper-num)))

Make it a ‘wrapper-target’ local procedure, and change ‘wrapper-tgt’ to
‘target’.

It looks OK.  It would be ideal if a test in tests/build-utils.scm made
sure that ‘wrap-program’ uses the right file names when called multiple
times, but I won’t object if the patch doesn’t have it.

Thanks,
Ludo’.