Hi Maxime, Maxime Devos 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 > 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: LGTM (will apply together with the other world-rebuild changes). > From f598c0168bfcb75f718cc8edf990b7a560334405 Mon Sep 17 00:00:00 2001 > From: Maxime Devos > 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: 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 > 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: [...] > + ;; 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 > 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: [...] > From 0370ad982e90c3e4def9cd5245cbd6769fda2830 Mon Sep 17 00:00:00 2001 > From: Maxime Devos > 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: [...] > From 92278afdc58430e8e9f6887d481964e1d73e551c Mon Sep 17 00:00:00 2001 > From: Maxime Devos > 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: LGTM! So in the end, I’m suggesting modifications to #2 and the rest LGTM. Thank you! Ludo’.