Re: [PATCH 1/1] gnu: Add dvtm.

[Mark H Weaver]

Hi!

Leo Famulari <address@hidden> writes:
> * gnu/packages/dvtm.scm: New file.
> * gnu-system.am: Add it.

Please include the Makefile variable being changed, like this:

* gnu-system.am (GNU_SYSTEM_MODULES): Add it.

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

This new file needs the same header as our other source files, but with
only your own copyright notice on top.

> +  #:use-module (guix packages)
> +  #:use-module (guix download)
> +  #:use-module (guix build-system gnu)
> +  #:use-module (guix licenses)
> +  #:use-module (gnu packages ncurses))
> +
> +(define-public dvtm 
> +  (package
> +   (name "dvtm")
> +   (version "0.14")
> +   (source (origin
> +         (method url-fetch)

Please don't use any tabs in *.scm files.

> +         (uri (string-append "http://www.brain-dump.org/projects/dvtm/dvtm-"; 
> version".tar.gz"))

Please keep all lines in *.scm files to 80 columns or less.  Also, there
should be a space after "version".

> +         (sha256
> +          (base32 "0ykl8dz7ivjgdzhmhlgidnp2ffh5gxq9lbg276w7iid4z10v76wa"))))
> +   (build-system gnu-build-system)
> +       (arguments
> +         `(#:make-flags (list (string-append "PREFIX=" (assoc-ref %outputs 
> "out")))

"(arguments" should be lined up with "(build-system", and the "`" should
be below the 'a'.  

> +           #:phases (alist-replace 'configure (lambda _ (setenv "CC" (which 
> "gcc"))) %standard-phases)

You should be able to remove this line and simply add "CC=gcc" to the
make flags.

> +           #:tests? #f))

Please add a comment saying why tests are disabled.  It could be as
simple as:

      #:tests? #f))  ; no test suite

> +
> +   (inputs `(("ncurses" ,ncurses)))
> +   (synopsis "Tiling window management for the console")
> +   (description "dvtm brings the concept of tiling window management, 
> +popularized by X11-window managers like dwm to the console.  As a console 
> window 
> +manager it tries to make it easy to work with multiple console based 
> programs.  ")

There should probably be a comma after "dwm" here.  Please remove the
extraneous spaces at the end of the description, and wrap to fit in 80
columns.

> +   (home-page "http://www.brain-dump.org/projects/dvtm/";)
> +   (license (list isc x11))))

When there is more than one license, we add a comment above the
'license' field explaining the license situation.  We need to do this
because the meaning of our license lists is ambiguous: it might mean
dual-licensing, or it might mean that some of the code is under one
license and some is under another.

Also, it should be the 'expat' license, not the 'x11' license.  They are
both often ambiguously called the "mit" license, and are very similar,
so it's a common mistake.

Can you send an updated patch?

     Thanks,
       Mark