Re: [PATCH] gnu: Add lmms

[Rodger Fox <thylakoid@openmailbox.org>]

> 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:
> (arguments
>   `(#: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