bug#26112: [PATCH 2/7] gnu: Add niftilib.

[Ricardo Wurmus]

Hi John,

thanks for the contribution.

> * gnu/packages/image.scm (niftilib): New variable.

I realize this has been pushed already, but here are some comments for
the future.

> +
> +(define-public niftilib
> +  (package
> +   (name "niftilib")
> +   (version "2.0.0")
> +   (source (origin
> +            (method url-fetch)
> +            (uri (list (string-append "mirror://sourceforge/niftilib/"
> +                                      "nifticlib/nifticlib_2_0_0/"
> +                                      "/nifticlib-" version ".tar.gz")))
> +            (sha256
> +             (base32 
> "123z9bwzgin5y8gi5ni8j217k7n683whjsvg0lrpii9flgk8isd3"))))
> +   (build-system gnu-build-system)
> +   (arguments
> +    '(#:tests? #f
> +      #:parallel-build? #f

Please add simple comments explaining why this is required.

> +      #:phases
> +      (modify-phases %standard-phases
> +        (replace 'install
> +                 (lambda _
> +                   (for-each
> +                    (lambda (dir)
> +                      (let ((directory (assoc-ref %outputs "out")))
> +                        (mkdir-p (string-append directory "/" dir))
> +                        (zero? (system* "cp" "-a" dir directory))))
> +                    '("bin" "lib" "include"))))

Instead of starting a shell process to copy things please use
“copy-recursively” or “install-file” instead.  It’s also better style to
pull the let binding out of the lambda.

Finally, please end the phase on “#t”.

> +        (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")))
> +                   #t)))))

It is preferrable to use #:make-flags instead of patching the Makefile.
When referencing inputs, please use “lambda* (#:key inputs
#:allow-other-keys)” instead of “lambda _” and “%build-inputs”.

It would also be good to check the indentation, which can be done with
“etc/indent-code.el”.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net