Re: [PATCH] envstore-2.1

[Mark H Weaver]

Hi,

Matthew Jordan <address@hidden> writes:

> From 8de06b6e26d9e1eb7bb7ef6df163f54a46db3d89 Mon Sep 17 00:00:00 2001
> From: Matthew Jordan <address@hidden>
> Date: Thu, 12 May 2016 14:57:34 -0400
> Subject: [PATCH] gnu: Added envstore package.

The summary line should be "gnu: Add envstore."

>
> * gnu/package/enstore.scm: New file.

You misspelled "envstore.scm", but it would be better to find an
existing file in gnu/package/*.scm that would be appropriate for this.

> diff --git a/gnu/packages/envstore.scm b/gnu/packages/envstore.scm
> new file mode 100644
> index 0000000..e3ec99d
> --- /dev/null
> +++ b/gnu/packages/envstore.scm
> @@ -0,0 +1,42 @@
> +(define-module (gnu packages envstore)

When adding a new *.scm file, it needs to contain a copyright notice and
header at the top, as with our other source files.

> +  #:use-module (guix)
> +  #:use-module (guix packages)
> +  #:use-module (guix build-system gnu)
> +  #:use-module (gnu packages)
> +  #:use-module (guix download)
> +  #:use-module (guix utils)
> +  #:use-module (guix licenses))
> +
> +(define-public envstore
> +  (package
> +    (name "envstore")
> +    (version "2.1")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (string-append "https://github.com/derf/"; name "/archive/"
> +                           version ".tar.gz"))

How about using

  https://finalrewind.org/projects/envstore/envstore-2.1.tar.bz2

instead?  That's the tarball linked from the project's home page, and
unlike the github tarball, it's digitally signed.

> +       (sha256
> +        (base32 "097yd6w0fql8a3xh0gmz8bf40w61j4893rp8c28rngrrk80bk9a8"))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:test-target "test"
> +       #:phases
> +       (modify-phases %standard-phases
> +         (delete 'configure)
> +         (replace 'build
> +           (lambda _
> +             (setenv "CC" (which "gcc"))
> +             (system* "make")))

Instead of replacing the 'build' phase, it would be better to add this
to the 'arguments':

  #:make-flags (list "CC=gcc")

See 'dvtm' in dvtm.scm for an example.

> +         (replace 'install
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let ((out (assoc-ref outputs "out")))
> +               (setenv "PREFIX" "/")
> +               (setenv "DESTDIR" out)
> +               (system* "make" "install")))))))

These are incorrect settings for PREFIX and DESTDIR.  In general, PREFIX
tells where the installed files will be located when the program is run,
and DESTDIR names a temporary staging directory where "make install"
will put the files, on the assumption that they will later be moved to
PREFIX before they are run.

So, PREFIX should be set to (assoc-ref outputs "out"), and DESTDIR
should be left alone.

Also, as with the 'build' phase, it would be better to simply add these
to make-flags, like this:

  #:make-flags (list "CC=gcc"
                     (string-append "PREFIX=" (assoc-ref %outputs "out")))

> +    (home-page "https://finalrewind.org/projects/envstore/";)
> +    (synopsis "Save and restore environment variables")
> +    (description "Envstore is a program for sharing environment variables
> +between various shells or commands.")
> +    (license
> +     (non-copyleft "http://www.wtfpl.net/txt/copying/";))))

Can you send an updated patch?

     Thanks,
       Mark