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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 47869 in the body.
You can then email your comments to 47869 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: guix-patches <at> gnu.org
Subject: [PATCH core-updates] ‘which’ looks in
 PATH, incorrect when cross-compiling
Date: Sun, 18 Apr 2021 13:20:37 +0200

[Message part 1 (text/plain, inline)]

The procedure ‘which’ from (guix build utils)
is used for two different purposes:

 1. for finding the absolute file name of a binary
    that we need 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 (SYSTEM=TARGET modulo nix/autotools differences),
this is perfectly fine.

However, when cross-compiling, we have 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.

Admittedly, ‘which’ is simply very convenient (although incorrect
when cross-compiling), and we don't have the right tool ...
until now, that is (see patch)!

This patch adds an optional 'inputs' argument.  When it is present,
'which' will look in the bin and sbin subdirectories of the directories
in the 'inputs' alist.

Use it like this:

  (package [...]
    (arguments
     `(#:modules (guix build utils)
       #:phases
       (modify-phases %standard-phases
         (delete 'configure)
         (delete 'check)
         (add-after 'install 'wrap
           (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)))))))))))

(Examples comes from the "cloud-utils" package)
The only change is adding adding the 'inputs' argument.  Isn't that easy?

Alternative methods I've seen:
* (string-append (assoc-ref inputs "coreutils") "/bin/readlink")
* (let ((coreutils (assoc-ref inputs "coreutils")))
    (setenv "PATH" (string-append coreutils "/bin"))
    [code using (which "readlink")])
* (which "readlink") ; possibly incorrect when cross-compiling

I've tested this with "cloud-utils", though admittedly I didn't try to cross-compile
yet, and I've placed my adjusted "which" in a separate module to avoid having to rebuild
everything.  (The attached patch just modifies (guix build utils).)  I've written
a few tests, which pass.  I also documented the new functionality in the manual.

Currently incorrect uses of "which" can be fixed in a follow-up patch.

Thoughts?
Maxime.

[0001-build-Add-argument-to-which-for-specifying-where-to-.patch (text/x-patch, attachment)]

[0002-doc-Document-new-functionality-of-which.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 47869 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47869 <at> debbugs.gnu.org
Subject: [PATCH v2 core-updates] various cross-compilation fixes in
 guix/build/utils.scm
Date: Mon, 19 Apr 2021 21:04:40 +0200

[Message part 1 (text/plain, inline)]

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

Greetings,
Maxime.

[0001-build-Add-argument-to-which-for-specifying-where-to-.patch (text/x-patch, attachment)]

[0002-doc-Document-new-functionality-of-which.patch (text/x-patch, attachment)]

[0003-build-utils-Make-inputs-of-wrap-script-explicit.patch (text/x-patch, attachment)]

[0004-doc-Document-wrap-script.patch (text/x-patch, attachment)]

[0005-tests-build-utils-Remove-obsolete-comment.patch (text/x-patch, attachment)]

[0006-build-utils-wrap-program-look-up-bash-in-inputs-not-.patch (text/x-patch, attachment)]

[0007-doc-Document-wrap-program.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 47869 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47869 <at> debbugs.gnu.org
Subject: Re: bug#47869: [PATCH core-updates] ‘which’
 looks in PATH, incorrect when cross-compiling
Date: Tue, 18 May 2021 22:51:02 +0200

Hi Maxime,

Maxime Devos <maximedevos <at> 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 <at> 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.

> When compiling natively (SYSTEM=TARGET modulo nix/autotools differences),
> this is perfectly fine.

Rather “target=#f” in Guix parlance

[...]

> +(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?


[...]

> +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 “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.

> From e78d2d8651d5f56afa7d57be78c5cccccebb117a Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos <at> 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.  :-)

[...]

> --- 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?

> From 8b843f0dd8803120718747b480983bd5888b1617 Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos <at> 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"))

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?

> +           (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.

> From cdd45bc0aef8b6cb60d351a8fded18700804e8db Mon Sep 17 00:00:00 2001
> From: Maxime Devos <maximedevos <at> telenet.be>
> Date: Mon, 19 Apr 2021 19:54:53 +0200
> Subject: [PATCH 7/7] doc: Document 'wrap-program'.
>
> * doc/guix.texi (Wrapping Code)[wrap-program]: Copy docstring from
>   guix/build/utils.scm and use Texinfo markup.

Neat!

>  doc/guix.texi | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> 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.

> +@lisp
> +  (wrap-program "foo"
> +                '("PATH" ":" = ("/gnu/.../bar/bin"))
> +                '("CERT_PATH" suffix ("/gnu/.../baz/certs"
> +                                        "/qux/certs")))
   ^^
You can remove indentation here too.

> +@end lisp
> +
> +will copy @file{foo} to @file{.foo-real} and create the file @file{foo} with
> +the following contents:
> +
> +@example
> +  #!location/of/bin/bash
> +  export PATH="/gnu/.../bar/bin"
> +  export CERT_PATH="$CERT_PATH$@{CERT_PATH:+:@}/gnu/.../baz/certs:/qux/certs"
> +  exec -a $0 location/of/.foo-real "$@@"

… and here.

This one can even go to master.

Thanks!

Ludo’.

Message #14 received at 47869 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 47869 <at> debbugs.gnu.org
Subject: Re: bug#47869: [PATCH core-updates]
 ‘which’ looks in PATH, incorrect when
 cross-compiling
Date: Tue, 18 May 2021 23:25:28 +0200

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

Message #17 received at 47869 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47869 <at> debbugs.gnu.org
Subject: Re: bug#47869: [PATCH core-updates] ‘which’
 looks in PATH, incorrect when cross-compiling
Date: Sat, 29 May 2021 16:50:53 +0200

Hi Maxime,

Maxime Devos <maximedevos <at> telenet.be> skribis:

> Ludovic Courtès schreef op di 18-05-2021 om 22:51 [+0200]:

[...]

>> 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?

Yes, it’s a good time for this!  For now, we can even afford world
rebuilds on ‘core-updates’.

>> > From e78d2d8651d5f56afa7d57be78c5cccccebb117a Mon Sep 17 00:00:00 2001
>> > From: Maxime Devos <maximedevos <at> 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.

Yes, and I agree that’s a difficulty.

>> >           (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.

Good point.  I think one could write:

  (wrap-script … #:guile (search-input-file inputs "/bin/guile"))

which is more reasonable.

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

Agreed, let’s have ‘search-input-file’ raise an exception if the file
isn’t found.

> For the few cases were the input binary is truly optional,
> we can define a 'search-optional-input-file' procedure.

Let’s ignore that until the problem shows up.

>> > -@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.

Yes.

Sorry for the delay, and thanks again!

Ludo’.

Message #20 received at 47869 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47869 <at> debbugs.gnu.org
Subject: [PATCH v3 core-updates] various cross-compilation fixes in
 guix/build/utils.scm
Date: Tue, 01 Jun 2021 21:53:17 +0200

[Message part 1 (text/plain, inline)]

Hi guix,

This is version three of the patch series,
which (no pun intended) incorporates feedback
from Ludovic Courtès.

This version adds a 'search-input-file' procedure
to (guix build utils). It is used like:

  (wrap-script something #:guile
    (search-input-file inputs "bin/guile")
    [...])

Explicitely setting #:guile instead of defaulting
to (which "guile") is required for cross-compilation,
to make sure the guile eventually used is compiled for
the correct architecture.

This patch series also extends 'wrap-program' with
a #:sh keyword argument, which has the same purpose
as #:guile for 'wrap-script'.

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.

For testing wrap-program:
Write to "a.sh":

  #!/stuff/etcetera
  echo "hello world!"

From ./pre-inst-env guix repl, do:

  (use-modules (guix build utils))
  (wrap-program "a.sh" #:sh "/bin/sh" '("PATH" = ("stuff")))

Now look at "a.sh":

  #!/bin/sh
  export PATH="stuff"
  exec -a "$0" "[current working directory]/.a.sh-real" "$@"

There are some tests in tests/build-utils.scm for 'search-input-file'.

I also ran "make && ./pre-inst-env guix build hello wireguard-tools".
(Not sure about which packages I tested actually.) This successfully
built "hello" (and all its dependencies, this can take a lot of time!).

Building wireguard-tools failed at first. It turned out I made a mistake
in 'wrap-program': the following ...

  (define vars/filtered
    (match vars
      ((#:sh . vars) vars)
      (vars vars)))

... should have been ...

 (define vars/filtered
    (match vars
      ((#:sh _ . vars) vars)
      (vars vars)))

That has been corrected. I tested the corrected "wrap-program" in a REPL
as above, but haven't tried building wireguard-tools again (that would
entail doing the whole bootstrapping process again).

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

Greetings,
Maxime.

[0001-build-Allow-overriding-the-shell-interpreter-in-wrap.patch (text/x-patch, attachment)]

[0002-build-Define-search-input-file-procedure.patch (text/x-patch, attachment)]

[0003-glib-or-gtk-build-system-Look-up-the-interpreter-in-.patch (text/x-patch, attachment)]

[0004-python-build-system-Look-up-the-interpreter-in-input.patch (text/x-patch, attachment)]

[0005-qt-build-system-Look-up-the-interpreter-in-inputs.patch (text/x-patch, attachment)]

[0006-rakudo-build-system-Look-up-the-interpreter-in-input.patch (text/x-patch, attachment)]

[0007-gnu-carla-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0008-gnu-bats-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0009-gnu-proteinortho-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0010-gnu-prinseq-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0011-gnu-gess-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0012-gnu-nanopolish-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0013-gnu-sieve-connect-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0014-gnu-clipmenu-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0015-gnu-vpnc-scripts-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0016-gnu-openconnect-sso-Set-sh-argument-of-wrap-program.patch (text/x-patch, attachment)]

[0017-gnu-protonvpn-cli-Set-sh-argument-of-wrap-program.patch (text/x-patch, attachment)]

[0018-gnu-wireguard-tools-Set-sh-argument-of-wrap-program.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #23 received at 47869 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47869 <at> debbugs.gnu.org
Subject: Re: bug#47869: [PATCH core-updates] ‘which’
 looks in PATH, incorrect when cross-compiling
Date: Tue, 01 Jun 2021 23:01:22 +0200

Hi Maxime,

Maxime Devos <maximedevos <at> 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 <at> 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 <at> 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 <at> 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 <at> 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 <at> 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 <at> 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’.

Message #26 received at 47869 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: 47869 <at> debbugs.gnu.org
Subject: [PATCH v4 core-updates] various cross-compilation fixes in
 guix/build/utils.scm
Date: Wed, 02 Jun 2021 09:56:11 +0200

[Message part 1 (text/plain, inline)]

Hi guix,

This is version 4 of the patch series.
It lets 'search-input-file' raise the new
&search-error exception instead of misc-error.

The documentation of 'search-input-file' has been
adjusted to give an example of how it can be used.

Also, there is one new test in tests/build-utils.scm
("search-input-file: can search in multiple directories").

Greetings,
Maxime.

[0001-build-Allow-overriding-the-shell-interpreter-in-wrap.patch (text/x-patch, attachment)]

[0002-build-Define-search-input-file-procedure.patch (text/x-patch, attachment)]

[0003-glib-or-gtk-build-system-Look-up-the-interpreter-in-.patch (text/x-patch, attachment)]

[0004-python-build-system-Look-up-the-interpreter-in-input.patch (text/x-patch, attachment)]

[0005-qt-build-system-Look-up-the-interpreter-in-inputs.patch (text/x-patch, attachment)]

[0006-rakudo-build-system-Look-up-the-interpreter-in-input.patch (text/x-patch, attachment)]

[0007-gnu-carla-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0008-gnu-bats-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0009-gnu-proteinortho-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0010-gnu-prinseq-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0011-gnu-gess-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0012-gnu-nanopolish-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0013-gnu-sieve-connect-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0014-gnu-clipmenu-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0015-gnu-vpnc-scripts-Set-guile-argument-of-wrap-script.patch (text/x-patch, attachment)]

[0016-gnu-openconnect-sso-Set-sh-argument-of-wrap-program.patch (text/x-patch, attachment)]

[0017-gnu-protonvpn-cli-Set-sh-argument-of-wrap-program.patch (text/x-patch, attachment)]

[0018-gnu-wireguard-tools-Set-sh-argument-of-wrap-program.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #31 received at 47869-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 47869-done <at> debbugs.gnu.org
Subject: Re: bug#47869: [PATCH core-updates] ‘which’
 looks in PATH, incorrect when cross-compiling
Date: Fri, 04 Jun 2021 23:31:28 +0200

Hi Maxime,

Maxime Devos <maximedevos <at> telenet.be> skribis:

> This is version 4 of the patch series.
> It lets 'search-input-file' raise the new
> &search-error exception instead of misc-error.
>
> The documentation of 'search-input-file' has been
> adjusted to give an example of how it can be used.
>
> Also, there is one new test in tests/build-utils.scm
> ("search-input-file: can search in multiple directories").

I pushed the whole series as d1827d5c636adb395153a4ed6064629ed5b7664b:

  d1827d5c63 * gnu: wireguard-tools: Set #:sh argument of 'wrap-program'.
  96a2ae40fb * gnu: protonvpn-cli: Set #:sh argument of 'wrap-program'.
  b74085ce36 * gnu: openconnect-sso: Set #:sh argument of 'wrap-program'.
  3bbb0ec888 * gnu: vpnc-scripts: Set #:guile argument of 'wrap-script'.
  a4e38cc216 * gnu: clipmenu: Set #:guile argument of 'wrap-script'.
  0758ee8002 * gnu: sieve-connect: Set #:guile argument of 'wrap-script'.
  b2459387b9 * gnu: nanopolish: Set #:guile argument of 'wrap-script'.
  2d092a2afa * gnu: gess: Set #:guile argument of 'wrap-script'.
  c4989f7569 * gnu: prinseq: Set #:guile argument of 'wrap-script'.
  fadbac0ecc * gnu: proteinortho: Set #:guile argument of 'wrap-script'.
  b202fdf131 * gnu: bats: Set #:guile argument of 'wrap-script'.
  0a843e3643 * gnu: carla: Set #:guile argument of 'wrap-script'.
  a62d17dc05 * rakudo-build-system: Look up the interpreter in 'inputs'.
  2ac898d7f8 * qt-build-system: Look up the interpreter in 'inputs'.
  5b24cbee31 * python-build-system: Look up the interpreter in 'inputs'.
  1dbc3b2b0c * glib-or-gtk-build-system: Look up the interpreter in 'inputs'.
  5378edeab4 * utils: Define ‘search-input-file’ procedure.
  8b0899963f * utils: Allow overriding the shell interpreter in ‘wrap-program’.

Thank you!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.