From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leo Famulari Subject: Re: [PATCH 1/1] gnu: Add dvtm. Date: Fri, 21 Aug 2015 20:46:02 -0400 Message-ID: <1440204362.2591105.362773905.11B54ED8@webmail.messagingengine.com> References: <1439873749-27812-1-git-send-email-leo@famulari.name> <1439873749-27812-2-git-send-email-leo@famulari.name> <87mvxkb78j.fsf@netris.org> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:48050) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZSwwh-0004YF-Jw for guix-devel@gnu.org; Fri, 21 Aug 2015 20:46:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZSwwd-0000Ro-Da for guix-devel@gnu.org; Fri, 21 Aug 2015 20:46:07 -0400 Received: from out3-smtp.messagingengine.com ([66.111.4.27]:42999) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZSwwd-0000RT-6b for guix-devel@gnu.org; Fri, 21 Aug 2015 20:46:03 -0400 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id D0A312031F for ; Fri, 21 Aug 2015 20:46:02 -0400 (EDT) In-Reply-To: <87mvxkb78j.fsf@netris.org> 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-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Mark H Weaver Cc: guix-devel@gnu.org Hello, Thanks for your detailed critique of my first patch! On Fri, Aug 21, 2015, at 18:35, Mark H Weaver wrote: > Please include the Makefile variable being changed, like this: > > * gnu-system.am (GNU_SYSTEM_MODULES): Add it. Done. > This new file needs the same header as our other source files, but with > only your own copyright notice on top. Done. > Please don't use any tabs in *.scm files. All the tabs are gone. I'm new to scheme so I tried to follow Riastradh's Lisp Style Rules, as suggested in guix docs section 7.4.4 (Coding Style). Basically, I just used C-M-q in vanilla emacs but I guess that that is not enough. I will have to read more code and configure my editor more carefully... > Please keep all lines in *.scm files to 80 columns or less. Also, there > should be a space after "version". Done. > "(arguments" should be lined up with "(build-system", and the "`" should > be below the 'a'. Done. > > + #: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. I'm glad you told me this. That line was copy-pasted. I've replaced the part that skipped configure and check using modify-phases, which I find much clearer. > Please add a comment saying why tests are disabled. It could be as > simple as: > > #:tests? #f)) ; no test suite Done. > > + (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. I copied this description verbatim from the project's home page. Think I should file a bug upstream? ;) I hadn't noticed the error but now it bugs me, so I've made the change. Trailing whitespace removed. > > + (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. Done. > Can you send an updated patch? It follows this email. Thanks for your help. Please let know if there are problems with the new revision. Leo On Fri, Aug 21, 2015, at 18:35, Mark H Weaver wrote: > Hi! > > Leo Famulari 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