diff mbox

envstore-2.1

Message ID 87zirssmjz.fsf@mailerver.i-did-not-set--mail-host-address--so-tickle-me
State New
Headers show

Commit Message

Matthew Jordan May 14, 2016, 3:10 p.m. UTC
Good day all,

Submitting patch for envstore-2.1.
Let me know if I need to make any changes.
Sincerely,

Comments

Mark H Weaver May 15, 2016, 12:14 a.m. UTC | #1
Hi,

Matthew Jordan <matthewjordandevops@yandex.com> writes:

> From 8de06b6e26d9e1eb7bb7ef6df163f54a46db3d89 Mon Sep 17 00:00:00 2001
> From: Matthew Jordan <matthewjordandevops@yandex.com>
> 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
diff mbox

Patch

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

* gnu/package/enstore.scm: New file.
---
 gnu/packages/envstore.scm | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 gnu/packages/envstore.scm

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