On Mon, Dec 07, 2015 at 10:17:38PM +0100, Ricardo Wurmus wrote: > Hi Florian, > > thanks for the patch! > > > From 6dee84494a522921baacbcad8c7618c9eb709eb1 Mon Sep 17 00:00:00 2001 > > From: Florian Paul Schmidt <[email protected]> > > Date: Wed, 2 Dec 2015 15:30:14 +0100 > > Subject: [PATCH] swh-plugins-lv2: New variable > > The commit message should be: > > gnu: Add swh-plugins-lv2. > > * gnu/packages/audio.scm (swh-plugins-lv2): New variable. > > > --- > > gnu/packages/audio.scm | 40 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > diff --git a/gnu/packages/audio.scm b/gnu/packages/audio.scm > > index db3f912..ec91c33 100644 > > --- a/gnu/packages/audio.scm > > +++ b/gnu/packages/audio.scm > > @@ -1891,3 +1891,43 @@ access to ALSA PCM devices, taking care of the many > > functions required to > > open, initialise and use a hw: device in mmap mode, and providing floating > > point audio data.") > > (license license:gpl3+))) > > + > > +(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.
Why is that? > > 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). > > > + (package > > + (name "swh-plugins-lv2") > > + (version (string-append "2015-11-11-" commit)) > > + (source (origin > > + (method url-fetch) > > + (uri (string-append "https://github.com/swh/"; > > + "lv2/archive/" > > + 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. > > > + (sha256 > > + (base32 > > + "11c6y4nfw5kz7647axpgdaryiiwwrplnkdbkglx362cbcmhpvds8")))) > > + (build-system gnu-build-system) > > + (arguments > > + `(#:phases > > + (alist-cons-after > > Please use ‘modify-phases’ instead of the ‘alist-*’ stuff. > > > + 'unpack 'patch-makefile-and-enter-directory > > + (lambda > > + _ > > This should not be on its own line. > > > + (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. > > > + (("install:") "install: install-system"))) > > > > + (alist-delete > > + 'check > > Use “#:tests? #f” instead with a note why tests are disabled. > > > + (alist-delete > > + 'configure > > Also here please add a note why. > > > + %standard-phases))) > > + #:make-flags '("CC=gcc"))) > > + (inputs > > + `(("lv2" ,lv2) > > + ("unzip" ,unzip) > > You don’t need “unzip” (see above), but had you actually needed it you > should have added it to “native-inputs”. > > > + ("fftw" ,fftw) > > + ("libxslt" ,libxslt))) > > + (home-page "http://plugin.org.uk";) > > + (synopsis "SWH Plugins in LV2 format") > > + (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 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, ...”). > > > + (license license:gpl3+)))) > > Could you please send an updated patch (after running your final version > through “guix lint”)? > > Thanks! > > ~~ Ricardo > >
