[Top][All Lists]

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

Re: [PATCH] gnu: Add lmms

From: Rodger Fox
Subject: Re: [PATCH] gnu: Add lmms
Date: Thu, 23 Feb 2017 08:38:48 -0800
User-agent: Roundcube Webmail/1.0.6

The message is correct, but it lacks returns between "Add lmms." and "*
gnu/packages". Have a look at git log for some examples.

Yeah, that's weird. It's actually inconsistent with my commit log.
Although, mine has only one line break instead of two, which I noticed
after you mentioned this. Either way I'm wrong, but I'm not sure
why the format-patch dropped it. I will figure that out and fix it.

+       (sha256
+        (base32
+        "1g76z7ha3hd53vbqaq9n1qg6s3lw8zzaw51iny6y2bz0j1xqwcsr"))))
There's a tab in the indentation, please use spaces.

+    (build-system cmake-build-system)
+    (arguments `(#:tests? #f              ; No tests to run.
+                #:validate-runpath? #f))
There's a tab here too, and it should rather look like this:
  `(#:tests? #f
    #:validate-runpath? #f))

Why do you need to disable runpath validation?

The build was failing on the validate runpath phase, but I noticed
that the package did exist in the store and was working. The failure
was something to do with a library in a subdirectory of /lib being
dependent on another library in its same directory. But it seems
only /lib itself is in the runpath. I guess the libraries can still
find each other, but they are not in the runpath. I actually meant
to ask about this, so I'm glad you caught it. Is there a better fix
for this situation? I guess I should at least put a comment in there
to explain it.

+    (native-inputs
+     `(("pkg-config" ,pkg-config)))
+     (inputs
Indentation is off by one (inputs should be aligned with native-inputs).

+    (description "LMMS is a digital audio workstation. It includes
tools for sequencing melodies and beats and for mixing and arranging
songs. It includes instruments based on audio samples and various soft
sythesizers. It can receive input from a MIDI keyboard.")
This line is way too long, please break it. Also please use two spaces
between sentences.

I missed the two spaces rule. I'll fix the long line, too.
I was having problems with guix lint, but I will be sure get that
working before I submit something again.

Ok, this is my first review, I tried to get it right but I probably
forgot something (I still can't get my own patches right on the first
try :p). Running "guix lint lmms" would have saved you the last comment
;). I can't try it now, but I'll test your patch (or an updated version)
this evening.

I will submit an updated version so you can do that tonight.

Thanks for the feedback.

-Rodger Fox

reply via email to

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