[Needs Packaging] LSP-Plugins for Eoan

[Erich Eickmeyer]

From the project page:

LSP (Linux Studio Plugins) is a collection of open-source plugins currently compatible with follwing formats:

 * LADSPA - set of plugins for Linux Audio Developer's Simple Plugin API
 * LV2 - set of plugins and UIs for Linux Audio Developer's Simple Plugin API (LADSPA) version 2
 * LinuxVST - set of plugins and UIs for Steinberg's VST 2.4 format ported on GNU/Linux Platform
 * JACK - Standalone versions for JACK Audio connection Kit with UI

The basic idea is to fill the lack of good and useful plugins under the GNU/Linux platform.
----
I have finished packaging this and would like it included in Ubuntu's universe repositories, targeting eoan.

Code is at https://launchpad.net/lsp-plugins

[Robie Basak (racb)]

Not a full review, but I glanced at it, so in the hope that this can help speed things up:

09:18 <rbasak> Eickmeyer: I don't think I have time to review the whole thing, but I did just glance at it
09:18 <rbasak> Eickmeyer: I think you need to use dpkg-buildflags to adjust as you need - overriding wholesale can cause issues
09:20 <rbasak> Eickmeyer: apart from that maybe look into if anything multiarch-related is advisable (I don't know - I'd need to rtfm to check)
09:20 <rbasak> Eickmeyer: besides that it all looks reasonable from a quick glance
09:20 <rbasak> Eickmeyer: maybe fix the test run?
09:20 <rbasak> Have you run lintian --pedantic against it?

[Robie Basak (racb)]

Also the short descriptions are all the same?

[Erich Eickmeyer (eeickmeyer)]

Fixed the short descriptions. Pedantic returns nothing. Looking over the rules file.

[Erich Eickmeyer (eeickmeyer)]

Moved many variables to dpkg-buildflags, unable to fix the test run, appears to be unnecessary per http://manpages.ubuntu.com/manpages/disco/en/man1/dh_auto_test.1.html, unless I'm interpreting that wrong. Build works well, tested in the Ubuntu Studio Autobuild PPA (https://code.launchpad.net/~ubuntustudio-dev/+archive/ubuntu/autobuild).

[Steve Langasek (vorlon)]

The version number in the debian/changelog of the git branch is wrong; this needs to be a non-native package using the upstream tarball from sourceforge.

+ libjack-jackd2-dev | libjack-dev,
+ php7.2-cli | php5-cli,
+ cairo-static | libcairo2-dev,

why are you using alternative build-dependencies for a new, Ubuntu-only package? You should only list whichever of these build dependencies you actually expect to be used (and there is no cairo-static package in Ubuntu anywhere, so that is definitely wrong).

Please use php-cli, not phpN.M-cli, as the build-dependency.

+Provides:
+ lv2-plugin,
+ ladspa-plugin,

Why are these Provides: attached to the lsp-plugins package, when there are separate lsp-plugins-ladspa and lsp-plugins-lv2 packages? The Provides look like they belong on the latter packages.

+Package: lsp-plugins-common
+Architecture: any
+Provides: lsp-plugins-standalone

Something is definitely wrong here given that you also have a binary package named lsp-plugins-standalone which depends on lsp-plugins-common. In fact, lsp-plugins-standalone appears to be an empty package; so you should not list this binary package at all in debian/control.

+FLAGS=-O3 -pipe -ffast-math -mtune=generic -msse -msse2 -mfpmath=sse -fPIC -DPIC -fvisibility=hidden -Wno-unused-result

How have you arrived at these flags? Compiler flags should a) inherit from dpkg-buildflags by default (You are completely overriding them), and b) document the reasons for any divergence from the distro defaults.

But actually, it seems that the only thing you're doing is setting these flags in a make variable called 'dpkg_buildflags', so in fact this has no effect at all.

Not that you could tell by looking at the build log, which does not show the compiler commands being invoked. This is significant for debuggability of builds in launchpad. Please figure out how to make this upstream's build system appropriately verbose.

+override_dh_auto_test:
+ #Tests only work manually, ok to override per http://manpages.ubuntu.com/manpages/disco/en/man1/dh_auto_test.1.html
+

what are these tests, and why do they only work manually? It's /allowed/ to override tests, but the goal should be to run them.

[Erich Eickmeyer (eeickmeyer)]

Hi Steve,

On Mon, May 6, 2019 at 2:24 PM, Steve Langasek
<email address hidden> wrote:
> The version number in the debian/changelog of the git branch is wrong;
> this needs to be a non-native package using the upstream tarball from
> sourceforge.

Fixed the versioning. The upstream tarball is on Github, no longer on
sourceforge (their website is a little behind, if that's where you got
your info).

> + libjack-jackd2-dev | libjack-dev,
> + php7.2-cli | php5-cli,
> + cairo-static | libcairo2-dev,
>
> why are you using alternative build-dependencies for a new,
> Ubuntu-only
> package? You should only list whichever of these build dependencies
> you
> actually expect to be used (and there is no cairo-static package in
> Ubuntu anywhere, so that is definitely wrong).
>
> Please use php-cli, not phpN.M-cli, as the build-dependency.
>

I don't know why, but I can fix. This is mostly a grab from the
KXStudio repo. As I don't know completely how to clean it, this is why
I appreciate feedback so I can learn these things and fix them,
reducing my ignorance through more practice. Learning by doing.

> +Provides:
> + lv2-plugin,
> + ladspa-plugin,
>
> Why are these Provides: attached to the lsp-plugins package, when
> there
> are separate lsp-plugins-ladspa and lsp-plugins-lv2 packages? The
> Provides look like they belong on the latter packages.

Fixed.

>
> +Package: lsp-plugins-common
> +Architecture: any
> +Provides: lsp-plugins-standalone
>
> Something is definitely wrong here given that you also have a binary
> package named lsp-plugins-standalone which depends on lsp-plugins-
> common. In fact, lsp-plugins-standalone appears to be an empty
> package;
> so you should not list this binary package at all in debian/control.

Fixed by renaming the common package to standalone. All of the other
plugins depend on these binaries, so it make sense. I guess I was being
redundant by having an empty package just to install the binaries as
standalone.

> +FLAGS=-O3 -pipe -ffast-math -mtune=generic -msse -msse2 -mfpmath=sse
> -fPIC -DPIC -fvisibility=hidden -Wno-unused-result
>
> How have you arrived at these flags? Compiler flags should a) inherit
> from dpkg-buildflags by default (You are completely overriding them),
> and b) document the reasons for any divergence from the distro
> defaults.
> But actually, it seems that the only thing you're doing is setting
> these
> flags in a make variable called 'dpkg_buildflags', so in fact this has
> no effect at all.
>
> Not that you could tell by looking at the build log, which does not
> show
> the compiler commands being invoked. This is significant for
> debuggability of builds in launchpad. Please figure out how to make
> this upstream's build system appropriately verbose.

I went ahead and removed all of those flags. The build worked fine, so
I don't understand why they were there to begin with. Again, that was
copied from KXStudio.

> +override_dh_auto_test:
> + #Tests only work manually, ok to override per
> <http://manpages.ubuntu.com/manpages/disco/en/man1/dh_auto_test.1.html>
> +
>
> what are these tests, and why do they only work manually? It's
> /allowed...

[Steve Langasek (vorlon)]

Thanks, this looks much better now, but there are a number of lintian errors that need to be addressed to get this archive-ready.

$ lintian ../lsp-plugins_1.1.9*changes
E: lsp-plugins-vst: binary-or-shlib-defines-rpath usr/lib/vst/lsp-plugins-lxvst-1.1.9/lsp-plugins-vst-comp-delay-mono.so /usr/local/lib
E: lsp-plugins-vst: binary-or-shlib-defines-rpath usr/lib/vst/lsp-plugins-lxvst-1.1.9/lsp-plugins-vst-comp-delay-mono.so /usr/lib
E: lsp-plugins-vst: binary-or-shlib-defines-rpath usr/lib/vst/lsp-plugins-lxvst-1.1.9/lsp-plugins-vst-comp-delay-mono.so /lib
E: lsp-plugins-vst: binary-or-shlib-defines-rpath ... use --no-tag-display-limit to see all (or pipe to a file/program)
E: lsp-plugins-standalone: binary-or-shlib-defines-rpath usr/bin/lsp-plugins-comp-delay-mono /usr/lib
E: lsp-plugins-standalone: binary-or-shlib-defines-rpath usr/bin/lsp-plugins-comp-delay-mono /lib
E: lsp-plugins-standalone: binary-or-shlib-defines-rpath usr/bin/lsp-plugins-comp-delay-mono /usr/local/lib
E: lsp-plugins-standalone: binary-or-shlib-defines-rpath ... use --no-tag-display-limit to see all (or pipe to a file/program)
W: lsp-plugins-standalone: privacy-breach-generic usr/share/doc/lsp-plugins/html/video.html [<iframe width="560" height="315" src="https://www.youtube.com/embed/j-rnb409gyg" frameborder="0" allowfullscreen>] (https://www.youtube.com/embed/j-rnb409gyg)
W: lsp-plugins-standalone: privacy-breach-generic usr/share/doc/lsp-plugins/html/video.html [<iframe width="560" height="315" src="https://www.youtube.com/embed/n4ojf2sjuhg" frameborder="0" allowfullscreen>] (https://www.youtube.com/embed/n4ojf2sjuhg)
W: lsp-plugins-standalone: privacy-breach-generic usr/share/doc/lsp-plugins/html/video.html [<iframe width="560" height="315" src="https://www.youtube.com/embed/gsnfz0tf-bk" frameborder="0" allowfullscreen>] (https://www.youtube.com/embed/gsnfz0tf-bk)
W: lsp-plugins-standalone: privacy-breach-generic ... use --no-tag-display-limit to see all (or pipe to a file/program)
W: lsp-plugins-standalone: binary-without-manpage usr/bin/lsp-plugins-comp-delay-mono
W: lsp-plugins-standalone: binary-without-manpage usr/bin/lsp-plugins-comp-delay-stereo
W: lsp-plugins-standalone: binary-without-manpage usr/bin/lsp-plugins-comp-delay-x2-stereo
W: lsp-plugins-standalone: binary-without-manpage ... use --no-tag-display-limit to see all (or pipe to a file/program)
E: lsp-plugins-standalone: sharedobject-in-library-directory-missing-soname usr/lib/lsp-plugins-jack-core-1.1.9.so
E: lsp-plugins-ladspa: binary-or-shlib-defines-rpath usr/lib/ladspa/lsp-plugins-ladspa.so /lib
E: lsp-plugins-ladspa: binary-or-shlib-defines-rpath usr/lib/ladspa/lsp-plugins-ladspa.so /usr/lib
E: lsp-plugins-ladspa: binary-or-shlib-defines-rpath usr/lib/ladspa/lsp-plugins-ladspa.so /usr/local/lib
E: lsp-plugins-lv2: binary-or-shlib-defines-rpath usr/lib/lv2/lsp-plugins.lv2/lsp-plugins-lv2.so /usr/local/lib
E: lsp-plugins-lv2: binary-or-shlib-defines-rpath usr/lib/lv2/lsp-plugins.lv2/lsp-plugins-lv2.so /usr/lib
E: lsp-plugins-lv2: binary-or-shlib-defines-rpath usr/lib/lv2/lsp-plugins.lv2/lsp-plugins-lv2.so /lib

[Erich Eickmeyer (eeickmeyer)]

Uploaded to NEW queue per @vorlon

[Łukasz Zemczak (sil2100)]

Thank you for your upload! I did a quick review of the package in the NEW queue and I do have a few things I'd like cleared out before we get this one accepted into universe:

1) debian/watch file seems to be incorrect? I would like this accepted before proceeding. At least when I tried running uscan I always got this warning and no upstream tarball has been downloaded:

uscan warn: In debian/watch no matching hrefs for version 1.1.9 in watch line
  https://github.com/sadko4u/lsp-plugins/tags .*/release-?(\d\S*)\.tar\.gz

2) I still see that override_dh_auto_test disables tests. In comment #6 you mentioned that you were somehow able to make those working. Do those really not work currently? Of course if there's no way of running them during build then that's fine, but I want to make sure that they're disabled for valid reasons. Especially that there was some discussion regarding that previously.

3) This here is not a problem per se, but just something I was curious about. In debian/control, in the long descriptions, I see that there's a link to documentation. Is this official documentation for the plugins? The webpage seems to be different than the home-page. Also, it seems to be all in French - which is fine, but I guess it would be best if for non-localized versions of the descriptions the docs was in English. It's fine if there's nothing like that.

[Steve Langasek (vorlon)]

On Mon, Jun 03, 2019 at 05:03:53PM -0000, Łukasz Zemczak wrote:
> 2) I still see that override_dh_auto_test disables tests. In comment #6
> you mentioned that you were somehow able to make those working. Do those
> really not work currently? Of course if there's no way of running them
> during build then that's fine, but I want to make sure that they're
> disabled for valid reasons. Especially that there was some discussion
> regarding that previously.

The only way to run the tests is by rebuilding the software with different
build flags and testing that. So the tests are not a valid test of the
binaries being shipped, and instead triple the package build time for
minimal added value.

This is an upstream issue to sort out before we would want to do build-time
tests.

[Erich Eickmeyer (eeickmeyer)]

@sil2100 Thanks for the review!

1) Looks like it was looking in "tags" instead of "release" so I fixed accordingly.

2) Actually, it turns out I was never able to get the test suite to work. Seems to be broken upstream. I thought it was referring to actually testing to see if they work, so that was my error. Either way, @vorlon was also unable to get them to work, hence his comment on the debian/rules file. I have filed a bug upstream (https://github.com/sadko4u/lsp-plugins/issues/50). Rest assured, though, the final product works exactly as intended.

3) You are absolutely right. The documentation page has since moved to https://lsp-plug.in/?page=manuals. debian/control updated accordingly.

Hopefully that meets your satisfaction. :)

[Erich Eickmeyer (eeickmeyer)]

Question from the upstream developer RE: sil2100's question 2):

> Is it possible to run build again before running tests? I can change the build scripts so the
> binaries for tests will be built in a different folder (not .build but .test, for example)
> and can be launched from this folder.

I don't have an answer. Anybody care to chime-in?

[Erich Eickmeyer (eeickmeyer)]

Addendum to previous comment (sorry about the noise):

> Or you can build both versions (test and non-test) on the build stage
> (the 'release' will be in '.build' subdirectory, the 'test' will be in
> '.test' subdirectory), then package all from the '.build' as a distribution
> and launch tests from '.test' at the testing build stage.

[Erich Eickmeyer (eeickmeyer)]

Working with the upstream developer, I was able to apply patches that fix the test suite issues. @sil2100's question 2) above (and the item frustrating myself and @vorlon) is fixed. Git repo is tagged for 1.1.9-0ubuntu1 release to eoan.

[Łukasz Zemczak (sil2100)]

Thanks for addressing all the issues! I also see that the new version of the package build successfully on the autobuild PPA, awesome. I think this is now good for upload and acceptance - let me do that now.

[Łukasz Zemczak (sil2100)]

...since the debian/watch file still didn't quite work, I fixed it and pushed the changes to the repository. Now it's good to go!