Re: 01/01: gnu: Add niftilib.

Re: 01/01: gnu: Add niftilib.

[Mark H Weaver]

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

Re: 01/01: gnu: Add niftilib.

[Kei Kebreau]

[-- Attachment #1: Type: text/plain, Size: 3916 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: 01/01: gnu: Add niftilib.

[Alex Kost]

Kei Kebreau (2017-03-19 17:03 -0400) wrote:

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

(srfi srfi-1)

You can read the documentation for 'every' in the Guile manual:
(info "(guile) SRFI-1 Searching")

-- 
Alex

Re: 01/01: gnu: Add niftilib.

[John Darrington]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

On Sun, Mar 19, 2017 at 04:42:54PM -0400, Mark H Weaver wrote:
     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.

Yes.  One person reviewed it before I pushed.   You are the second person to 
have reviewed it afterwards - making a total of three.    So it would seem
that the best way to get one's patches reviewed is to push them!

Thanks for the comments anyway.
     

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]