From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Paul Schmidt Subject: Re: swh-plugins-lv2: New variable [WIP] v2 Date: Mon, 7 Dec 2015 22:41:45 +0100 Message-ID: <5665FD19.3090905@gmx.net> References: <5664BA45.3080600@gmx.net> <5664BB8C.6030100@gmx.net> <87wpsqm00d.fsf@elephly.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:54660) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a63Xf-0003LF-Pl for guix-devel@gnu.org; Mon, 07 Dec 2015 16:41:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a63Xa-0004wj-Ot for guix-devel@gnu.org; Mon, 07 Dec 2015 16:41:55 -0500 Received: from mout.gmx.net ([212.227.17.22]:59126) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a63Xa-0004wT-F5 for guix-devel@gnu.org; Mon, 07 Dec 2015 16:41:50 -0500 In-Reply-To: <87wpsqm00d.fsf@elephly.net> 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: Ricardo Wurmus Cc: guix-devel@gnu.org On 12/07/2015 10:17 PM, Ricardo Wurmus wrote: > Hi Florian, > > thanks for the patch! Hi rekado, thanks for the feedback :) > The commit message should be: > > gnu: Add swh-plugins-lv2. > > * gnu/packages/audio.scm (swh-plugins-lv2): New variable. Oh drats, no more -a -m "lalalala" commits. OK, noted. This is definitely more helpful than the multi-section changelog style GNU documentation. Thanks. >> +(define-public swh-plugins-lv2 >> + (let ((commit "5098e09e255eaed14e0d40ca5e7e6dfcb782d7ea")) > > We usually don’t use full commit hashes. You could probably trim it to > the first six characters or so. Oh, OK, I wasn't aware github allowed this. Noted. > It would also be nice to state in a comment why you used this commit > (e.g. because that’s the latest commit as of now, and there haven’t been > any releases). OK >> + commit ".zip")) > > There is no good reason to use “.zip” here. “.tar.gz” will work just > fine and you won’t need the “unzip” input then. OK >> + (alist-cons-after > > Please use ‘modify-phases’ instead of the ‘alist-*’ stuff. OK, noted. I think I searched through other packages to come up with this. fps@cherry ~/src/guix [env]$ grep "#:phases (alist-cons-after" gnu/packages/*.scm | wc -l 53 etc.. > >> + 'unpack 'patch-makefile-and-enter-directory >> + (lambda >> + _ > > This should not be on its own line. OK, this is a stylistic choice I disagree with but I can live with it ;) > >> + (substitute* "Makefile" >> + (("/usr/local") (assoc-ref %outputs "out")) > > I don’t think this is really necessary. You could just add a definition > for “PREFIX” to the make-flags. Are you sure? In the makefile PREFIX is set with the = operator instead of the ?= operator. Oh, but you might be right.. >> + (alist-delete >> + 'check > > Use “#:tests? #f” instead with a note why tests are disabled. OK >> + (alist-delete >> + 'configure > > Also here please add a note why. OK >> + ("unzip" ,unzip) > > You don’t need “unzip” (see above), but had you actually needed it you > should have added it to “native-inputs”. OK >> + (description >> + "A collection of Steve Harris' audio plugins in LV2 format.") > > “guix lint” probably complains about this description, because it is a > sentence fragment, not a full sentence. It didn't. But terminals are still somewhat messy in GuixSD. This is for example the lint output I get (except for the additional line breaks introduced by my mail client): fps@cherry ~/src/guix [env]$ ./pre-inst-env guix lint swh-plugins-lv2 ;;; note: source file /home/fps/src/guix/gnu/packages/audio.scm ;;; newer than compiled /home/fps/src/guix/gnu/packages/audio.go fps@cherry ~/src/guix [env]$ -11-11-5098e09e255eaed14e0d40ca5e7e6dfcb782d7ea [formatting]...me]...ive]... See how the new prompt is in the middle of the lint output? I suspect broken terminfo entries or something (command editing in bash is seriously broken as well for commands longer than a line).. If I add a trailing whitespace in the package definition I get: fps@cherry ~/src/guix [env]$ ./pre-inst-env guix lint swh-plugins-lv2 ;;; note: source file /home/fps/src/guix/gnu/packages/audio.scm ;;; newer than compiled /home/fps/src/guix/gnu/packages/audio.go gnu/packages/audio.scm:1897:4: swh-plugins-lv2-2015-11-11-5098e09e255eaed14e0d40ca5e7e6dfcb782d7ea: trailing white space on line 1929 fps@cherry ~/src/guix [env]$ So lint didn't complain before really, but it garbled some of its own output so it's hard to tell. Or maybe it tried to be smart with using the terminal and failed :) It would also be nice if this > would include a list of plugin classes that are among this collection. > People who are looking for a chorus effect with “guix package -s > chorus”, for example, would not find this package. It doesn’t have to > be the complete list of plugins, but the categories that are covered > should be mentioned (e.g. “filters” instead of “lowpass, butterworth, > simple comb, ...”). OK > Could you please send an updated patch (after running your final version > through “guix lint”)? Sure, as I did before :) It'll be a while until I get to updating it.. Thanks again for the feedback.. Regards, Flo