[bug#40032] [PATCH] gnu: Add nvme-cli

[bug#40032] [PATCH] gnu: Add nvme-cli

[Vincent Legoll]

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

Lightly tested (list & list-subsys commands)
on real hardware.

Guix pack'ed the binary and copied out of the
VM to run (in debian)

-- 
Vincent Legoll

[-- Attachment #2: 0001-gnu-Add-nvme-cli.patch --]
[-- Type: text/x-patch, Size: 1975 bytes --]

From 9c836b206d65c0e80706c0a3730e77f508b10f43 Mon Sep 17 00:00:00 2001
From: Vincent Legoll <vincent.legoll@gmail.com>
Date: Thu, 12 Mar 2020 01:51:12 +0100
Subject: [PATCH] gnu: Add nvme-cli

* gnu/packages/linux.scm (nvme-cli): New variable.
---
 gnu/packages/linux.scm | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index b6048a8cfb..d7b31103c8 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -3544,6 +3544,35 @@ IDE driver subsystem.  Many external USB drive enclosures with SCSI-ATA Command
 Translation (@dfn{SAT}) are also supported.")
     (license (license:non-copyleft "file://LICENSE.TXT"))))
 
+(define-public nvme-cli
+  (package
+    (name "nvme-cli")
+    (version "1.10.1")
+    (home-page "https://github.com/linux-nvme/nvme-cli")
+    (source (origin
+              (method git-fetch)
+              (uri (git-reference
+                    (url home-page)
+                    (commit (string-append "v" version))))
+              (sha256
+               (base32 "12wp2wxmsw2v8m9bhvwvdbhdgx1md8iilhbl19sfzz2araiwi2x8"))
+              (file-name (git-file-name name version))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:make-flags (list "CC=gcc")
+       #:phases (modify-phases %standard-phases
+                  (delete 'configure)
+                  (replace 'install
+                    (lambda _
+                      (zero? (system* "make" "install-spec" "PREFIX="
+                                      (string-append "DESTDIR=" %output))))))
+       #:tests? #f))
+    (synopsis "NVM-Express user space tooling for Linux")
+    (description "Utility to provide standards compliant tooling for NVM-Express
+drives.  It was made specifically for Linux as it relies on the IOCTLs defined
+by the mainline kernel driver.")
+    (license license:gpl2+)))
+
 (define-public rfkill
   (package
     (name "rfkill")
-- 
2.25.1

[bug#40032] [PATCH] gnu: Add nvme-cli

[Leo Famulari]

On Thu, Mar 12, 2020 at 02:13:06AM +0100, Vincent Legoll wrote:
> +       #:tests? #f))

Why are the tests skipped? Please, always add a code comment here for
reviewers.

[bug#40032] [PATCH] gnu: Add nvme-cli

[Vincent Legoll]

I'll investigate further and amend the patch

On Thu, Mar 12, 2020 at 7:15 PM Leo Famulari <leo@famulari.name> wrote:
>
> On Thu, Mar 12, 2020 at 02:13:06AM +0100, Vincent Legoll wrote:
> > +       #:tests? #f))
>
> Why are the tests skipped? Please, always add a code comment here for
> reviewers.



-- 
Vincent Legoll

[bug#40032] [PATCH] gnu: Add nvme-cli

[Vincent Legoll]

It looks like the tests require the presence of /sys/devices...

Should I just add that explanation as comment ?

On Sat, Mar 14, 2020 at 12:14 AM Vincent Legoll
<vincent.legoll@gmail.com> wrote:
>
> I'll investigate further and amend the patch
>
> On Thu, Mar 12, 2020 at 7:15 PM Leo Famulari <leo@famulari.name> wrote:
> >
> > On Thu, Mar 12, 2020 at 02:13:06AM +0100, Vincent Legoll wrote:
> > > +       #:tests? #f))
> >
> > Why are the tests skipped? Please, always add a code comment here for
> > reviewers.
>
>
>
> --
> Vincent Legoll



-- 
Vincent Legoll

[bug#40032] [PATCH] gnu: Add nvme-cli

[Leo Famulari]

On Sun, Mar 15, 2020 at 06:07:44PM +0100, Vincent Legoll wrote:
> It looks like the tests require the presence of /sys/devices...
> 
> Should I just add that explanation as comment ?

Yeah, I would say that they require sysfs, which is not accessible from
the build environment.

One could check with "ls -l /sys" in a build phase, or check the docs:

https://guix.gnu.org/manual/en/html_node/Build-Environment-Setup.html

Can you send a revised patch?

[bug#40032] [PATCH] gnu: Add nvme-cli

[Vincent Legoll]

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

Like the following ?

On Sun, Mar 15, 2020 at 6:22 PM Leo Famulari <leo@famulari.name> wrote:
>
> On Sun, Mar 15, 2020 at 06:07:44PM +0100, Vincent Legoll wrote:
> > It looks like the tests require the presence of /sys/devices...
> >
> > Should I just add that explanation as comment ?
>
> Yeah, I would say that they require sysfs, which is not accessible from
> the build environment.
>
> One could check with "ls -l /sys" in a build phase, or check the docs:
>
> https://guix.gnu.org/manual/en/html_node/Build-Environment-Setup.html
>
> Can you send a revised patch?



-- 
Vincent Legoll

[-- Attachment #2: 0001-gnu-Add-nvme-cli.patch --]
[-- Type: text/x-patch, Size: 2078 bytes --]

From 4f794f64ca5438773fc9980bbf0cf5739144dc87 Mon Sep 17 00:00:00 2001
From: Vincent Legoll <vincent.legoll@gmail.com>
Date: Thu, 12 Mar 2020 01:51:12 +0100
Subject: [PATCH] gnu: Add nvme-cli

* gnu/packages/linux.scm (nvme-cli): New variable.
---
 gnu/packages/linux.scm | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index 688d9eefaf..e8cc01d288 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -3577,6 +3577,36 @@ IDE driver subsystem.  Many external USB drive enclosures with SCSI-ATA Command
 Translation (@dfn{SAT}) are also supported.")
     (license (license:non-copyleft "file://LICENSE.TXT"))))
 
+(define-public nvme-cli
+  (package
+    (name "nvme-cli")
+    (version "1.10.1")
+    (home-page "https://github.com/linux-nvme/nvme-cli")
+    (source (origin
+              (method git-fetch)
+              (uri (git-reference
+                    (url home-page)
+                    (commit (string-append "v" version))))
+              (sha256
+               (base32 "12wp2wxmsw2v8m9bhvwvdbhdgx1md8iilhbl19sfzz2araiwi2x8"))
+              (file-name (git-file-name name version))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:make-flags (list "CC=gcc")
+       #:phases (modify-phases %standard-phases
+                  (delete 'configure)
+                  (replace 'install
+                    (lambda _
+                      (zero? (system* "make" "install-spec" "PREFIX="
+                                      (string-append "DESTDIR=" %output))))))
+       #:tests? #f)) ; The tests require sysfs, which is not accessible from
+                     ; the build environment
+    (synopsis "NVM-Express user space tooling for Linux")
+    (description "Utility to provide standards compliant tooling for NVM-Express
+drives.  It was made specifically for Linux as it relies on the IOCTLs defined
+by the mainline kernel driver.")
+    (license license:gpl2+)))
+
 (define-public rfkill
   (package
     (name "rfkill")
-- 
2.25.1

bug#40032: [PATCH] gnu: Add nvme-cli

[Leo Famulari]

On Sun, Mar 15, 2020 at 06:36:54PM +0100, Vincent Legoll wrote:
> From 4f794f64ca5438773fc9980bbf0cf5739144dc87 Mon Sep 17 00:00:00 2001
> From: Vincent Legoll <vincent.legoll@gmail.com>
> Date: Thu, 12 Mar 2020 01:51:12 +0100
> Subject: [PATCH] gnu: Add nvme-cli
> 
> * gnu/packages/linux.scm (nvme-cli): New variable.

Thanks! Pushed as 323841bda4e5b8f9b30626ab768aaf711ee6aabf with the
following changes that I somehow forgot to mention. There was no need to
make you revise the patch again...

> +       #:phases (modify-phases %standard-phases
> +                  (delete 'configure)

I added a comment about why the phase is deleted.

> +                  (replace 'install
> +                    (lambda _
> +                      (zero? (system* "make" "install-spec" "PREFIX="
> +                                      (string-append "DESTDIR=" %output))))))

I replaced (zero? (system* ...)) with (invoke ...), which raises
exceptions on failure rather than returning a boolean #f. This is "the
Guix way" for a while now:

https://lists.gnu.org/archive/html/guix-devel/2017-12/msg00278.html

> +    (description "Utility to provide standards compliant tooling for NVM-Express
> +drives.  It was made specifically for Linux as it relies on the IOCTLs defined

And I made this first sentence into a complete sentence.

[bug#40032] [PATCH] gnu: Add nvme-cli

[Vincent Legoll]

Thanks for all the fixes, I'll try to get better at doing that myself...

On Sun, Mar 15, 2020 at 6:57 PM Leo Famulari <leo@famulari.name> wrote:
>
> On Sun, Mar 15, 2020 at 06:36:54PM +0100, Vincent Legoll wrote:
> > From 4f794f64ca5438773fc9980bbf0cf5739144dc87 Mon Sep 17 00:00:00 2001
> > From: Vincent Legoll <vincent.legoll@gmail.com>
> > Date: Thu, 12 Mar 2020 01:51:12 +0100
> > Subject: [PATCH] gnu: Add nvme-cli
> >
> > * gnu/packages/linux.scm (nvme-cli): New variable.
>
> Thanks! Pushed as 323841bda4e5b8f9b30626ab768aaf711ee6aabf with the
> following changes that I somehow forgot to mention. There was no need to
> make you revise the patch again...
>
> > +       #:phases (modify-phases %standard-phases
> > +                  (delete 'configure)
>
> I added a comment about why the phase is deleted.
>
> > +                  (replace 'install
> > +                    (lambda _
> > +                      (zero? (system* "make" "install-spec" "PREFIX="
> > +                                      (string-append "DESTDIR=" %output))))))
>
> I replaced (zero? (system* ...)) with (invoke ...), which raises
> exceptions on failure rather than returning a boolean #f. This is "the
> Guix way" for a while now:
>
> https://lists.gnu.org/archive/html/guix-devel/2017-12/msg00278.html
>
> > +    (description "Utility to provide standards compliant tooling for NVM-Express
> > +drives.  It was made specifically for Linux as it relies on the IOCTLs defined
>
> And I made this first sentence into a complete sentence.



-- 
Vincent Legoll

[bug#40032] [PATCH] gnu: Add nvme-cli

[Leo Famulari]

On Sun, Mar 15, 2020 at 07:10:18PM +0100, Vincent Legoll wrote:
> Thanks for all the fixes, I'll try to get better at doing that myself...

No worries! We're glad to have your patches :)