Ludovic Courtès schreef op di 18-05-2021 om 22:51 [+0200]: > Hi Maxime, > > Maxime Devos 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 > > 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. It is? Then it seems the first patch can be dropped and replaced with something else, as you mentioned below. > > When compiling natively (SYSTEM=TARGET modulo nix/autotools differences), > > this is perfectly fine. > > Rather “target=#f” in Guix parlance Yes, correct. > [...] > > > +(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? The current correct way is (string-append (assoc-ref inputs "the-input") "/bin/the-binary") which can easily lead to long lines. Ideally, there would be a shorter way to do this, such as ... the weird interface above. Or the "search-input-file" from below. > > [...] > > > +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 "dirname" & "which" look weird to me to! I grabbed that from some package definition. I guess a different example is needed. > 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. This is something that can be fixed on 'core-updates', right? At least when fixing the package definitions doesn't cause to many rebuilds. > > From e78d2d8651d5f56afa7d57be78c5cccccebb117a Mon Sep 17 00:00:00 2001 > > From: Maxime Devos > > 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. :-) It should be used practically everywhere, no? So making it optional doesn't make much sense to me when we want to support cross-compilation. > > [...] > > > --- 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? Ok, this looks a good interface to me, though I think 'wrap-script' will need to be modified. IIRC, #:guile must be the full file name of the guile binary and not simply /gnu/store/[...]-guile-$VERSION. > > From 8b843f0dd8803120718747b480983bd5888b1617 Mon Sep 17 00:00:00 2001 > > From: Maxime Devos > > 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")) LGTM, though rather verbose. > 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? That should help with the verbosity. The previous code becomes (wrap-program (string-append libexec "/dhclient-script") `("PATH" …) #:sh (search-input-file inputs "bin/sh")) which isn't overly verbose. This procedure 'search-input-file' would return #f if the input was not found, right? I would rather it raises an exception instead. There have been some bugs where "#f" was silently written into some file, which is unlikely to work well. For the few cases were the input binary is truly optional, we can define a 'search-optional-input-file' procedure. > > > > + (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. Agreed, that's what I thought as well, but that seems like a separate (stylistic) bug to fix. IIRC, the surrounding code used %build-inputs instead of #:inputs. > [...] > > 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. @var can be used inside @lisp? Didn't know that. > [...] > > This one can even go to master. Yep. Greetings, Maxime.