Bug 750139 - Review Request: lv2-mdala-plugins - LV2 port of the MDA VST plugins
Summary: Review Request: lv2-mdala-plugins - LV2 port of the MDA VST plugins
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 16
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jerry James
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-10-31 06:46 UTC by Brendan Jones
Modified: 2013-01-11 23:19 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-01-02 04:57:22 UTC
Type: ---
Embargoed:
loganjerry: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Brendan Jones 2011-10-31 06:46:24 UTC
lv2-mda-plugins is a LV2 port of the MDA VST plugins 

This project exists only in SVN and has done for quite some time. It is still under active development by one of the co-authors of the LV2 standard.

SPEC: http://bsjones.fedorapeople.org/lv2-mdala-plugins.spec
SRPM: http://bsjones.fedorapeople.org/lv2-mdala-plugins-0-0.1.svn3580.fc16.src.rpm

rpmlint /var/lib/mock/fedora-16-x86_64/result/*.rpm
lv2-mdala-plugins.src: W: spelling-error %description -l en_US phaser -> phase, phrase, phases
lv2-mdala-plugins.src: W: spelling-error %description -l en_US reverb -> revere, revers, revert
lv2-mdala-plugins.src: W: invalid-url Source0: lv2-mdala-plugins-0-svn3580.tar.bz2
lv2-mdala-plugins.x86_64: W: spelling-error %description -l en_US phaser -> phase, phrase, phases
lv2-mdala-plugins.x86_64: W: spelling-error %description -l en_US reverb -> revere, revers, revert
3 packages and 0 specfiles checked; 0 errors, 5 warnings.

Comment 2 Jerry James 2011-11-23 22:46:38 UTC
I'll take this review.  Thanks for swapping with me.  Now I get to learn what LV2, MDA, and VST stand for. :-)

Comment 3 Jerry James 2011-11-23 23:14:07 UTC
Here are a few preliminary comments.  First, the BuildRoot tag is not necessary on Fedora.  Second, it appears that README does not need to have its line endings converted, and neither README nor COPYING need to be converted to UTF-8.  Third, it looks like the source files are not being compiled with RPM_OPT_FLAGS.  Is that right?  If I'm reading the docs right, I think you want to configure like this:

CFLAGS="$RPM_OPT_FLAGS" ./waf configure --prefix=%{_prefix} --libdir=%{_libdir}

Fourth, I'm not familiar with waf.  Is it wise to use the shipped version, instead of the version in the Fedora waf package?

+: OK
-: must be fixed
=: should be fixed (at your discretion)
N: not applicable

MUST:
[+] rpmlint output:
lv2-mdala-plugins.x86_64: W: spelling-error %description -l en_US phaser -> phase, phrase, phases
lv2-mdala-plugins.x86_64: W: spelling-error %description -l en_US reverb -> revere, revers, revert
lv2-mdala-plugins.spec: W: invalid-url Source0: lv2-mdala-plugins-0-svn3580.tar.bz2
2 packages and 1 specfiles checked; 0 errors, 3 warnings.
[+] follows package naming guidelines
[+] spec file base name matches package name
[=] package meets the packaging guidelines: with the possible exception of the RPM_OPT_FLAGS issue noted above
[+] package uses a Fedora approved license
[+] license field matches the actual license
[=] license file is included in %doc: the license file actually included is the GPL v2 license.  See if upstream will update that to the GPL v3 license text.
[+] spec file is in American English
[+] spec file is legible
[+] sources match upstream
[+] package builds on at least one primary arch (tried x86_64)
[N] appropriate use of ExcludeArch
[+] all build requirements in BuildRequires
[N] spec file handles locales properly
[N] ldconfig in %post and %postun
[+] no bundled copies of system libraries
[N] no relocatable packages
[+] package owns all directories that it creates
[+] no files listed twice in %files
[+] proper permissions on files
[+] consistent use of macros
[+] code or permissible content
[N] large documentation in -doc
[+] no runtime dependencies in %doc
[N] header files in -devel
[N] static libraries in -static
[N] .so in -devel
[N] -devel requires main package
[+] package contains no libtool archives
[N] package contains a desktop file, uses desktop-file-install
[+] package does not own files/dirs owned by other packages
[+] all filenames in UTF-8

SHOULD:
[=] query upstream for license text: see above
[N] description and summary contains available translations
[+] package builds in mock: tried fedora-rawhide-i386
[+] package builds on all supported arches: tried i386 and x86_64
[=] package functions as described: did not test
[+] sane scriptlets
[N] subpackages require the main package
[N] placement of pkgconfig files
[N] file dependencies versus package dependencies
[N] package contains man pages for binaries/scripts

Comment 4 Brendan Jones 2011-11-24 08:08:16 UTC
Hi Jerry, thanks for the review!

I've removed the README form the encoding fix and dropped the buildroot tag.

RPM_OPT_FLAGS and %{optflags} are interchangeable [1]

From what I know the waf is not so concerned with backwards compatibility as other build tools (such as cmake) and recommends shipping the waf script. There are no clear guidelines on this - the version of waf used here is actually earlier than the one in f16, so I will leave as is.  

I have contacted the author with regards to the incorrect license file, stay tuned.

thanks

Brendan

[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

Comment 5 Jerry James 2011-11-24 18:41:01 UTC
(In reply to comment #4)
> RPM_OPT_FLAGS and %{optflags} are interchangeable [1]

Errr, yeah, I knew that.  I don't know how I overlooked that %{optflags}.  Must be time to go see the eye doctor again...

> I have contacted the author with regards to the incorrect license file, stay
> tuned.

OK, thanks.

Comment 6 Brendan Jones 2011-11-27 13:48:08 UTC
The License file has been updated in SVN however the author has expressed some misgivings about packaging pre-release, so I will respect his wishes. This review can be put on hold until further notice, ( a release is imminent however)

Comment 7 Brendan Jones 2011-12-09 20:55:25 UTC
Hi Jerry,

this may be on hold for a while. I have this other LV@ plugin if interested bug 760270

cheers

Comment 8 Brendan Jones 2012-05-16 14:34:04 UTC
OK - upstream has officially released this:

SPEC: http://bsjones.fedorapeople.org/lv2-mdala-plugins.spec
SRPM: http://bsjones.fedorapeople.org/lv2-mdala-plugins-1.0.0.fc17.src.rpm

ta

Comment 9 Jerry James 2012-05-16 19:10:28 UTC
I'm getting an HTTP 404 on that SRPM URL.

Comment 11 Jerry James 2012-05-16 21:40:38 UTC
No problem.  It's not like I've never done that. :-)

There is no %changelog entry for the new release.

I couldn't tell if the right CFLAGS were being used for compilation, so I added -v to the waf build invocation.  They aren't.  The compiler is being invoked like this (plus some -I and -D options):

/usr/lib/ccache/g++ -O0 -g -fshow-column -fPIC <filename>.cpp

The file COPYING is ASCII with Unix line endings, so the sed and iconv invocations in %prep create an exact duplicate of the original file.

rpm -q --provides lv2-mdala-plugins shows a bunch of .so files (Ambience.so, Bandisto.so, etc.).  Is that correct, or should those be filtered out?

Everything else looks okay.

Comment 12 Brendan Jones 2012-05-18 09:27:17 UTC
SPEC: http://bsjones.fedorapeople.org/lv2-mdala-plugins.spec
SRPM: http://bsjones.fedorapeople.org/lv2-mdala-plugins-1.0.0-2.fc17.src.rpm

Thanks again. Changelog updated.

I have updated the wscript to honour Fedora coimpiler flags and removed the line encoding script and left the verbose flag in for good measure

The .so's are actually the plugin binaries themselves and are required

Comment 13 Jerry James 2012-05-26 20:56:26 UTC
Sorry for the delay.  It's crunch time at work.

That takes care of the last of my concerns, so this package is APPROVED.

Comment 14 Jerry James 2012-08-03 16:07:13 UTC
Brendan: ping.

Comment 15 Jerry James 2012-08-17 15:24:47 UTC
Brendan, are you out there?

Comment 16 Brendan Jones 2012-09-19 09:29:23 UTC
Sorry James, been a bit crazy mine end workwise - will jump on this ASAP. Thanks for the review!

Comment 17 Brendan Jones 2012-10-07 04:20:24 UTC

Thanks for the review!

New Package SCM Request
=======================
Package Name: lv2-mdala-plugins
Short Description: LV2 port of the MDA VST plugins
Owners: bsjones
Branches: f16 f17 f18
InitialCC:

Comment 18 Jason Tibbitts 2012-10-08 16:22:08 UTC
Git done (by process-git-requests).

Comment 19 Fedora Update System 2012-12-18 08:22:41 UTC
lv2-mdala-plugins-1.0.0-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/lv2-mdala-plugins-1.0.0-2.fc18

Comment 20 Fedora Update System 2012-12-18 08:23:01 UTC
lv2-mdala-plugins-1.0.0-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/lv2-mdala-plugins-1.0.0-2.fc17

Comment 21 Fedora Update System 2012-12-18 21:25:29 UTC
lv2-mdala-plugins-1.0.0-2.fc18 has been pushed to the Fedora 18 testing repository.

Comment 22 Fedora Update System 2013-01-02 04:57:24 UTC
lv2-mdala-plugins-1.0.0-2.fc17 has been pushed to the Fedora 17 stable repository.

Comment 23 Fedora Update System 2013-01-11 23:19:48 UTC
lv2-mdala-plugins-1.0.0-2.fc18 has been pushed to the Fedora 18 stable repository.


Note You need to log in before you can comment on or make changes to this bug.