Re: 04/05: gnu: autoconf: Support cross-build.

Re: 04/05: gnu: autoconf: Support cross-build.

[Ludovic Courtès]

Hi!

guix-commits@gnu.org skribis:

> commit 6833403fb393a513c77aa3cb8fca7d57b87befe1
> Author: Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
> AuthorDate: Sat Apr 18 19:49:54 2020 +0200
>
>     gnu: autoconf: Support cross-build.
>     
>     Autoconf cannot be cross-built properly: it lacks the concept of
>     <tool>-for-build.  It runs the host `autom4te' (a perl script) during build.
>     
>     * gnu/packages/autotools.scm (autoconf)[inputs]: When cross-building, add perl
>     and m4.
>     [native-inputs]: when cross-building, use -for-build names.
>     [arguments]: When cross-building, add `fake-cross-build' phase to substitute
>     m4 and perl.

Thanks for fixing it!  Some comments:

> +    (inputs
> +     (if (%current-target-system)
> +         `(("perl" ,perl)
> +           ("m4" ,m4))
> +         '()))
>      (native-inputs
> -     `(("perl" ,perl)
> -       ("m4" ,m4)))
> +     (if (%current-target-system)
> +         `(("perl-for-build" ,perl)
> +           ("m4-for-build" ,m4))
> +         `(("perl" ,perl)
> +           ("m4" ,m4))))

You can remove the ‘if’ in both cases: we always need Perl/M4 both as a
native input and as an input in both cases.

However, that might trigger a rebuild, so perhaps you’ll have to leave
the ifs, but with a TODO telling to remove it on the next rebuild or
Marius will be mad at us.

> +    (arguments
> +     `(#:tests? #f

Nope.  :-)

> +       ,@(if (%current-target-system)
> +             `(#:phases
> +               (modify-phases %standard-phases
> +                 ;; Autoconf cannot be cross-built properly: it lacks the
> +                 ;; concept of <tool>-for-build.  It even runs the host
> +                 ;; `autom4te' (a perl script) during build.
> +                 (add-after 'install 'fake-cross-build
> +                   (lambda* (#:key build inputs outputs #:allow-other-keys)
> +                     (let ((m4 (assoc-ref inputs "m4"))
> +                           (perl (assoc-ref inputs "perl"))
> +                           (out  (assoc-ref outputs "out")))
> +                      (substitute* (find-files (string-append out "/bin"))
> +                        (("/gnu/store/[^/]*-m4-[^/]*") m4)
> +                        (("/gnu/store/[^/]*-perl-[^/]*") perl))
> +                      #t)))))

Why is this needed?  The ‘patch-shebangs’ phase normally takes the
inputs, not the native inputs, when changing shebangs.

(You previously found that something’s wrong there, but I forgot what…)

Thanks,
Ludo’.

Re: 04/05: gnu: autoconf: Support cross-build.

[Jan Nieuwenhuizen]

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

Ludovic Courtès writes:

Hello,

>>     gnu: autoconf: Support cross-build.

> Thanks for fixing it!  Some comments:

>> +    (inputs
>> +     (if (%current-target-system)
>> +         `(("perl" ,perl)
>> +           ("m4" ,m4))
>> +         '()))
>>      (native-inputs
>> -     `(("perl" ,perl)
>> -       ("m4" ,m4)))
>> +     (if (%current-target-system)
>> +         `(("perl-for-build" ,perl)
>> +           ("m4-for-build" ,m4))
>> +         `(("perl" ,perl)
>> +           ("m4" ,m4))))
>
> You can remove the ‘if’ in both cases: we always need Perl/M4 both as a
> native input and as an input in both cases.

...yes

> However, that might trigger a rebuild, so perhaps you’ll have to leave
> the ifs, but with a TODO telling to remove it on the next rebuild or
> Marius will be mad at us.

Indeed; so changed to:

    (inputs
     ;; TODO: remove `if' in the next rebuild cycle.
     (if (%current-target-system)
         `(("perl" ,perl)
           ("m4" ,m4))
         '()))
    (native-inputs
     `(("perl" ,perl)
       ("m4" ,m4)))


>> +    (arguments
>> +     `(#:tests? #f
>
> Nope.  :-)

Ahum, yes, well...this made me blush for a moment ("I should not rely on
a review by Ludovic for catching such a development hack") but look at
the three preceeding lines:

     ;; XXX: testsuite: 209 and 279 failed. The latter is an impurity. It
     ;; should use our own "cpp" instead of "/lib/cpp".
-    (arguments `(#:tests? #f))

Tests are disabled currently (even on master!), so I guess that
inserting this line break is fine :-)

>> +       ,@(if (%current-target-system)
>> +             `(#:phases
>> +               (modify-phases %standard-phases
>> +                 ;; Autoconf cannot be cross-built properly: it lacks the
>> +                 ;; concept of <tool>-for-build.  It even runs the host
>> +                 ;; `autom4te' (a perl script) during build.
>> +                 (add-after 'install 'fake-cross-build
>> +                   (lambda* (#:key build inputs outputs #:allow-other-keys)
>> +                     (let ((m4 (assoc-ref inputs "m4"))
>> +                           (perl (assoc-ref inputs "perl"))
>> +                           (out  (assoc-ref outputs "out")))
>> +                      (substitute* (find-files (string-append out "/bin"))
>> +                        (("/gnu/store/[^/]*-m4-[^/]*") m4)
>> +                        (("/gnu/store/[^/]*-perl-[^/]*") perl))
>> +                      #t)))))
>
> Why is this needed?  The ‘patch-shebangs’ phase normally takes the
> inputs, not the native inputs, when changing shebangs.

Because it's not only the shebangs...but

Good question...especially because it teaches me about patch-shebangs!
Also, I failed to determine exactly what went wrong.  Without m4, perl
in INPUTS, the shebangs are wrong (obviously).

After adding m4,perl in INPUTS, the shebangs are indeed correct.  I
hadn't noticed that before, because look:

--8<---------------cut here---------------start------------->8---
$ head $(./pre-inst-env guix build --target=i586-pc-gnu autoconf)/bin/autoheader
#!/gnu/store/ drz7805gcsrqkgr8v43r1f7zydlsxh05-perl-5.30.2/bin/perl
# -*- Perl -*-
# Generated from autoheader.in; do not edit by hand.

eval 'case $# in 0) exec /gnu/store/8zvc5mvk0xm3ygrxsgpyy5ilxb5rzjry-perl-5.30.2/bin/perl -S "$0";; *) exec /gnu/store/8zvc5mvk0xm3ygrxsgpyy5ilxb5rzjry-perl-5.30.2/bin/perl -S "$0" "$@";; esac'
    if 0;
--8<---------------cut here---------------end--------------->8---

shebangs correct, re-execute EVALs: wrong.

I have now changed this bit to:

       ,@(if (%current-target-system)
             `(#:phases
               (modify-phases %standard-phases
                 ;; `patch-shebangs' patches shebangs only, and the Perl
                 ;; scripts use a re-exec feature that references the build
                 ;; hosts' perl.  Also, M4 store references hide in the
                 ;; scripts.
                 (add-after 'install 'patch-non-shebang-references
                   (lambda* (#:key build inputs outputs #:allow-other-keys)
                     (let ((m4 (assoc-ref inputs "m4"))
                           (perl (assoc-ref inputs "perl"))
                           (out  (assoc-ref outputs "out"))
                           (store-directory (%store-directory)))
                      (substitute* (find-files (string-append out "/bin"))
                        (((string-append store-directory "/[^/]*-m4-[^/]*")) m4)
                        (((string-append store-directory "/[^/]*-perl-[^/]*"))
                         perl))
                      #t)))))

> (You previously found that something’s wrong there, but I forgot what…)

(yes, and I did not have the whole story)

Attaching the updated patch in full.

(automake is pretty similar, i'll send an updated patch for that right
after this review is finished).

Greetings,
janneke


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-autoconf-Support-cross-build.patch --]
[-- Type: text/x-patch, Size: 2682 bytes --]

From 3d776e0077d62000f20d23a1b42b32fef718a503 Mon Sep 17 00:00:00 2001
From: "Jan (janneke) Nieuwenhuizen" <janneke@gnu.org>
Date: Sat, 18 Apr 2020 19:49:54 +0200
Subject: [PATCH] gnu: autoconf: Support cross-build.

Autoconf cannot be cross-built properly: it lacks the concept of
<tool>-for-build.  It runs the host `autom4te' (a perl script) during build.

* gnu/packages/autotools.scm (autoconf)[inputs]: When cross-building, add perl
and m4.
[native-inputs]: when cross-building, use -for-build names.
[arguments]: When cross-building, add `fake-cross-build' phase to substitute
m4 and perl.
---
 gnu/packages/autotools.scm | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/autotools.scm b/gnu/packages/autotools.scm
index 99ca52730e..fa3ced182f 100644
--- a/gnu/packages/autotools.scm
+++ b/gnu/packages/autotools.scm
@@ -55,12 +55,38 @@
        (base32
         "113nlmidxy9kjr45kg9x3ngar4951mvag1js2a3j8nxcz34wxsv4"))))
     (build-system gnu-build-system)
+    (inputs
+     ;; TODO: remove `if' in the next rebuild cycle.
+     (if (%current-target-system)
+         `(("perl" ,perl)
+           ("m4" ,m4))
+         '()))
     (native-inputs
      `(("perl" ,perl)
        ("m4" ,m4)))
     ;; XXX: testsuite: 209 and 279 failed. The latter is an impurity. It
     ;; should use our own "cpp" instead of "/lib/cpp".
-    (arguments `(#:tests? #f))
+    (arguments
+     `(#:tests? #f
+       ,@(if (%current-target-system)
+             `(#:phases
+               (modify-phases %standard-phases
+                 ;; `patch-shebangs' patches shebangs only, and the Perl
+                 ;; scripts use a re-exec feature that references the build
+                 ;; hosts' perl.  Also, M4 store references hide in the
+                 ;; scripts.
+                 (add-after 'install 'patch-non-shebang-references
+                   (lambda* (#:key build inputs outputs #:allow-other-keys)
+                     (let ((m4 (assoc-ref inputs "m4"))
+                           (perl (assoc-ref inputs "perl"))
+                           (out  (assoc-ref outputs "out"))
+                           (store-directory (%store-directory)))
+                      (substitute* (find-files (string-append out "/bin"))
+                        (((string-append store-directory "/[^/]*-m4-[^/]*")) m4)
+                        (((string-append store-directory "/[^/]*-perl-[^/]*"))
+                         perl))
+                      #t)))))
+             '())))
     (home-page "https://www.gnu.org/software/autoconf/")
     (synopsis "Create source code configuration scripts")
     (description
-- 
2.26.0


[-- Attachment #3: Type: text/plain, Size: 152 bytes --]


-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com

Re: 04/05: gnu: autoconf: Support cross-build.

[Jan Nieuwenhuizen]

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

Jan Nieuwenhuizen writes:

(fix typo/update renamed phase in commit message)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-autoconf-Support-cross-build.patch --]
[-- Type: text/x-patch, Size: 2694 bytes --]

From 2b7ae7542fd77b35d7a143c90556cf32a7f9ae48 Mon Sep 17 00:00:00 2001
From: "Jan (janneke) Nieuwenhuizen" <janneke@gnu.org>
Date: Sat, 18 Apr 2020 19:49:54 +0200
Subject: [PATCH] gnu: autoconf: Support cross-build.

Autoconf cannot be cross-built properly: it lacks the concept of
<tool>-for-build.  It runs the host `autom4te' (a perl script) during build.

* gnu/packages/autotools.scm (autoconf)[inputs]: When cross-building, add perl
and m4.
[native-inputs]: when cross-building, use -for-build names.
[arguments]: When cross-building, add `patch-non-shebang-references' phase to
substitute m4 and perl.
---
 gnu/packages/autotools.scm | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/autotools.scm b/gnu/packages/autotools.scm
index 99ca52730e..fa3ced182f 100644
--- a/gnu/packages/autotools.scm
+++ b/gnu/packages/autotools.scm
@@ -55,12 +55,38 @@
        (base32
         "113nlmidxy9kjr45kg9x3ngar4951mvag1js2a3j8nxcz34wxsv4"))))
     (build-system gnu-build-system)
+    (inputs
+     ;; TODO: remove `if' in the next rebuild cycle.
+     (if (%current-target-system)
+         `(("perl" ,perl)
+           ("m4" ,m4))
+         '()))
     (native-inputs
      `(("perl" ,perl)
        ("m4" ,m4)))
     ;; XXX: testsuite: 209 and 279 failed. The latter is an impurity. It
     ;; should use our own "cpp" instead of "/lib/cpp".
-    (arguments `(#:tests? #f))
+    (arguments
+     `(#:tests? #f
+       ,@(if (%current-target-system)
+             `(#:phases
+               (modify-phases %standard-phases
+                 ;; `patch-shebangs' patches shebangs only, and the Perl
+                 ;; scripts use a re-exec feature that references the build
+                 ;; hosts' perl.  Also, M4 store references hide in the
+                 ;; scripts.
+                 (add-after 'install 'patch-non-shebang-references
+                   (lambda* (#:key build inputs outputs #:allow-other-keys)
+                     (let ((m4 (assoc-ref inputs "m4"))
+                           (perl (assoc-ref inputs "perl"))
+                           (out  (assoc-ref outputs "out"))
+                           (store-directory (%store-directory)))
+                      (substitute* (find-files (string-append out "/bin"))
+                        (((string-append store-directory "/[^/]*-m4-[^/]*")) m4)
+                        (((string-append store-directory "/[^/]*-perl-[^/]*"))
+                         perl))
+                      #t)))))
+             '())))
     (home-page "https://www.gnu.org/software/autoconf/")
     (synopsis "Create source code configuration scripts")
     (description
-- 
2.26.0


[-- Attachment #3: Type: text/plain, Size: 152 bytes --]


-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com

Re: 04/05: gnu: autoconf: Support cross-build.

[Ludovic Courtès]

Hullo!

Jan Nieuwenhuizen <janneke@gnu.org> skribis:

>>> +    (arguments
>>> +     `(#:tests? #f
>>
>> Nope.  :-)
>
> Ahum, yes, well...this made me blush for a moment ("I should not rely on
> a review by Ludovic for catching such a development hack") but look at
> the three preceeding lines:
>
>      ;; XXX: testsuite: 209 and 279 failed. The latter is an impurity. It
>      ;; should use our own "cpp" instead of "/lib/cpp".
> -    (arguments `(#:tests? #f))
>
> Tests are disabled currently (even on master!), so I guess that
> inserting this line break is fine :-)

Damn it, this is terrible!  But not your fault, for sure.  :-)

>>> +       ,@(if (%current-target-system)
>>> +             `(#:phases
>>> +               (modify-phases %standard-phases
>>> +                 ;; Autoconf cannot be cross-built properly: it lacks the
>>> +                 ;; concept of <tool>-for-build.  It even runs the host
>>> +                 ;; `autom4te' (a perl script) during build.
>>> +                 (add-after 'install 'fake-cross-build
>>> +                   (lambda* (#:key build inputs outputs #:allow-other-keys)
>>> +                     (let ((m4 (assoc-ref inputs "m4"))
>>> +                           (perl (assoc-ref inputs "perl"))
>>> +                           (out  (assoc-ref outputs "out")))
>>> +                      (substitute* (find-files (string-append out "/bin"))
>>> +                        (("/gnu/store/[^/]*-m4-[^/]*") m4)
>>> +                        (("/gnu/store/[^/]*-perl-[^/]*") perl))
>>> +                      #t)))))
>>
>> Why is this needed?  The ‘patch-shebangs’ phase normally takes the
>> inputs, not the native inputs, when changing shebangs.
>
> Because it's not only the shebangs...but
>
> Good question...especially because it teaches me about patch-shebangs!
> Also, I failed to determine exactly what went wrong.  Without m4, perl
> in INPUTS, the shebangs are wrong (obviously).
>
> After adding m4,perl in INPUTS, the shebangs are indeed correct.  I
> hadn't noticed that before, because look:
>
> $ head $(./pre-inst-env guix build --target=i586-pc-gnu autoconf)/bin/autoheader
> #!/gnu/store/ drz7805gcsrqkgr8v43r1f7zydlsxh05-perl-5.30.2/bin/perl
> # -*- Perl -*-
> # Generated from autoheader.in; do not edit by hand.
>
> eval 'case $# in 0) exec /gnu/store/8zvc5mvk0xm3ygrxsgpyy5ilxb5rzjry-perl-5.30.2/bin/perl -S "$0";; *) exec /gnu/store/8zvc5mvk0xm3ygrxsgpyy5ilxb5rzjry-perl-5.30.2/bin/perl -S "$0" "$@";; esac'
>     if 0;
>
> shebangs correct, re-execute EVALs: wrong.

Oh, got it.

> From 3d776e0077d62000f20d23a1b42b32fef718a503 Mon Sep 17 00:00:00 2001
> From: "Jan (janneke) Nieuwenhuizen" <janneke@gnu.org>
> Date: Sat, 18 Apr 2020 19:49:54 +0200
> Subject: [PATCH] gnu: autoconf: Support cross-build.
>
> Autoconf cannot be cross-built properly: it lacks the concept of
> <tool>-for-build.  It runs the host `autom4te' (a perl script) during build.
>
> * gnu/packages/autotools.scm (autoconf)[inputs]: When cross-building, add perl
> and m4.
> [native-inputs]: when cross-building, use -for-build names.
> [arguments]: When cross-building, add `fake-cross-build' phase to substitute
> m4 and perl.
> ---
>  gnu/packages/autotools.scm | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/packages/autotools.scm b/gnu/packages/autotools.scm
> index 99ca52730e..fa3ced182f 100644
> --- a/gnu/packages/autotools.scm
> +++ b/gnu/packages/autotools.scm
> @@ -55,12 +55,38 @@
>         (base32
>          "113nlmidxy9kjr45kg9x3ngar4951mvag1js2a3j8nxcz34wxsv4"))))
>      (build-system gnu-build-system)
> +    (inputs
> +     ;; TODO: remove `if' in the next rebuild cycle.
> +     (if (%current-target-system)
> +         `(("perl" ,perl)
> +           ("m4" ,m4))
> +         '()))
>      (native-inputs
>       `(("perl" ,perl)
>         ("m4" ,m4)))
>      ;; XXX: testsuite: 209 and 279 failed. The latter is an impurity. It
>      ;; should use our own "cpp" instead of "/lib/cpp".
> -    (arguments `(#:tests? #f))
> +    (arguments
> +     `(#:tests? #f

I’m really nitpicking, but could you move the XXX comment right after
#:tests? #f?

> +       ,@(if (%current-target-system)
> +             `(#:phases
> +               (modify-phases %standard-phases
> +                 ;; `patch-shebangs' patches shebangs only, and the Perl
> +                 ;; scripts use a re-exec feature that references the build
> +                 ;; hosts' perl.  Also, M4 store references hide in the
> +                 ;; scripts.
> +                 (add-after 'install 'patch-non-shebang-references
> +                   (lambda* (#:key build inputs outputs #:allow-other-keys)
> +                     (let ((m4 (assoc-ref inputs "m4"))
> +                           (perl (assoc-ref inputs "perl"))
> +                           (out  (assoc-ref outputs "out"))
> +                           (store-directory (%store-directory)))
> +                      (substitute* (find-files (string-append out "/bin"))
> +                        (((string-append store-directory "/[^/]*-m4-[^/]*")) m4)
> +                        (((string-append store-directory "/[^/]*-perl-[^/]*"))
> +                         perl))

I’m very much in favor of keeping regexps literal.  What about writing
them as:

  (substitute* …
    (("/[[:graph:]]/bin/m4")
     (string-append m4 "/bin/m4"))
    …)

?

Also please move the comment right below ‘lambda*’.  :-)

And then I think you can push it to ‘core-updates’.

Thank you!

Ludo’.

Re: 04/05: gnu: autoconf: Support cross-build.

[Jan Nieuwenhuizen]

Ludovic Courtès writes:

Hi!

>> Tests are disabled currently (even on master!), so I guess that
>> inserting this line break is fine :-)
>
> Damn it, this is terrible!  But not your fault, for sure.  :-)

Yes, it would be good to re-enable them during the next cycle.
>> -    (arguments `(#:tests? #f))
>> +    (arguments
>> +     `(#:tests? #f
>
> I’m really nitpicking, but could you move the XXX comment right after
> #:tests? #f?

As checkn on IRC: yes, right after "(arguments `(" right before #:tests

>> +                           (out  (assoc-ref outputs "out"))
>> +                           (store-directory (%store-directory)))
>> +                      (substitute* (find-files (string-append out "/bin"))
>> +                        (((string-append store-directory "/[^/]*-m4-[^/]*")) m4)
>> +                        (((string-append store-directory "/[^/]*-perl-[^/]*"))
>> +                         perl))
>
> I’m very much in favor of keeping regexps literal.  What about writing
> them as:
>
>   (substitute* …
>     (("/[[:graph:]]/bin/m4")
>      (string-append m4 "/bin/m4"))
>     …)

After discussing various options on IRC: keeping the parenthetical
(((string-append stor-directory...; we are using than for git and
perl, see https://debbugs.gnu.org/cgi/bugreport.cgi?bug=40698#16

> Also please move the comment right below ‘lambda*’.  :-)

Done!

> And then I think you can push it to ‘core-updates’.

Pushed to core-updates, together with a simalar patch for automake,
and a trivial one for texinfo.

Thanks!
janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com