Subject: [bug#31999] [PATCH 2/7] gnu: Add ndctl.

From: Marius Bakke <[email-masked]>
Date: Mon, 30 Jul 2018 00:23:36 +0200

Pierre Neidhardt <[email-masked]> writes:

> * gnu/package/disk.scm (ndctl): New variable.

[...]

> +(define-public ndctl
> +  (package
> +    (name "ndctl")
> +    (version "60.3")

Version 61.2 is out.  Can you see if that still works?

> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://github.com/pmem/ndctl")
> +                    (commit (string-append "v" version))))
> +              (file-name (string-append name "-" version "-checkout"))
> +              (sha256
> +               (base32
> +                "0w19yh6f9skf5zy4bhdjlrn3wdx5xx9cq8j6h04cmw4nla6zj9ar"))))
> +    (build-system gnu-build-system)
> +    (native-inputs
> +     `(("asciidoc" ,asciidoc)
> +       ("automake" ,automake)
> +       ("autoconf" ,autoconf)
> +       ("bash-completion" ,bash-completion)

That's an unusual input.  Can you add a comment about why it's needed?

> +       ("docbook-xsl" ,docbook-xsl)
> +       ("libtool" ,libtool)
> +       ("libxml2" ,libxml2)
> +       ("pkg-config" ,pkg-config)
> +       ("xmlto" ,xmlto)
> +       ;; Required for offline docbook generation:
> +       ("which" ,which)))
> +    (inputs
> +     `(("eudev" ,eudev)
> +       ("json-c" ,json-c)
> +       ("kmod" ,kmod)
> +       ("util-linux" ,util-linux)))
> +     (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (add-after 'unpack 'autogen
> +           (lambda _
> +             (substitute* "autogen.sh"
> +               (("/bin/sh") (which "sh")))
> +             (substitute* "git-version-gen"
> +               (("/bin/sh") (which "sh")))
> +             (substitute* "git-version"
> +               (("/bin/bash") (which "bash")))
> +             (invoke "sh" "autogen.sh"))))

Same here: please use the existing bootstrap phase if possible, or
replace it.

> +       #:make-flags
> +       (let ((out (assoc-ref %outputs "out")))
> +         (list (string-append "BASH_COMPLETION_DIR=" out
> +                              "/share/bash-completion/completions")))))
> +    (home-page "https://github.com/pmem/ndctl")
> +    (synopsis "Manage the non-volatile memory device sub-system in the Linux kernel")
> +    (description
> +     "Utility library for managing the libnvdimm (non-volatile memory device)
> +sub-system in the Linux kernel.")

Please use full sentences in descriptions.  E.g. "This package provides
a ...".

> +    (license (list license:lgpl2.1 license:gpl2))))

Please add comments when the licensing situation is not clear cut.  I
had a look, and while COPYING says LGPL2.1, most of the source headers
were GPL2.

In that situation GPL2 "wins", so I'd write this as:

  ;; COPYING says LGPL2.1, but many source files are GPL2 so that's
  ;; the effective license.  Note that some files under ccan/ are
  ;; covered by BSD-3 or public domain, see the individual folders.
  (license license:gpl2)))