From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [Patch] Tmux Themepack Date: Sat, 11 Jun 2016 09:45:38 +0200 Message-ID: <8737ok6ue5.fsf@mdc-berlin.de> References: <871t46gtte.fsf@mailerver.i-did-not-set--mail-host-address--so-tickle-me> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:53717) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBddD-00089U-EU for guix-devel@gnu.org; Sat, 11 Jun 2016 03:47:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bBdd9-00014z-7b for guix-devel@gnu.org; Sat, 11 Jun 2016 03:46:58 -0400 Received: from pegasus.bbbm.mdc-berlin.de ([141.80.25.20]:39486) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bBdd8-0000yu-On for guix-devel@gnu.org; Sat, 11 Jun 2016 03:46:55 -0400 In-Reply-To: <871t46gtte.fsf@mailerver.i-did-not-set--mail-host-address--so-tickle-me> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Matthew Jordan Cc: guix-devel 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 > 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 =E2=80=9C:=E2=80=9D (after =E2=80=9Cscm=E2=80=9D) should be del= eted. > @@ -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 =E2=80=9Cuse-module=E2= =80=9D expressions for =E2=80=9C(guix utils)=E2=80=9D and =E2=80=9C(guix build u= tils)=E2=80=9D further up where =E2=80=9C(guix packages)=E2=80=9D is. > + > +(define-public tmux-themepack > + (package > + (name "tmux-themepack") > + (version "03a3728") ;; No version tags Please see =E2=80=9C7.6.3 Version Numbers=E2=80=9D in the Guix manual. W= e 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 "/b= in/env"))) > + (mkdir-p out) > + (copy-recursively "." out))))) I wonder: could you use the gnu-build-system, delete the =E2=80=9Cconfigu= re=E2=80=9D and =E2=80=9Cbuild=E2=80=9D phases, and override the =E2=80=9Cinstall=E2=80=9D= phase to copy the files? Instead of running =E2=80=9Csubstitute*=E2=80=9D on =E2=80=9Cthemepack.tm= ux=E2=80=9D you can use the =E2=80=9Cpatch-shebang=E2=80=9D procedure. > + (propagated-inputs > + `(("tmux" ,tmux) > + ("coreutils" ,coreutils))) Why should coreutils be propagated? And why should tmux be installed automatically? I think =E2=80=9Ctmux=E2=80=9D doesn=E2=80=99t even need = to be an input at all. > + (home-page "https://github.com/jimeh/tmux-themepack") > + (synopsis "Pack of various Tmux themes") How about =E2=80=9CCollection of themes for Tmux=E2=80=9D. > + (description "A pack of various Tmux themes.") Please add a description with full sentences. Could you please send an updated patch? Thank you! ~~ Ricardo