unofficial mirror of guix-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: 01/01: gnu: Add niftilib.
       [not found] ` <20170318142617.B58A320DF8@vcs0.savannah.gnu.org>
@ 2017-03-19 20:42   ` Mark H Weaver
  2017-03-19 21:03     ` Kei Kebreau
  2017-03-20  5:57     ` John Darrington
  0 siblings, 2 replies; 4+ messages in thread
From: Mark H Weaver @ 2017-03-19 20:42 UTC (permalink / raw)
  To: John Darrington; +Cc: guix-devel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: 01/01: gnu: Add niftilib.
  2017-03-19 20:42   ` 01/01: gnu: Add niftilib Mark H Weaver
@ 2017-03-19 21:03     ` Kei Kebreau
  2017-03-20 10:56       ` Alex Kost
  2017-03-20  5:57     ` John Darrington
  1 sibling, 1 reply; 4+ messages in thread
From: Kei Kebreau @ 2017-03-19 21:03 UTC (permalink / raw)
  To: Mark H Weaver; +Cc: guix-devel, John Darrington

[-- 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 --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: 01/01: gnu: Add niftilib.
  2017-03-19 20:42   ` 01/01: gnu: Add niftilib Mark H Weaver
  2017-03-19 21:03     ` Kei Kebreau
@ 2017-03-20  5:57     ` John Darrington
  1 sibling, 0 replies; 4+ messages in thread
From: John Darrington @ 2017-03-20  5:57 UTC (permalink / raw)
  To: Mark H Weaver; +Cc: guix-devel, 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 --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: 01/01: gnu: Add niftilib.
  2017-03-19 21:03     ` Kei Kebreau
@ 2017-03-20 10:56       ` Alex Kost
  0 siblings, 0 replies; 4+ messages in thread
From: Alex Kost @ 2017-03-20 10:56 UTC (permalink / raw)
  To: Kei Kebreau; +Cc: guix-devel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-03-20 10:56 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20170318142616.19421.55332@vcs0.savannah.gnu.org>
     [not found] ` <20170318142617.B58A320DF8@vcs0.savannah.gnu.org>
2017-03-19 20:42   ` 01/01: gnu: Add niftilib Mark H Weaver
2017-03-19 21:03     ` Kei Kebreau
2017-03-20 10:56       ` Alex Kost
2017-03-20  5:57     ` John Darrington

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).