diff mbox

[2/3] gnu: Add wcslib

Message ID 1472584872-19300-2-git-send-email-jmd@gnu.org
State New
Headers show

Commit Message

John Darrington Aug. 30, 2016, 7:21 p.m. UTC
* gnu/packages/astronomy.scm (wcslib): New variable.
---
 gnu/packages/astronomy.scm | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Leo Famulari Aug. 30, 2016, 8:47 p.m. UTC | #1
On Tue, Aug 30, 2016 at 09:21:11PM +0200, John Darrington wrote:
> * gnu/packages/astronomy.scm (wcslib): New variable.

LGTM
Alex Kost Aug. 31, 2016, 6:42 a.m. UTC | #2
John Darrington (2016-08-30 22:21 +0300) wrote:

> * gnu/packages/astronomy.scm (wcslib): New variable.
> ---
>  gnu/packages/astronomy.scm | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
>
> diff --git a/gnu/packages/astronomy.scm b/gnu/packages/astronomy.scm
> index 881e549..53e86c8 100644
> --- a/gnu/packages/astronomy.scm
> +++ b/gnu/packages/astronomy.scm
> @@ -51,3 +51,30 @@ provides many advanced features for manipulating and filtering the information
>  in FITS files.")
>      (license (license:non-copyleft "file://License.txt"
>                            "See License.txt in the distribution."))))
> +
> +(define-public wcslib
> +  (package
> +    (name "wcslib")
> +    (version "5.15")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (string-append
> +             "ftp://ftp.atnf.csiro.au/pub/software/wcslib/" name "-" version ".tar.bz2"))

As for me, this line is too long, I would write:

          (uri (string-append
                "ftp://ftp.atnf.csiro.au/pub/software/wcslib/"
                name "-" version ".tar.bz2"))

> +       (sha256
> +        (base32 "1s2nig327g4bimd9xshlk11ww09a7mrjmsbpdcd8smsmn2kl1glb"))))
> +    (inputs
> +     `(("cfitsio" ,cfitsio)))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:phases (modify-phases %standard-phases
> +                  (add-before 'configure 'patch-/bin/sh
> +                    (lambda _
> +                      (substitute* "makedefs.in"
> +                        (("/bin/sh") "sh")))))))

This phase should end with #t.

> +    (home-page "http://www.atnf.csiro.au/people/mcalabre/WCS")
> +    (synopsis "An implementation of the FITS WCS standard")

I think we don't begin synopses with articles ("guix lint" should report
about it).

> +    (description "The FITS \"World Coordinate System\" (WCS) standard defines
> +keywords and usage that provide for the description of astronomical coordinate
> +systems in a FITS image header.")  
                                    ^^
Please remove trailing spaces

> +    (license license:lgpl3+)))
Alex Kost Sept. 12, 2016, 1:44 p.m. UTC | #3
Alex Kost (2016-08-31 09:42 +0300) wrote:

> John Darrington (2016-08-30 22:21 +0300) wrote:
>
>> * gnu/packages/astronomy.scm (wcslib): New variable.
>> ---
>>  gnu/packages/astronomy.scm | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>>
>> diff --git a/gnu/packages/astronomy.scm b/gnu/packages/astronomy.scm
>> index 881e549..53e86c8 100644
>> --- a/gnu/packages/astronomy.scm
>> +++ b/gnu/packages/astronomy.scm
>> @@ -51,3 +51,30 @@ provides many advanced features for manipulating and filtering the information
>>  in FITS files.")
>>      (license (license:non-copyleft "file://License.txt"
>>                            "See License.txt in the distribution."))))
>> +
>> +(define-public wcslib
>> +  (package
>> +    (name "wcslib")
>> +    (version "5.15")
>> +    (source
>> +     (origin
>> +       (method url-fetch)
>> +       (uri (string-append
>> +             "ftp://ftp.atnf.csiro.au/pub/software/wcslib/" name "-" version ".tar.bz2"))
>
> As for me, this line is too long, I would write:
>
>           (uri (string-append
>                 "ftp://ftp.atnf.csiro.au/pub/software/wcslib/"
>                 name "-" version ".tar.bz2"))
>
>> +       (sha256
>> +        (base32 "1s2nig327g4bimd9xshlk11ww09a7mrjmsbpdcd8smsmn2kl1glb"))))
>> +    (inputs
>> +     `(("cfitsio" ,cfitsio)))
>> +    (build-system gnu-build-system)
>> +    (arguments
>> +     `(#:phases (modify-phases %standard-phases
>> +                  (add-before 'configure 'patch-/bin/sh
>> +                    (lambda _
>> +                      (substitute* "makedefs.in"
>> +                        (("/bin/sh") "sh")))))))
>
> This phase should end with #t.

I've noticed that you didn't fix these things (long line and #t after
substitute*).  Could please do it next time :-)

The same for 'cfitsio' package.
John Darrington Sept. 12, 2016, 3:40 p.m. UTC | #4
On Mon, Sep 12, 2016 at 04:44:44PM +0300, Alex Kost wrote:

     I've noticed that you didn't fix these things (long line and #t after
     substitute*).  Could please do it next time :-)
     
     The same for 'cfitsio' package.
     
Done.

If this is important why doesn't guix build and/or guix lint check for it?
Alex Kost Sept. 13, 2016, 8:10 a.m. UTC | #5
John Darrington (2016-09-12 17:40 +0200) wrote:

> On Mon, Sep 12, 2016 at 04:44:44PM +0300, Alex Kost wrote:
>
>      I've noticed that you didn't fix these things (long line and #t after
>      substitute*).  Could please do it next time :-)
>
>      The same for 'cfitsio' package.
>
> Done.
>
> If this is important why doesn't guix build and/or guix lint check for it?

"guix lint" can't check if a phase should end with #t or not, it's up to
you check if it is needed.  The thing is: if a phase fails, it should
return false value, and if it succeeds, it should return non-false
value.  A returned value of 'substitute*' is unspecified, so here you
should add #t to the end of the phase.  It works without it, but I would
say it happens "by chance" because #<unspecified> is considered to be
non-false, but we shouldn't rely on it, so adding #t to such phases is
the right thing.

As for the long line, it is 89 chars long, while "guix lint" reports
about exceeding 90 chars (see 'report-long-line' in (guix scripts lint)
module).  BTW I think this is too loose, I would limit it to 80.

But your line could be easily shorten, as I wrote at
<http://lists.gnu.org/archive/html/guix-devel/2016-08/msg02070.html>,
so instead of the current long line:

       (uri (string-append
             "ftp://ftp.atnf.csiro.au/pub/software/wcslib/" name "-" version ".tar.bz2"))

it could be:

       (uri (string-append
             "ftp://ftp.atnf.csiro.au/pub/software/wcslib/"
             name "-" version ".tar.bz2"))

which fits any screen and thus is more readable I think.

I just felt a bit of a letdown that you ignored my comments and
didn't send a message why.
John Darrington Sept. 13, 2016, 12:44 p.m. UTC | #6
On Tue, Sep 13, 2016 at 11:10:57AM +0300, Alex Kost wrote:
     John Darrington (2016-09-12 17:40 +0200) wrote:
     
     > On Mon, Sep 12, 2016 at 04:44:44PM +0300, Alex Kost wrote:
     >
     >      I've noticed that you didn't fix these things (long line and #t after
     >      substitute*).  Could please do it next time :-)
     >
     >      The same for 'cfitsio' package.
     >
     > Done.
     >
     > If this is important why doesn't guix build and/or guix lint check for it?
     
     "guix lint" can't check if a phase should end with #t or not, it's up to
     you check if it is needed.  The thing is: if a phase fails, it should
     return false value, and if it succeeds, it should return non-false
     value.  A returned value of 'substitute*' is unspecified, so here you
     should add #t to the end of the phase.  It works without it, but I would
     say it happens "by chance" because #<unspecified> is considered to be
     non-false, but we shouldn't rely on it, so adding #t to such phases is
     the right thing.


1.  Presumably guix build somewhere calls something like:

   (if (run-phase x) (run-next-phase) (error))

We could change this to:

   (if (let ((result (run-phase x)))
          (if (unspecfied? result) (error "Result of phase is unspecified"))
           result)
        (run-next-phase)
	(error))

.... or something similar ...


2. It wouldn't be a bad idea to change subsitute* to return something.  For
   example, the number of substitutions performed.
     


     As for the long line, it is 89 chars long, while "guix lint" reports
     about exceeding 90 chars (see 'report-long-line' in (guix scripts lint)
     module).  BTW I think this is too loose, I would limit it to 80.
     
     But your line could be easily shorten, as I wrote at
     <http://lists.gnu.org/archive/html/guix-devel/2016-08/msg02070.html>,
     so instead of the current long line:
     
            (uri (string-append
                  "ftp://ftp.atnf.csiro.au/pub/software/wcslib/" name "-" version ".tar.bz2"))
     
     it could be:
     
            (uri (string-append
                  "ftp://ftp.atnf.csiro.au/pub/software/wcslib/"
                  name "-" version ".tar.bz2"))
     
     which fits any screen and thus is more readable I think.
     

I think all the lines are less than 80 right now aren't they?


     I just felt a bit of a letdown that you ignored my comments and
     didn't send a message why.
     

I didn't mean to shun you.   I appreciate the time and effort you took to 
review my code.

Unfortunately I find the Guix workflow awkward to manage so sometimes I
have omitted corrections which ought to have been made.   Part of my
problem is that currently (due to lack of many services) I cannot send or
receive mail from the machine on which I have GuixSD running.  Hopefully
this will change soon.

J'
diff mbox

Patch

diff --git a/gnu/packages/astronomy.scm b/gnu/packages/astronomy.scm
index 881e549..53e86c8 100644
--- a/gnu/packages/astronomy.scm
+++ b/gnu/packages/astronomy.scm
@@ -51,3 +51,30 @@  provides many advanced features for manipulating and filtering the information
 in FITS files.")
     (license (license:non-copyleft "file://License.txt"
                           "See License.txt in the distribution."))))
+
+(define-public wcslib
+  (package
+    (name "wcslib")
+    (version "5.15")
+    (source
+     (origin
+       (method url-fetch)
+       (uri (string-append
+             "ftp://ftp.atnf.csiro.au/pub/software/wcslib/" name "-" version ".tar.bz2"))
+       (sha256
+        (base32 "1s2nig327g4bimd9xshlk11ww09a7mrjmsbpdcd8smsmn2kl1glb"))))
+    (inputs
+     `(("cfitsio" ,cfitsio)))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:phases (modify-phases %standard-phases
+                  (add-before 'configure 'patch-/bin/sh
+                    (lambda _
+                      (substitute* "makedefs.in"
+                        (("/bin/sh") "sh")))))))
+    (home-page "http://www.atnf.csiro.au/people/mcalabre/WCS")
+    (synopsis "An implementation of the FITS WCS standard")
+    (description "The FITS \"World Coordinate System\" (WCS) standard defines
+keywords and usage that provide for the description of astronomical coordinate
+systems in a FITS image header.")  
+    (license license:lgpl3+)))