Message ID | 3319cbc48171ae821c3297f9e5cbb8e9011b87ed.camel@telenet.be |
---|---|
State | Accepted |
Headers | show |
Series | [bug#47869,v3,core-updates] various cross-compilation fixes in guix/build/utils.scm | expand |
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’.
From c1ab0f5161254e66ae3515df60440dd4bcb46fd4 Mon Sep 17 00:00:00 2001 From: Maxime Devos <maximedevos@telenet.be> Date: Mon, 31 May 2021 20:12:55 +0200 Subject: [PATCH 18/18] gnu: wireguard-tools: Set #:sh argument of 'wrap-program'. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * gnu/packages/vpn.scm (wireguard-tools)[arguments]<#:phases>{wrap-wg-quick}: Set #:sh argument of ‘wrap-program’. --- gnu/packages/vpn.scm | 1 + 1 file changed, 1 insertion(+) diff --git a/gnu/packages/vpn.scm b/gnu/packages/vpn.scm index 66c103e75f..34715a4cc8 100644 --- a/gnu/packages/vpn.scm +++ b/gnu/packages/vpn.scm @@ -726,6 +726,7 @@ WireGuard was added to Linux 5.6.") (coreutils (string-append (assoc-ref inputs "coreutils") "/bin"))) (wrap-program (string-append out "/bin/wg-quick") + #:sh (search-input-file inputs "bin/bash") `("PATH" ":" prefix ,(append inputs-sbin (list coreutils)))) #t)))))) -- 2.31.1