guix-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Florian Paul Schmidt
Subject: Re: swh-plugins-lv2: New variable [WIP] v2
Date: Mon, 7 Dec 2015 22:41:45 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0



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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]