bug#40698: [core-updates]: [PATCH v2] gnu: perl: Actually produce a host perl when cross-compiling.

[Jan Nieuwenhuizen <janneke@gnu.org>]

Marius Bakke writes:

Hello Marius,

>> Using this, I can cross-build perl and which allows me to ceate patches
>> to cross build, autoconf and automake that work to configure Guix git on
>> the Hurd.
>
> Wooow, awesome work (as usual)!
>
> Some feedback on the patch:

Wow, thanks!

>> -                (which "pwd")))
>> +                (which "pwd"))) ;TODO: fix cross-compile next rebuild cycle
>
> It might be clearer to add "TODO: use coreutils from INPUTS instead of
> 'which'" here, maybe mentioning the related substitution below.

Yes, changed now to

             ;; Use the right path for `pwd'.
             ;; TODO: use coreutils from INPUTS instead of 'which'
             ;; in next rebuild cycle, see fixup below.
             (substitute* "dist/PathTools/Cwd.pm"
               (("/bin/pwd")
                (which "pwd")))


>> +                     (let ((cross-checkout (assoc-ref %build-inputs "perl-cross")))
>> +                       (invoke "chmod" "-R" "+w" ".")
>
> Please use 'make-file-writable' instead of chmod.

Finally changed it to

                       (rename-file "Artistic" "Artistic.perl")
                       (rename-file "Copying" "Copying.perl")

>> +                       (copy-recursively cross-checkout "."))
>> +                     (let ((bash (assoc-ref %build-inputs "bash")))
>
> Use the scoped 'inputs' instead of the magical %build-inputs.

Ah, right!  There is the scoped `native-inputs' too.  I have always
missed that and been using %build-inputs instead.  Hmm.

>> +                       (substitute* '("ext/Errno/Errno_pm.PL")
>> +                         (( "\\$cpp < errno.c") "gcc -E errno.c")))
>
> Should $cpp not be replaced with 'g++'?

No, I don't think so.  The non-replaced value of $cpp is "gcc -E -P -", and
that breaks terribly; this substitution is only to remove -the `-P' and
input redirection.  I did change this to the somewhat nicer

                       (substitute* '("ext/Errno/Errno_pm.PL")
                         (("\\$cpp < errno.c") "$Config{cc} -E errno.c")))

and mentioned this with my patch sent to `perl-cross'.

>> +                     #t))
>> +                 (replace 'configure
...
>> +                         (("/gnu/store/[^/]*-bash-[^/]*") bash))
>
> This phase should add a let binding for (%store-directory) and refer to
> that instead of the literal /gnu/store strings (see e.g. 'git').

Ah...nice!  Done.

>> +                       (substitute* '("config.h")
>> +                         (("^#define OSNAME .*")
>> +                          (string-append "#define OSNAME \""
>> +                                         ,(if (hurd-target?) "GNU" "Linux")
>
> Would it make sense to upstream this?

Yes, I think so.  I created a patch, now added as `perl-cross.patch' too
and sent it upstream.

>> +                                         "\"\n"))
>> +                         (("^# HAS_NANOSLEEP") "/* #undef HAS_NANOSLEEP */")
>
> Is this substitution required on all cross-compilation targets?

Good question.  This is no longer necessary now that configure actually
detects the Hurd using gcc; togeether with my patch.

>> +                     ;; `make install' wants to install this; it wasn't built...
>> +                     (mkdir-p "cpan/Pod-Usage/blib/script")
>> +                     (with-output-to-file "cpan/Pod-Usage/blib/script/pod2text"
>> +                       (lambda _ (display "")))
>> +                     (with-output-to-file "cpan/Pod-Usage/blib/script/pod2usage"
>> +                       (lambda _ (display "")))
>> +                     (with-output-to-file "cpan/Pod-Checker/blib/script/podchecker"
>> +                       (lambda _ (display "")))
>> +                     (mkdir-p "cpan/Pod-Parser/blib/script")
>> +                     (with-output-to-file "cpan/Pod-Parser/blib/script/podselect"
>> +                       (lambda _ (display "")))
>
> Using '(call-with-output-file "foo" (const #t))' is clearer IMO.  Also
> consider using 'for-each' here.

Changed to

                     (with-directory-excursion "cpan"
                       (mkdir-p "Pod-Usage/blib/script")
                       (mkdir-p "Pod-Parser/blib/script")
                       (for-each (lambda (file)
                                   (call-with-output-file file
                                     (lambda (port) (display "" port))))
                                 '("Pod-Usage/blib/script/pod2text"
                                   "Pod-Usage/blib/script/pod2usage"
                                   "Pod-Checker/blib/script/podchecker"
                                   "Pod-Parser/blib/script/podselect")))

> Phew!  Thanks a lot for this, LGTM!

With these changes, pushed to core-updates as eaff60b35fed75c60d0db76c589e17d1500f60dd

Thanks a lot for your review, I was pretty certain that I missed some
things; but there were more things that I learned than I expected.
Also, a good question here and there is really helpful for me to improve
things or to do the right thing.

Greetings,
janneke

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