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

[Ricardo Wurmus <rekado@elephly.net>]

Hi Florian,

thanks for the patch!

> From 6dee84494a522921baacbcad8c7618c9eb709eb1 Mon Sep 17 00:00:00 2001
> From: Florian Paul Schmidt <mista.tapas@gmx.net>
> 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.

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