Mark H Weaver <mhw@netris.org> writes:

> 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.
>

It was in fact posted for review. I reviewed it.

>> +(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.
>

I missed this somehow.

>> +            (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').
>

This is interesting. Which module contains the definition for the
'every' procedure?

>> +        (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

I should keep a closer eye on details like these. Thank you for the
second review.