Re: [PATCH] gnu: Add lmms

Re: [PATCH] gnu: Add lmms

[Rodger Fox]

[-- Attachment #1: Type: text/plain, Size: 103 bytes --]

Sorry for the delay. I got the rpath issue fixed on this.
It should be ready to apply now.

-Rodger Fox

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-Add-lmms.patch --]
[-- Type: text/x-diff; name=0001-gnu-Add-lmms.patch, Size: 2912 bytes --]

From 70f155cd4bde3060b87c0834f12f71886c1e0f3f Mon Sep 17 00:00:00 2001
From: Rodger Fox <thylakoid@openmailbox.org>
Date: Wed, 8 Mar 2017 20:28:10 -0800
Subject: [PATCH] gnu: Add lmms.

* gnu/packages/music.scm (lmms): New variable.
---
 gnu/packages/music.scm | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/gnu/packages/music.scm b/gnu/packages/music.scm
index 87b6d5b9b..07f30cfdd 100644
--- a/gnu/packages/music.scm
+++ b/gnu/packages/music.scm
@@ -9,6 +9,7 @@
 ;;; Copyright © 2016 John J. Foerch <jjfoerch@earthlink.net>
 ;;; Copyright © 2016 Alex Griffin <a@ajgrf.com>
 ;;; Copyright © 2017 ng0 <contact.ng0@cryptolab.net>
+;;; Copyright © 2017 Rodger Fox <thylakoid@openmailbox.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -2820,3 +2821,55 @@ collections and wantlists, inventory, and orders.")
 conversions between time and pulses, tempo map handling and more.  The only dependencies
 are a C compiler and glib.  Full API documentation and examples are included.")
     (license license:bsd-2)))
+
+(define-public lmms
+  (package
+    (name "lmms")
+    (version "1.1.3")
+    (source
+     (origin
+       (method url-fetch)
+       (uri (string-append "https://github.com/LMMS/lmms/archive/v"
+                           version ".tar.gz"))
+       (file-name (string-append name "-" version ".tar.gz"))
+       (sha256
+        (base32
+         "1g76z7ha3hd53vbqaq9n1qg6s3lw8zzaw51iny6y2bz0j1xqwcsr"))))
+    (build-system cmake-build-system)
+    (arguments
+     `(#:tests? #f ; no tests
+       #:phases
+       (modify-phases %standard-phases
+         (add-before
+          'configure 'set-ldflags
+          (lambda* (#:key outputs #:allow-other-keys)
+            (setenv "LDFLAGS"
+                    (string-append
+                     "-Wl,-rpath=\""
+                     (assoc-ref outputs "out") "/lib/lmms"
+                     ":"
+                     (assoc-ref outputs "out") "/lib/lmms/ladspa"
+                     "\"")))))))
+    (native-inputs
+     `(("pkg-config" ,pkg-config)))
+    (inputs
+     `(("sdl" ,sdl)
+       ("qt" ,qt-4)
+       ("fltk" ,fltk)
+       ("libogg" ,libogg)
+       ("libsamplerate" ,libsamplerate)
+       ("fluidsynth" ,fluidsynth)
+       ("libvorbis" ,libvorbis)
+       ("alsa-lib" ,alsa-lib)
+       ("portaudio" ,portaudio)
+       ("ladspa" ,ladspa)
+       ("libsndfile1" ,libsndfile)
+       ("libxft" ,libxft)
+       ("freetype2" ,freetype)
+       ("fftw3f" ,fftwf)))
+    (home-page "https://lmms.io/")
+    (synopsis "Music composition tool")
+    (description "LMMS is a digital audio workstation.  It includes tools for sequencing
+melodies and beats and for mixing and arranging songs.  LMMS includes instruments based on
+audio samples and various soft sythesizers.  It can receive input from a MIDI keyboard.")
+    (license license:gpl2+)))
-- 
2.12.0

Re: [PATCH] gnu: Add lmms

[Leo Famulari]

On Wed, Mar 08, 2017 at 08:47:03PM -0800, Rodger Fox wrote:
> Subject: [PATCH] gnu: Add lmms.
> 
> * gnu/packages/music.scm (lmms): New variable.

Thanks, this looks fun!

> +       ("qt" ,qt-4)

Qt 4 is no longer supported by the Qt project, but security
vulnerabilities continue to be discovered in it, and so we should not
add new packages that depend on it.

Apparently, LMMS can be built with Qt 5:

https://github.com/LMMS/lmms/wiki/Compiling-lmms#using-qt5

Can you try it and let us know how it goes?

Re: [PATCH] gnu: Add lmms

[Rodger Fox]

> Qt 4 is no longer supported by the Qt project, but security
> vulnerabilities continue to be discovered in it, and so we should not
> add new packages that depend on it.
> 
> Apparently, LMMS can be built with Qt 5:
> 
> https://github.com/LMMS/lmms/wiki/Compiling-lmms#using-qt5
> 
> Can you try it and let us know how it goes?

Ok, I looked into this. Qt5 is supported only in the newest development
version of lmms, but what I packaged is the latest stable release.

I will make a note of this for when I update the package, but what can
we do for the mean time? Accept this as is? Package the release
candidiate? Or just wait for the next stable release of lmms before
adding this to guix?

To be honest, the RC looks like it has some cool new features, so I
will probably be building it for myself either way. Also, lmms is on
a pretty slow release cycle. The last stable was 2015, RC-1 was
early 2016, and RC-2 was Jan 2017. So maybe if it seems stable enough
when I build it I can package the RC-2. I could do one for each if that
makes sense, kind of like the guile and guile-next packages.
What do you think? lmms and lmms-rc?

-Rodger

Re: [PATCH] gnu: Add lmms

[Leo Famulari]

On Thu, Mar 16, 2017 at 10:54:09PM -0700, Rodger Fox wrote:
> > Apparently, LMMS can be built with Qt 5:
> > 
> > https://github.com/LMMS/lmms/wiki/Compiling-lmms#using-qt5
> > 
> > Can you try it and let us know how it goes?
> 
> Ok, I looked into this. Qt5 is supported only in the newest development
> version of lmms, but what I packaged is the latest stable release.
> 
> I will make a note of this for when I update the package, but what can
> we do for the mean time? Accept this as is? Package the release
> candidiate? Or just wait for the next stable release of lmms before
> adding this to guix?
> 
> To be honest, the RC looks like it has some cool new features, so I
> will probably be building it for myself either way. Also, lmms is on
> a pretty slow release cycle. The last stable was 2015, RC-1 was
> early 2016, and RC-2 was Jan 2017. So maybe if it seems stable enough
> when I build it I can package the RC-2. I could do one for each if that
> makes sense, kind of like the guile and guile-next packages.
> What do you think? lmms and lmms-rc?

I think it's fine to package the stable release with qt-4, with a
comment in the package definition indicating that we should try qt-5
when upgrading.

Re: [PATCH] gnu: Add lmms

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

On Wed, Mar 08, 2017 at 08:47:03PM -0800, Rodger Fox wrote:
> Sorry for the delay. I got the rpath issue fixed on this.
> It should be ready to apply now.
> 
> -Rodger Fox

> From 70f155cd4bde3060b87c0834f12f71886c1e0f3f Mon Sep 17 00:00:00 2001
> From: Rodger Fox <thylakoid@openmailbox.org>
> Date: Wed, 8 Mar 2017 20:28:10 -0800
> Subject: [PATCH] gnu: Add lmms.
> 
> * gnu/packages/music.scm (lmms): New variable.

Pushed as a42619e5e211ed2511f71029ee2c5777c0f54c3b, thanks!

PS— The patch did not apply cleanly due to text encoding problems. For
example, the copyright symbols are garbled:

>  ;;; Copyright ?? 2016 John J. Foerch <jjfoerch@earthlink.net>
>  ;;; Copyright ?? 2016 Alex Griffin <a@ajgrf.com>
>  ;;; Copyright ?? 2017 ng0 <contact.ng0@cryptolab.net>
> +;;; Copyright ?? 2017 Rodger Fox <thylakoid@openmailbox.org>

And you can see similar issues in the patch as provided by mailing
list's web interface:

http://lists.gnu.org/archive/html/guix-devel/2017-03/msg00268.html

I'm not sure what do to about this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gnu: Add lmms

[Rodger Fox]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

>  It's a good idea to run "./pre-inst-env guix lint PACKAGENAME" which
>  will pick up long lines for example.
>  I have a ruler/colored line at character 83 in vim, but I'm not yet
>  ready with the setup to write correct code again in vim. There's
>  a similar thing for emacs.
> 
>  I would replace the third or second "It" with @code{LMMS} or simply
>  LMMS.

Thanks for pointing that out. I missed that pre-inst-env tool in
the manual until you just mentioned it here. That helps a lot.
My corrected patch is attached. It passes the linter this time.

-Rodger Fox

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-Add-lmms.patch --]
[-- Type: text/x-diff; name=0001-gnu-Add-lmms.patch, Size: 2636 bytes --]

From a40d4032431d5a22809c25d81ecdc8ed370e989c Mon Sep 17 00:00:00 2001
From: Rodger Fox <thylakoid@openmailbox.org>
Date: Wed, 22 Feb 2017 15:08:30 -0800
Subject: [PATCH] gnu: Add lmms.

* gnu/packages/music.scm (lmms): New variable.
---
 gnu/packages/music.scm | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/gnu/packages/music.scm b/gnu/packages/music.scm
index 730d981e6..fe24c94f6 100644
--- a/gnu/packages/music.scm
+++ b/gnu/packages/music.scm
@@ -9,6 +9,7 @@
 ;;; Copyright © 2016 John J. Foerch <jjfoerch@earthlink.net>
 ;;; Copyright © 2016 Alex Griffin <a@ajgrf.com>
 ;;; Copyright © 2017 ng0 <contact.ng0@cryptolab.net>
+;;; Copyright © 2017 Rodger Fox <thylakoid@openmailbox.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -2818,3 +2819,45 @@ collections and wantlists, inventory, and orders.")
 conversions between time and pulses, tempo map handling and more.  The only dependencies
 are a C compiler and glib.  Full API documentation and examples are included.")
     (license license:bsd-2)))
+
+(define-public lmms
+  (package
+    (name "lmms")
+    (version "1.1.3")
+    (source
+     (origin
+       (method url-fetch)
+       (uri (string-append "https://github.com/LMMS/lmms/archive/v"
+                           version ".tar.gz"))
+       (file-name (string-append name "-" version ".tar.gz"))
+       (sha256
+        (base32
+         "1g76z7ha3hd53vbqaq9n1qg6s3lw8zzaw51iny6y2bz0j1xqwcsr"))))
+    (build-system cmake-build-system)
+    (arguments
+     `(#:tests? #f              ; No tests to run.
+       #:validate-runpath? #f)) ; Binaries in subdirectories of /lib are dependent.
+                                ; It fails the runpath validation, but still works.
+    (native-inputs
+     `(("pkg-config" ,pkg-config)))
+    (inputs
+     `(("sdl" ,sdl)
+       ("qt" ,qt-4)
+       ("fltk" ,fltk)
+       ("libogg" ,libogg)
+       ("libsamplerate" ,libsamplerate)
+       ("fluidsynth" ,fluidsynth)
+       ("libvorbis" ,libvorbis)
+       ("alsa-lib" ,alsa-lib)
+       ("portaudio" ,portaudio)
+       ("ladspa" ,ladspa)
+       ("libsndfile1" ,libsndfile)
+       ("libxft" ,libxft)
+       ("freetype2" ,freetype)
+       ("fftw3f" ,fftwf)))
+    (home-page "https://lmms.io/")
+    (synopsis "Music composition tool")
+    (description "LMMS is a digital audio workstation.  It includes tools for sequencing
+melodies and beats and for mixing and arranging songs.  LMMS includes instruments based on
+audio samples and various soft sythesizers.  It can receive input from a MIDI keyboard.")
+    (license license:gpl2+)))
-- 
2.11.1

Re: [PATCH] gnu: Add lmms

[Julien Lepiller]

On Thu, 23 Feb 2017 10:13:54 -0800
Rodger Fox <thylakoid@openmailbox.org> wrote:

> >  It's a good idea to run "./pre-inst-env guix lint PACKAGENAME"
> > which will pick up long lines for example.
> >  I have a ruler/colored line at character 83 in vim, but I'm not yet
> >  ready with the setup to write correct code again in vim. There's
> >  a similar thing for emacs.
> > 
> >  I would replace the third or second "It" with @code{LMMS} or simply
> >  LMMS.  
> 
> Thanks for pointing that out. I missed that pre-inst-env tool in
> the manual until you just mentioned it here. That helps a lot.
> My corrected patch is attached. It passes the linter this time.
> 
> -Rodger Fox

I've tested the patch, and it seems to work. One of the libraries
indeed cannot find its neighbor. I've loaded the functionality that is
enabled by it, and it works, but it may break at some point. Usually
runpath problems can be solved by passing "-Wl,runpath=" option to ld
during the make phase. Can you try it? Otherwise your patch is good.

Re: [PATCH] gnu: Add lmms

[Rodger Fox]

> 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

[PATCH] gnu: Add lmms

[Rodger Fox]

[-- Attachment #1: Type: text/plain, Size: 109 bytes --]

Here is another package. I think I got the commit message right this 
time. Let me know if I missed anything.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-Add-lmms.patch --]
[-- Type: text/x-diff; name=0001-gnu-Add-lmms.patch, Size: 2415 bytes --]

From 8e2757bee584f4686e02da512662fb512b05c037 Mon Sep 17 00:00:00 2001
From: Rodger Fox <thylakoid@openmailbox.org>
Date: Wed, 22 Feb 2017 15:08:30 -0800
Subject: [PATCH] gnu: Add lmms. * gnu/packages/music.scm (lmms): New variable.

---
 gnu/packages/music.scm | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/gnu/packages/music.scm b/gnu/packages/music.scm
index 730d981e6..924fb8109 100644
--- a/gnu/packages/music.scm
+++ b/gnu/packages/music.scm
@@ -9,6 +9,7 @@
 ;;; Copyright © 2016 John J. Foerch <jjfoerch@earthlink.net>
 ;;; Copyright © 2016 Alex Griffin <a@ajgrf.com>
 ;;; Copyright © 2017 ng0 <contact.ng0@cryptolab.net>
+;;; Copyright © 2017 Rodger Fox <thylakoid@openmailbox.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -2818,3 +2819,40 @@ collections and wantlists, inventory, and orders.")
 conversions between time and pulses, tempo map handling and more.  The only dependencies
 are a C compiler and glib.  Full API documentation and examples are included.")
     (license license:bsd-2)))
+
+(define-public lmms
+  (package
+    (name "lmms")
+    (version "1.1.3")
+    (source
+     (origin
+       (method url-fetch)
+       (uri (string-append "https://github.com/LMMS/lmms/archive/v"
+                           version ".tar.gz"))
+       (sha256
+        (base32
+	 "1g76z7ha3hd53vbqaq9n1qg6s3lw8zzaw51iny6y2bz0j1xqwcsr"))))
+    (build-system cmake-build-system)
+    (arguments `(#:tests? #f              ; No tests to run.
+	         #:validate-runpath? #f))
+    (native-inputs
+     `(("pkg-config" ,pkg-config)))
+     (inputs
+     `(("sdl" ,sdl)
+       ("qt" ,qt-4)
+       ("fltk" ,fltk)
+       ("libogg" ,libogg)
+       ("libsamplerate" ,libsamplerate)
+       ("fluidsynth" ,fluidsynth)
+       ("libvorbis" ,libvorbis)
+       ("alsa-lib" ,alsa-lib)
+       ("portaudio" ,portaudio)
+       ("ladspa" ,ladspa)
+       ("libsndfile1" ,libsndfile)
+       ("libxft" ,libxft)
+       ("freetype2" ,freetype)
+       ("fftw3f" ,fftwf)))
+    (home-page "https://lmms.io/")
+    (synopsis "Music composition tool")
+    (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.")
+    (license license:gpl2+)))
-- 
2.11.1

Re: [PATCH] gnu: Add lmms

[julien lepiller]

Le 2017-02-23 00:18, Rodger Fox a écrit :
> Here is another package. I think I got the commit message right this
> time. Let me know if I missed anything.

Hi!

I wanted to try this software for while now, so you're great :)

I added some comments on your patch below:

> From 8e2757bee584f4686e02da512662fb512b05c037 Mon Sep 17 00:00:00 2001
> From: Rodger Fox <thylakoid@openmailbox.org>
> Date: Wed, 22 Feb 2017 15:08:30 -0800
> Subject: [PATCH] gnu: Add lmms. * gnu/packages/music.scm (lmms): New 
> variable.
The message is correct, but it lacks returns between "Add lmms." and "* 
gnu/packages". Have a look at git log for some examples.

> +       (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?

> +    (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.

> +    (license license:gpl2+)))
> --
> 2.11.1

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.

Re: [PATCH] gnu: Add lmms

[ng0]

On 17-02-23 10:15:47, julien lepiller wrote:
> Le 2017-02-23 00:18, Rodger Fox a écrit :
> > Here is another package. I think I got the commit message right this
> > time. Let me know if I missed anything.
> 
> Hi!
> 
> I wanted to try this software for while now, so you're great :)
> 
> I added some comments on your patch below:
> 
> > From 8e2757bee584f4686e02da512662fb512b05c037 Mon Sep 17 00:00:00 2001
> > From: Rodger Fox <thylakoid@openmailbox.org>
> > Date: Wed, 22 Feb 2017 15:08:30 -0800
> > Subject: [PATCH] gnu: Add lmms. * gnu/packages/music.scm (lmms): New
> > variable.
> The message is correct, but it lacks returns between "Add lmms." and "*
> gnu/packages". Have a look at git log for some examples.
> 
> > +       (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?
> 
> > +    (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.

It's a good idea to run "./pre-inst-env guix lint PACKAGENAME" which
will pick up long lines for example.
I have a ruler/colored line at character 83 in vim, but I'm not yet
ready with the setup to write correct code again in vim. There's
a similar thing for emacs.

I would replace the third or second "It" with @code{LMMS} or simply
LMMS.
  
> > +    (license license:gpl2+)))
> > --
> > 2.11.1
> 
> 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.
>