Re: 01/01: gnu: Add niftilib.

[Mark H Weaver <mhw@netris.org>]

john@darrington.wattle.id.au (John Darrington) writes:

> jmd pushed a commit to branch master
> in repository guix.
>
> commit 21122bd79e7f9b0b5349ffffe2c146bace7205dc
> Author: John Darrington <jmd@gnu.org>
> Date:   Tue Mar 7 07:59:21 2017 +0100
>
>     gnu: Add niftilib.
>     
>     * gnu/packages/image.scm (niftilib): New variable.

Did you post this for review?  Please see below for comments.

> +(define-public niftilib
> +  (package
> +   (name "niftilib")
> +   (version "2.0.0")
> +   (source (origin
> +            (method url-fetch)
> +            (uri (list (string-append "mirror://sourceforge/niftilib/"
> +                                      "nifticlib/nifticlib_"
> +                                      (string-join (string-split version #\.) "_")
> +                                      "/nifticlib-" version ".tar.gz")))

Omit the superfluous 'list' call above.

> +            (sha256
> +             (base32 "123z9bwzgin5y8gi5ni8j217k7n683whjsvg0lrpii9flgk8isd3"))))
> +   (build-system gnu-build-system)
> +   (arguments
> +    '(#:tests? #f
> +      #:parallel-build? #f
> +      #:phases
> +      (modify-phases %standard-phases
> +        (replace 'install

Is there a reason not to use the included "make install" target?  It
looks like it should work, if you pass appropriate settings for
INSTALL_{BIN,LIB,INC}_DIR in #:make-flags.

> +                 (lambda _
> +                   (for-each
> +                    (lambda (dir)
> +                      (let ((directory (assoc-ref %outputs "out")))

If you were going to keep the custom 'install' phase, then instead of
using %outputs, please accept the 'outputs' keyword argument and use
that.

> +                        (mkdir-p (string-append directory "/" dir))
> +                        (zero? (system* "cp" "-a" dir directory))))
> +                    '("bin" "lib" "include"))))

We have a 'copy-recursively' procedure that you could use here.  If you
were going to use "cp", then you should pay attention to its result code
so that failures are not silently ignored (by using 'every' instead of
'for-each').

> +        (replace 'configure
> +                 (lambda _
> +                   (substitute* "Makefile"
> +                     (("^SHELL[ \t]*=[ \t]*csh")
> +                      (string-append "SHELL = "
> +                                     (assoc-ref %build-inputs "bash")
> +                                     "/bin/sh"))
> +
> +                     (("^CFLAGS[ \t]*=[ \t]\\$\\(ANSI_FLAGS\\)")
> +                      "CFLAGS = $(ANSI_FLAGS) -fPIC")
> +
> +                     (("^ZLIB_INC[ \t]*=[ \t]*-I/usr/include")
> +                      (string-append "ZLIB_INC = -I"
> +                                     (assoc-ref %build-inputs "zlib")
> +                                     "/include"))
> +
> +                     (("^CP[ \t]*=[ \t]*cp")
> +                      (string-append "CP = "
> +                                     (assoc-ref %build-inputs "coreutils")
> +                                     "/bin/cp")))

Instead of patching the Makefile, it's preferable to simply pass these
settings in #:make-flags.  Also, within phase procedures, please accept
the 'inputs' and 'outputs' keyword arguments instead of using
%build-inputs and %outputs.  Finally, for purposes of Makefile settings,
SHELL can simply be set to "bash" or "sh", since it's in the PATH.  I'm
not sure why you changed the setting for CP.

      Mark