[PATCH] gnu: Add mcelog.

[PATCH] gnu: Add mcelog.

[Tobias Geerinckx-Rice]

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

diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
index 3ec6514..fc4faa4 100644
--- a/gnu/packages/linux.scm
+++ b/gnu/packages/linux.scm
@@ -74,6 +74,7 @@
   #:use-module (guix build-system python)
   #:use-module (guix build-system trivial)
   #:use-module (guix download)
+  #:use-module (guix git-download)
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (guix packages)
   #:use-module (guix utils)
@@ -2939,3 +2940,43 @@ the default @code{nsswitch} and the experimental @code{umich_ldap}.")
      "Tools for loading and managing Linux kernel modules, such as `modprobe',
 `insmod', `lsmod', and more.")
     (license license:gpl2+)))
+
+(define-public mcelog
+  (package
+    (name "mcelog")
+    (version "141")
+    (source
+     (let ((commit (string-append "v" version)))
+       (origin
+         (method git-fetch)
+         (uri (git-reference
+               (url "https://git.kernel.org/pub/scm/utils/cpu/mce/mcelog.git")
+               (commit commit)))
+         (sha256
+          (base32
+           "0iqvqiwf3aawmgjcyg2rj427m8nvfbfnmmfv0606nhr59l14h5jr"))
+         (file-name (string-append name "-" version))
+         (modules '((guix build utils)))
+         (snippet
+          ;; Hard-code version to avoid a git (and .git/) build dependency.
+          `(substitute* "Makefile"
+             (("\"unknown\"") (string-append "\"" ,commit "\"")))))))
+    (build-system gnu-build-system)
+    (arguments
+     `(#:phases (modify-phases %standard-phases
+                  (delete 'configure))  ; no configure script
+                #:make-flags (list "CC=gcc"
+                                   (string-append "DESTDIR="
+                                                  (assoc-ref %outputs "out"))
+                                   "prefix="
+                                   "DOCDIR=/share/doc/mcelog"
+                                   "etcprefix=$(DOCDIR)/examples")
+                #:tests? #f))           ; tests must be run as root
+    (home-page "http://mcelog.org/")
+    (synopsis "Machine check monitor for x86 Linux systems")
+    (description
+     "The mcelog daemon is required by the Linux kernel to log memory, I/O,
+      CPU, and other hardware errors on x86 systems.  It can also perform
+      user-defined tasks, such as bringing bad pages off-line, when
+      configurable error thresholds are exceeded.")
+    (license license:gpl2)))
-- 
2.7.4

Re: [PATCH] gnu: Add mcelog.

[Marius Bakke]

Tobias Geerinckx-Rice <me@tobias.gr> writes:

> * gnu/packages/linux.scm (mcelog): New variable.

Thanks!

> ---
>  gnu/packages/linux.scm | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
> index 3ec6514..fc4faa4 100644
> --- a/gnu/packages/linux.scm
> +++ b/gnu/packages/linux.scm
> @@ -74,6 +74,7 @@
>    #:use-module (guix build-system python)
>    #:use-module (guix build-system trivial)
>    #:use-module (guix download)
> +  #:use-module (guix git-download)
>    #:use-module ((guix licenses) #:prefix license:)
>    #:use-module (guix packages)
>    #:use-module (guix utils)
> @@ -2939,3 +2940,43 @@ the default @code{nsswitch} and the experimental @code{umich_ldap}.")
>       "Tools for loading and managing Linux kernel modules, such as `modprobe',
>  `insmod', `lsmod', and more.")
>      (license license:gpl2+)))
> +
> +(define-public mcelog
> +  (package
> +    (name "mcelog")
> +    (version "141")
> +    (source
> +     (let ((commit (string-append "v" version)))
> +       (origin
> +         (method git-fetch)
> +         (uri (git-reference
> +               (url "https://git.kernel.org/pub/scm/utils/cpu/mce/mcelog.git")
> +               (commit commit)))

It's not visible in the cgit interface, but it actually seems to support
normal snapshot downloads:
https://git.kernel.org/cgit/utils/cpu/mce/mcelog.git/snapshot/v141.tar.gz

> +         (sha256
> +          (base32
> +           "0iqvqiwf3aawmgjcyg2rj427m8nvfbfnmmfv0606nhr59l14h5jr"))
> +         (file-name (string-append name "-" version))
> +         (modules '((guix build utils)))
> +         (snippet
> +          ;; Hard-code version to avoid a git (and .git/) build dependency.
> +          `(substitute* "Makefile"
> +             (("\"unknown\"") (string-append "\"" ,commit "\"")))))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:phases (modify-phases %standard-phases
> +                  (delete 'configure))  ; no configure script
> +                #:make-flags (list "CC=gcc"
> +                                   (string-append "DESTDIR="
> +                                                  (assoc-ref %outputs "out"))
> +                                   "prefix="
> +                                   "DOCDIR=/share/doc/mcelog"
> +                                   "etcprefix=$(DOCDIR)/examples")
> +                #:tests? #f))           ; tests must be run as root

Does all tests have to run as root? Also, could you reverse the order of
the arguments to match other package definitions?

> +    (home-page "http://mcelog.org/")

Nit-pick: the trailing slash is unnecessary :)

> +    (synopsis "Machine check monitor for x86 Linux systems")

If this is x86-only, perhaps we should set (supported-systems)?

> +    (description
> +     "The mcelog daemon is required by the Linux kernel to log memory, I/O,
> +      CPU, and other hardware errors on x86 systems.  It can also perform
> +      user-defined tasks, such as bringing bad pages off-line, when
> +      configurable error thresholds are exceeded.")
> +    (license license:gpl2)))

The rest LGTM.

Re: [PATCH] gnu: Add mcelog.

[Tobias Geerinckx-Rice]

[-- Attachment #1.1: Type: text/plain, Size: 1265 bytes --]

Marius,

On 14/09/16 15:45, Marius Bakke wrote:
> It's not visible in the cgit interface, but it actually seems to 
> support normal snapshot downloads: 
> https://git.kernel.org/cgit/utils/cpu/mce/mcelog.git/snapshot/v141.tar.gz

I did not know that. Thanks for the tip! It still requires a snippet,
unfortunately.

> Tobias Geerinckx-Rice <me@tobias.gr> writes:
>> +                #:tests? #f))           ; tests must be run as 
>> root
> Does all tests have to run as root?

Yes. Each test wants to load modules & inject synthetic MCE events. The
daemon will even fail to start on an unsupported CPU like my current AMD
laptop.

> Also, could you reverse the order of the arguments to match other 
> package definitions?

Hm: *some* other. I rather keep them in approximate order of use.

Unrelated: I see the ‘arguments‘ indentation went funky. Will fix.

>> +    (home-page "http://mcelog.org/")
> Nit-pick: the trailing slash is unnecessary :)

Oh, I know, I just have a thing for proper root paths in URIs.

I'm seeing someone about that.

>> +    (synopsis "Machine check monitor for x86 Linux systems")
> If this is x86-only, perhaps we should set (supported-systems)?

Indeed. Thanks!

Kind regards,

T G-R


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] gnu: Add mcelog.

[Marius Bakke]

Tobias Geerinckx-Rice <me@tobias.gr> writes:

> On 14/09/16 15:45, Marius Bakke wrote:
>> It's not visible in the cgit interface, but it actually seems to 
>> support normal snapshot downloads: 
>> https://git.kernel.org/cgit/utils/cpu/mce/mcelog.git/snapshot/v141.tar.gz
>
> I did not know that. Thanks for the tip! It still requires a snippet,
> unfortunately.

Another thing, I think the snippet should be moved to a phase, as AFAIK
origin snippets should be reserved for removing unwanted files, or for
reproducibility. But, I may be wrong here.

>>> +    (home-page "http://mcelog.org/")
>> Nit-pick: the trailing slash is unnecessary :)
>
> Oh, I know, I just have a thing for proper root paths in URIs.
>
> I'm seeing someone about that.

This made me chuckle. Perhaps I should see someone about saving that
precious byte, too :)

Also, is DESTDIR supposed to be /share, shouldn't it be $out/share?
I think I'd define destdir as a variable, and use that also for the
etcprefix instead of using a make variable.

Thanks!
Marius

Re: [PATCH] gnu: Add mcelog.

[Danny Milosavljevic]

DESTDIR is a feature which lets you install into a tempdir. The idea is that you install it all there and then atomically replace the ORIGINAL version at the non-DESTDIR location.

It's very bad to set DESTDIR to out because the outdir is used after installation, too (your program will stay at that place). This is not expected by DESTDIR.

So for example let's say you'd have

DESTDIR=/tmp
PREFIX=/usr

.

Then for example a theme is first installed to /tmp/usr/share/themes (by make) - but after it's all done, the distribution is supposed to move it to /usr/share/themes atomically. The program will *always* (also when its still installed to /tmp/usr/share/themes) try to load its files from /usr/share/themes.

Re: [PATCH] gnu: Add mcelog.

[Marius Bakke]

Danny Milosavljevic <dannym@scratchpost.org> writes:

> DESTDIR is a feature which lets you install into a tempdir. The idea
> is that you install it all there and then atomically replace the
> ORIGINAL version at the non-DESTDIR location.

Oops, I meant DOCDIR! Sorry for the confusion!

>> >> +    (home-page "http://mcelog.org/")  
>> > Nit-pick: the trailing slash is unnecessary :)  
>
> If you don't include the trailing slash it will require an additional
> round-trip to the server in order to add it via a (302) redirect.

That's a very good point, although all HTTP clients I've used add it
automatically. But, explicit is better than implicit and all that.

Now I'll have to start doing it too :)

Thanks!
Marius

Re: [PATCH] gnu: Add mcelog.

[Tobias Geerinckx-Rice]

[-- Attachment #1.1: Type: text/plain, Size: 673 bytes --]

Danny, Marius,

Danny Milosavljevic <dannym@scratchpost.org> writes:
> DESTDIR is a feature which lets you install into a tempdir. The idea
> is that you install it all there and then atomically replace the
> ORIGINAL version at the non-DESTDIR location.

Herp. I know all this, no idea how I managed to confuse them.

On 14/09/16 17:32, Marius Bakke wrote:
> Oops, I meant DOCDIR! Sorry for the confusion!

I think my confusion above had to do with wrestling DOCDIR when I first
packaged this. It should indeed be $out/share... when using
prefix/DESTDIR correctly!

> Now I'll have to start doing it too :)

We have cookies.

Kind regards,

T G-R


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] gnu: Add mcelog.

[Tobias Geerinckx-Rice]

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

Marius,

On 14/09/16 17:19, Marius Bakke wrote:
> Another thing, I think the snippet should be moved to a phase, as AFAIK
> origin snippets should be reserved for removing unwanted files, or for
> reproducibility.

Not if we use your suggested snapshot tarball, which rightly lack a .git
directory. There is no way for the build system to divine the version
number at build time with ‘git describe’.

Unpatched mcelog, manually built from ‘guix download’ed sources, would
report its version as ‘unknown’. With this snippet, it just works.

Kind regards,

T G-R


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] gnu: Add mcelog.

[Marius Bakke]

Tobias Geerinckx-Rice <me@tobias.gr> writes:

> Marius,
>
> On 14/09/16 17:19, Marius Bakke wrote:
>> Another thing, I think the snippet should be moved to a phase, as AFAIK
>> origin snippets should be reserved for removing unwanted files, or for
>> reproducibility.
>
> Not if we use your suggested snapshot tarball, which rightly lack a .git
> directory. There is no way for the build system to divine the version
> number at build time with ‘git describe’.
>
> Unpatched mcelog, manually built from ‘guix download’ed sources, would
> report its version as ‘unknown’. With this snippet, it just works.

Yes, I was mostly echoing Leos sentiment from this post:

https://lists.gnu.org/archive/html/guix-devel/2016-08/msg00937.html

I don't think reporting "unknown" as a version is a critical bug, but
don't have any strong opinions either way. It seems like the manual
could use some clarification on its use, though.

Cheers,
Marius

Re: [PATCH] gnu: Add mcelog.

[Danny Milosavljevic]

> >> +    (home-page "http://mcelog.org/")  
> > Nit-pick: the trailing slash is unnecessary :)  

If you don't include the trailing slash it will require an additional round-trip to the server in order to add it via a (302) redirect.

Re: [PATCH] gnu: Add mcelog.

[Ludovic Courtès]

Tobias Geerinckx-Rice <me@tobias.gr> skribis:

> * gnu/packages/linux.scm (mcelog): New variable.

To complement what Marius wrote…

> +    (arguments
> +     `(#:phases (modify-phases %standard-phases
> +                  (delete 'configure))  ; no configure script
> +                #:make-flags (list "CC=gcc"

Please align the #:.

> +    (description
> +     "The mcelog daemon is required by the Linux kernel to log memory, I/O,
> +      CPU, and other hardware errors on x86 systems.  It can also perform
> +      user-defined tasks, such as bringing bad pages off-line, when
> +      configurable error thresholds are exceeded.")
   ^^^^^^
Remove leading space here.

Thanks.  :-)

Ludo’.

Re: [PATCH] gnu: Add mcelog.

[Tobias Geerinckx-Rice]

[-- Attachment #1.1: Type: text/plain, Size: 166 bytes --]

Ludo',

On 14/09/16 17:02, Ludovic Courtès wrote:
> Please align the #:.
> Remove leading space here.

Yup, I noticed :-) Thanks.

Kind regards,

T G-R


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]