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

[Florian Paul Schmidt]

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.

address@hidden ~/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):

address@hidden ~/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
address@hidden ~/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:

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