Subject: Re: bug#26730: [PATCH 1/2] gnu: bzip2: Use 'modify-phases' syntax.

From: Marius Bakke <[email-masked]>
Date: Mon, 15 May 2017 17:43:06 +0200

Christopher Baines <[email-masked]> writes:

> * gnu/packages/compression.scm (bzip2)[arguments]: Use 'modify-phases' syntax.

Thanks a lot for sorting this out! I only have a few nitpicks:

> +         (replace 'configure
> +           (lambda* (#:key target #:allow-other-keys)
> +             (if ,(%current-target-system)
> +                 ;; Cross-compilation: use the cross tools.
> +                 (substitute* (find-files "." "Makefile")
> +                   (("CC=.*$")
> +                    (string-append "CC = " target "-gcc\n"))
> +                   (("AR=.*$")
> +                    (string-append "AR = " target "-ar\n"))
> +                   (("RANLIB=.*$")
> +                    (string-append "RANLIB = " target "-ranlib\n"))
> +                   (("^all:(.*)test" _ prerequisites)
> +                    ;; Remove 'all' -> 'test' dependency.
> +                    (string-append "all:" prerequisites "\n"))))))

Noob question: What is returned here when (%current-target-system) is
false? Can we make it more explicit? We try to make sure all phases end
on a #t.

> +         (add-after 'install 'install-shared-lib
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let* ((out    (assoc-ref outputs "out"))
> +                    (libdir (string-append out "/lib")))
> +               (for-each (lambda (file)
> +                           (let ((base (basename file)))
> +                             (format #t "installing `~a' to `~a'~%"
> +                                     base libdir)
> +                             (copy-file file
> +                                        (string-append libdir "/" base))))
> +                         (find-files "." "^libbz2\\.so"))))))

Similarly, if you send an updated patch, can you add a #t at the end of
this phase, since "for-each" has an unspecified return value? Otherwise
I can do so in a follow-up commit.

Apart from these "added-value" nitpicks LGTM. Tricky one!