Re: [Patch] Tmux Themepack

[Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de>]

Hi Matthew,

thank you very much for your patch!  Below are some comments.

> From 9d400b025a3d60bc29b58ba0bf1301a418ba7aa2 Mon Sep 17 00:00:00 2001
> From: Matthew Jordan <matthewjordandevops@yandex.com>
> Date: Wed, 18 May 2016 18:20:30 -0400
> Subject: [PATCH] gnu: Add Tmux themepack

> * gnu/packages/tmux.scm: (tmux-themepack): New variable.

The first “:” (after “scm”) should be deleted.

> @@ -21,10 +22,16 @@
>    #:use-module (guix licenses)
>    #:use-module (guix packages)
>    #:use-module (guix download)
> +  #:use-module (guix git-download)
> +  #:use-module (guix build-system trivial)
>    #:use-module (guix build-system gnu)
>    #:use-module (gnu packages)
> +  #:use-module (gnu packages base)
>    #:use-module (gnu packages libevent)
> -  #:use-module (gnu packages ncurses))
> +  #:use-module (gnu packages ncurses)
> +  #:use-module (guix utils)
> +  #:use-module (guix build utils))
> +

This may not be important, but I suggest putting the “use-module”
expressions for “(guix utils)” and “(guix build utils)” further up where
“(guix packages)” is.

> +
> +(define-public tmux-themepack
> +  (package
> +    (name "tmux-themepack")
> +    (version "03a3728") ;; No version tags

Please see “7.6.3 Version Numbers” in the Guix manual.  We use the full
commit (in a let binding) and construct a version string from it.

> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://github.com/jimeh/tmux-themepack.git")
> +                    (commit version)))
> +              (sha256
> +               (base32
> +                "1d3k87mq5lca042jbap5kxskjy3kg79wjhhpnm6jacbn3anc67zl"))))
> +    (build-system trivial-build-system)
> +    (arguments
> +     `(#:modules ((guix build utils))
> +       #:builder (begin
> +                   (use-modules (guix build utils))
> +                   (let* ((out (string-append
> +                                (assoc-ref %outputs "out") "/share/" ,name "-" ,version))
> +                          (env (assoc-ref %build-inputs "coreutils"))
> +                          (in (string-append (assoc-ref %build-inputs "source"))))
> +                     (copy-recursively in ".")
> +                     (substitute* "themepack.tmux"
> +                       (("#!/usr/bin/env") (string-append "#!" env "/bin/env")))
> +                     (mkdir-p out)
> +                     (copy-recursively "." out)))))

I wonder: could you use the gnu-build-system, delete the “configure” and
“build” phases, and override the “install” phase to copy the files?
Instead of running “substitute*” on “themepack.tmux” you can use the
“patch-shebang” procedure.

> +    (propagated-inputs
> +     `(("tmux" ,tmux)
> +       ("coreutils" ,coreutils)))

Why should coreutils be propagated?  And why should tmux be installed
automatically?  I think “tmux” doesn’t even need to be an input at all.

> +    (home-page "https://github.com/jimeh/tmux-themepack")
> +    (synopsis "Pack of various Tmux themes")

How about “Collection of themes for Tmux”.

> +    (description "A pack of various Tmux themes.")

Please add a description with full sentences.

Could you please send an updated patch?

Thank you!

~~ Ricardo