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