[bug#47869] [PATCH core-updates] ‘which’ looks in PATH, incorrect when cross-compiling

["Ludovic Courtès" <ludo@gnu.org>]

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> skribis:

> This is version two of the patch series, removing a 'pk' that
> I added for debugging, and also fixing 'wrap-script' and 'wrap-program'.
> To fix 'wrap-script' and 'wrap-program', I added a required 'inputs' argument.
> All callers have been adjusted to pass it.
>
> Perhaps 'patch-shebang' can be fixed and needs to be fixed, but I don't
> have investigated that closely yet.  ('patch-shebangs' (with a #\s) works
> properly IIUC).

Thanks for this long patch series, and sorry for the equally long delay!

Since we don’t get to change those interfaces very often, I’m going to
nitpick somewhat because I think we’d rather get them right.

> From 42e7cf4ca6e4d6e1cd31c2807f608275a5ca759a Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Sun, 18 Apr 2021 12:45:13 +0200
> Subject: [PATCH 1/7] build: Add argument to which for specifying where to
>  search.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The procedure ‘which’ from (guix build utils)
> is used for two different purposes:
>
>  1. for finding the absolute file name of a binary
>     that needs to run during the build process
>
>  2. for finding the absolute file name of a binary,
>     for the target system (as in --target=TARGET),
>     e.g. for substituting sh->/gnu/store/.../bin/sh,
>     python->/gnu/store/.../bin/python.

But note that only #1 is the intended purpose.

> When compiling natively (SYSTEM=TARGET modulo nix/autotools differences),
> this is perfectly fine.

Rather “target=#f” in Guix parlance

[...]

> +(define* (which program #:optional inputs)
> +  "Return the complete file name for PROGRAM as found in $PATH, or #false if
> +PROGRAM could not be found.  If INPUTS is not #false, instead look in the
> +/bin and /sbin subdirectories of INPUTS.  INPUTS is an alist; its keys
> +are ignored."

I find that this leads to a weird interface; ‘which’ is intended to be
like the same-named shell command, and the notion of “input alists”
seems out of place to me.

I was thinking we could make it:

--8<---------------cut here---------------start------------->8---
(define* (which program #:optional
                (path (search-path-as-string->list (getenv "PATH"))))
  "Return the complete file name for PROGRAM as found in $PATH, or #f if
PROGRAM could not be found."
  (search-path path program))
--8<---------------cut here---------------end--------------->8---

… but that doesn’t buy us much.

I think what we need is to do is find and fix misuses of ‘which’.

WDYT?


[...]

> +Here is an example using the @code{which} procedure in a build phase:
> +
> +@lisp
> +(lambda* (#:key outputs inputs #:allow-other-keys)
> +  (let ((growpart (string-append (assoc-ref outputs "out")
> +                                          "/bin/growpart")))
> +     (wrap-program growpart
> +                   `("PATH" ":" prefix (,(dirname (which "sfdisk" inputs))
> +                                        ,(dirname (which "readlink" inputs)))))))

That looks weird to me.  The “correct” way we do it right now is like
this:

        (lambda* (#:key outputs inputs #:allow-other-keys)
          (let ((out (assoc-ref outputs "out"))
                (curl (assoc-ref inputs "curl")))
            (wrap-program (string-append out "/bin/akku")
              `("LD_LIBRARY_PATH" ":" prefix (,(string-append curl "/lib"))))
            #t))

Here, when cross-compiling, (assoc-ref inputs "curl") returns the target
cURL.

> From e78d2d8651d5f56afa7d57be78c5cccccebb117a Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Sun, 18 Apr 2021 20:44:28 +0200
> Subject: [PATCH 3/7] build: utils: Make inputs of 'wrap-script' explicit.
>
> Previously, 'wrap-script' used (which "guile") to determine where to locate
> the guile interpreter.  But this is incorrect when cross-compiling.  When
> cross-compiling, this would locate the (native) guile interpreter that is
> in the PATH, while a guile interpreter for the target is required.
>
> Remove the optional #:guile argument which is only used in tests and replace
> it with a required 'inputs' argument and adjust all callers.  Write a new
> test verifying a guile for the target is located, instead of a native guile.

I think the #:guile argument was a fine interface: clear and
to-the-point.  The problem IMO is just that it’s not use where it
should.  :-)

[...]

> --- a/gnu/packages/audio.scm
> +++ b/gnu/packages/audio.scm
> @@ -4712,9 +4712,9 @@ as is the case with audio plugins.")
>                 (chmod (string-append out "/share/carla/carla") #o555)
>                 #t)))
>           (add-after 'install 'wrap-executables
> -           (lambda* (#:key outputs #:allow-other-keys)
> +           (lambda* (#:key inputs outputs #:allow-other-keys)
>               (let ((out (assoc-ref outputs "out")))
> -               (wrap-script (string-append out "/bin/carla")
> +               (wrap-script (string-append out "/bin/carla") inputs
>                              `("GUIX_PYTHONPATH" ":" prefix (,(getenv "GUIX_PYTHONPATH"))))

This would become:

  (wrap-script (string-append out "/bin/carla")
               `(…)
               #:guile (assoc-ref inputs "guile"))

WDYT?

> From 8b843f0dd8803120718747b480983bd5888b1617 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Mon, 19 Apr 2021 16:56:00 +0200
> Subject: [PATCH 6/7] build: utils: wrap-program: look up bash in inputs, not
>  in PATH
>
> 'wrap-program' is almost always used for creating wrappers for the
> target system.  It is only rarely (once) used for creating wrappers for
> the native system.  However, 'wrap-program' always creates wrappers for
> the native system and provides no option for creating wrappers for the
> target system instead.

[...]

> -                 (wrap-program
> -                     (string-append libexec "/dhclient-script")
> +                 (wrap-program (string-append libexec "/dhclient-script")
> +                   inputs
>                     `("PATH" ":" prefix
>                       ,(map (lambda (dir)
>                               (string-append dir "/bin:"

I’m also skeptical here; ‘wrap-program’ needs to know the file name of
‘sh’ and instead we’re passing it the full input list.

I would instead add #:bash (or #:sh?).  The downside is that it’d be a
bit more verbose, but in terms of interfaces, I’d find it clearer:

  (wrap-program (string-append libexec "/dhclient-script")
    `("PATH" …)
    #:sh (string-append (assoc-ref inputs "bash") "/bin/sh"))

We could introduce a helper procedure to replace (string-append …) with:

  (search-input-file inputs "/bin/sh")

where:

  (define (search-input-file inputs file)
    (any (match-lambda
           ((label . directory)
            (let ((file (string-append directory "/" file)))
              (and (file-exists? file) file))))
         inputs))

WDYT?

> +           (wrap-program (string-append out "/bin/screenfetch")
> +             %build-inputs

As a rule of thumb we should refer to #:inputs and #:outputs instead of
the global variables ‘%build-inputs’ etc.

> From cdd45bc0aef8b6cb60d351a8fded18700804e8db Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Mon, 19 Apr 2021 19:54:53 +0200
> Subject: [PATCH 7/7] doc: Document 'wrap-program'.
>
> * doc/guix.texi (Wrapping Code)[wrap-program]: Copy docstring from
>   guix/build/utils.scm and use Texinfo markup.

Neat!

>  doc/guix.texi | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index a2ff13fe0f..6235ae9bf7 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -8703,7 +8703,42 @@ Here is an example using the @code{which} procedure in a build phase:
>  This section documents procedures that create ‘wrappers’ around existing
>  binaries, that e.g. set environment variables required during execution.
>  
> -@c TODO document wrap-program
> +@deffn {Scheme Procedure} wrap-program @var{prog} @var{inputs} @var{vars}
> +Make a wrapper for @var{prog}.  @var{vars} should look like this:
> +
> +@lisp
> +  '(VARIABLE DELIMITER POSITION LIST-OF-DIRECTORIES)
   ^
You can remove indentation and use @var instead of capital letters.

> +@lisp
> +  (wrap-program "foo"
> +                '("PATH" ":" = ("/gnu/.../bar/bin"))
> +                '("CERT_PATH" suffix ("/gnu/.../baz/certs"
> +                                        "/qux/certs")))
   ^^
You can remove indentation here too.

> +@end lisp
> +
> +will copy @file{foo} to @file{.foo-real} and create the file @file{foo} with
> +the following contents:
> +
> +@example
> +  #!location/of/bin/bash
> +  export PATH="/gnu/.../bar/bin"
> +  export CERT_PATH="$CERT_PATH$@{CERT_PATH:+:@}/gnu/.../baz/certs:/qux/certs"
> +  exec -a $0 location/of/.foo-real "$@@"

… and here.

This one can even go to master.

Thanks!

Ludo’.