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

[Maxime Devos <maximedevos@telenet.be>]

[-- Attachment #1: Type: text/plain, Size: 10602 bytes --]

Ludovic Courtès schreef op di 18-05-2021 om 22:51 [+0200]:
> 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.

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 <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.  :-)

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 <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"))

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]