[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:

> Some differences to v2:
>
>   * The #:sh and #:guile arguments are optional.
>     The default value should be good when compiling natively,
>     but not when cross-compiling.
>
>     Eventually, we may look into making them required,
>     but let's pun for later.
>
>   * I left 'wrap-qt-program' alone for now.
>
>   * I left documenting 'wrap-program' and 'wrap-script' for later.
>
>   * I didn't adjust all uses of wrap-program to set #:sh,
>     only a few.

[...]

> This patch series is on top of commit 9ba35475ede5eb61bfeead096bc6b73f123ac891
> on core-updates.

Woow, nice!

I’ll first focus on the first few patches, those that trigger a world
rebuild.  Subsequent patches look good and are less “critical”.

> From 02d2b52458fae1c391e79f89a89696f3b07fdb2b Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Mon, 31 May 2021 18:22:31 +0200
> Subject: [PATCH 01/18] =?UTF-8?q?build:=20Allow=20overriding=20the=20shell?=
>  =?UTF-8?q?=20interpreter=20in=20=E2=80=98wrap-program=E2=80=99.?=
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Previously, when creating new wrappers, 'wrap-program' would search
> for an interpreter to use in PATH. However, this is incorrect when
> cross-compiling. Allow overriding the shell interpreter to use,
> via an optional keyword argument #:sh.
>
> In time, when all users of 'wrap-program' have been corrected,
> this keyword argument can be made mandatory.
>
> * guix/build/utils.scm (wrap-program): Introduce a #:sh keyword
>   argument, defaulting to (which "sh"). Use this keyword argument.
>
> Partially-Fixes: <https://issues.guix.gnu.org/47869>

LGTM (will apply together with the other world-rebuild changes).

> From f598c0168bfcb75f718cc8edf990b7a560334405 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Mon, 31 May 2021 18:36:09 +0200
> Subject: [PATCH 02/18] =?UTF-8?q?build:=20Define=20=E2=80=98search-input-f?=
>  =?UTF-8?q?ile=E2=80=99=20procedure.?=
> 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.
>
> When compiling natively (target=#f in Guix parlance),
> this is perfectly fine.
>
> However, when cross-compiling, there is a problem.
> "which" looks in $PATH for binaries.  That's good for purpose (1),
> but incorrect for (2), as the $PATH contains binaries from native-inputs
> instead of inputs.
>
> This commit defines a ‘search-input-file’ procedure. It functions
> like 'which', but instead of searching in $PATH, it searches in
> the 'inputs' of the build phase, which must be passed to
> ‘search-input-file’ as an argument. Also, the file name must
> include "bin/" or "sbin/" as appropriate.
>
> * guix/build/utils.scm (search-input-file): New procedure.
> * tests/build-utils.scm
>   ("search-input-file: exception if not found")
>   ("search-input-file: can find if existent"): Test it.
> * doc/guix.texi (File Search): Document it.
>
> Partially-Fixes: <https://issues.guix.gnu.org/47869>

I don’t think we need the whole story here :-) though it doesn’t hurt.
‘search-input-file’ is useful on its own IMO.

> +@deffn {Scheme Procedure} search-input-file @var{inputs} @var{name}
> +Return the complete file name for @var{name} as found in @var{inputs}.
> +If @var{name} could not be found, an exception is raised instead.
> +Here, @var{inputs} is an association list like @var{inputs} and
> +@var{native-inputs} as available to build phases.
> +
> +This procedure can be used for telling @code{wrap-script} and
> +@code{wrap-program} (currently undocumented) where the Guile
> +binary or shell binary are located. In fact, that's the
> +purpose for which @code{search-input-file} has been created
> +in the first place.
> +@end deffn

I’d remove the second paragraph: IMO it’s not the right place to
document the motivation.  However, an @lisp example would be nice.

BTW, please remember to leave two spaces after end-of-sentence periods.

> +(define (search-input-file inputs file)
> +  "Find a file named FILE among the INPUTS and return its absolute file name.
> +
> +FILE must be a string like \"bin/sh\". If FILE is not found, an exception is
> +raised."
> +  (or (search-path (map cdr inputs) file)
> +      (error "could not find ~a among the inputs" file)))

Rather:

  (match inputs
    (((_ . directories) ...)
     (or (search-path directories file)
         (raise (condition (&search-error (path directories) (file file)))))))

… so you’d need to define a new error condition type.

It’s better to make this extra effort; ‘error’ throws to 'misc-error and
cannot be meaningfully handled by callers.

> +(test-assert "search-input-file: exception if not found"
> +  (not (false-if-exception
> +         (search-input-file '() "does-not-exist"))))

Here you’d use ‘guard’ to check you got the right exception.

> From 98856ca64218bd98c0d066a25ac93038a98c7ff5 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Tue, 1 Jun 2021 21:47:01 +0200
> Subject: [PATCH 03/18] glib-or-gtk-build-system: Look up the interpreter in
>  'inputs'.
>
> * guix/build/glib-or-gtk-build-system.scm (wrap-all-programs): Pass
>   the shell interpreter from 'inputs' to 'wrap-program' using
>   'search-input-file'.
>
> Partially-Fixes: <https://issues.guix.gnu.org/47869>

[...]

> +  ;; Do not require bash to be present in the package inputs
> +  ;; even when there is nothing to wrap.
> +  ;; Also, calculate (sh) only once to prevent some I/O.
> +  (define %sh (delay (search-input-file inputs "bin/bash")))
> +  (define (sh) (force %sh))

I’d be tempted for clarity to simply write:

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

The extra ‘stat’ calls may be okay in practice but yeah, dunno.

> From bc0085b79dd42e586cc5fcffa6f4972db9f42563 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Tue, 1 Jun 2021 21:48:44 +0200
> Subject: [PATCH 04/18] python-build-system: Look up the interpreter in
>  'inputs'.
>
> * guix/build/python-build-system.scm (wrap): Pass the shell
>   interpreter from 'inputs' to 'wrap-program' using 'search-input-file'.
>
> Partially-Fixes: <https://issues.guix.gnu.org/47869>

[...]

> From 0370ad982e90c3e4def9cd5245cbd6769fda2830 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Mon, 31 May 2021 19:20:12 +0200
> Subject: [PATCH 05/18] qt-build-system: Look up the interpreter in 'inputs'.
>
> * guix/build/qt-build-system.scm (wrap-all-programs): Pass
>   the shell interpreter from 'inputs' to 'wrap-program' using
>   'search-input-file'.
>
> Partially-Fixes: <https://issues.guix.gnu.org/47869>

[...]

> From 92278afdc58430e8e9f6887d481964e1d73e551c Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos@telenet.be>
> Date: Mon, 31 May 2021 19:21:16 +0200
> Subject: [PATCH 06/18] rakudo-build-system: Look up the interpreter in
>  'inputs'.
>
> * guix/build/rakudo-build-system.scm (wrap): Pass
>   the shell interpreter from 'inputs' to 'wrap-program' using
>   'search-input-file'.
>
> Partially-Fixes: <https://issues.guix.gnu.org/47869>

LGTM!

So in the end, I’m suggesting modifications to #2 and the rest LGTM.

Thank you!

Ludo’.