Re: swh-plugins-lv2: New variable [WIP] v2

[Leo Famulari]

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 <address@hidden>
> > 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
> 
>