LV2 is a standard for plugins and matching host applications, mainly targeted at audio processing and generation. There are a large number of open source and free software synthesis packages in use or development at this time. This API ('LV2') attempts to give programmers the ability to write simple 'plugin' audio processors in C/C++ and link them dynamically ('plug') into a range of these packages ('hosts'). It should be possible for any host and any plugin to communicate completely through this interface. LV2 is a successor to LADSPA, created to address the limitations of LADSPA which many hosts have outgrown. lv2-ui provides an interface for LV2 plugins requiring a UI. SPEC: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui.spec SRPM: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui-2.4-1.fc16.src.rpm
I reviewed this package. It is mostly fine. There are a few minor issues. Stuff marked with "does no harm" are just my remarks. Those are not strictly required to be changed. ! Please include the file NEWS in %doc umentation * rpmlint says: lv2-ui-debuginfo.x86_64: E: empty-debuginfo-package We should probably disable this via %global debug_package %{nil} lv2-ui.x86_64: E: no-binary lv2-ui.x86_64: W: only-non-binary-in-usr-lib lv2-ui-devel.x86_64: W: dangling-relative-symlink /usr/include/lv2/lv2plug.in/ns/extensions/ui ../../../../../lib64/lv2/ui.lv2 lv2-ui-devel.x86_64: W: no-documentation Ignore the above four lv2-ui.x86_64: W: no-documentation See above (NEWS file) - BR: pkgconfig not required. They don't do harm though - R: pkgconfig not required. No harm either ? Does this %{_libdir}/lv2/ui.lv2 need to be %dir %{_libdir}/lv2/ui.lv2 The former will include everything inside the directory, but you are also including the individual files inside the directory later on. * /usr/include/lv2/lv2plug.in/ns/extensions/ remains unowned. Should lv2core own this or should this package? ! These %post -p /sbin/ldconfig %postun -p /sbin/ldconfig should probably be removed. We are not placing anything directly in %_libdir (or in any other ld search path) - We don't compile anything, thus # Enforce Fedora specific build flags sed -i 's|c99|c99 %{optflags}|' wscript seems irrelevant. Does no harm though. - rm -rf %{buildroot} is not required anymore. Does no harm.
Thanks for the review! All of that makes sense. One issue that probably will come up is the missing license file. It is specified in the TTL documents, I have added comments to that affect with a link to the license file and will ask upstream to include in the package. SPEC: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui.spec SRPM: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui-2.4-2.fc16.src.rpm
Hi Brendan, Orcan. Orcan, Brendan asked me to take a look at this as part of a review swap, but it seems you're already reviewing this one, so I guess I'll tackle the next one. But since I'm here anyways and I've already taken a quick peek already I would like to throw in my 2 cents: My comments are based on: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui-2.4-2.fc16.src.rpm My main concern with the current package is that there are several directory ownership issues. First of all looking at the main package I see: %files %doc NEWS %dir %{_libdir}/lv2/ui.lv2 %{_libdir}/lv2/ui.lv2/manifest.ttl %{_libdir}/lv2/ui.lv2/ui.ttl %{_libdir}/lv2/ui.lv2/%{name}.doap.ttl But who owns %{_libdir}/lv2 ? The answer to that is probably lv2core, but if that is the case then the main package should have a Requires: lv2core, because otherwise this package may end up getting installed without lv2core, and then on removal the %{_libdir}/lv2 will stay behind. Then we have the %files section for the -devel package: %files devel %dir %{_includedir}/lv2/lv2plug.in/ns/extensions %{_includedir}/lv2/lv2plug.in/ns/extensions/ui %{_libdir}/lv2/ui.lv2/ui.h %{_libdir}/pkgconfig/lv2-lv2plug.in-ns-extensions-ui.pc Which brings many questions with it, first of all which package owns %{_includedir}/lv2/lv2plug.in/ns ? The answer seems to be lv2core-devel, which means that the -devel sub package should have a Requires: lv2core-devel. Then comes the question, should we have all extensions owning %dir %{_includedir}/lv2/lv2plug.in/ns/extensions I don't think that is a good idea, I think that instead lv2core-devel should own this, simply add a: mkdir $RPM_BUILD_ROOT%{_includedir}/lv2/lv2plug.in/ns/extensions to its %build and a %dir %{_includedir}/lv2/lv2plug.in/ns/extensions to its "%files devel" Looking into the includes a bit further I noticed some weird things with lv2core: # rpm -ql lv2core | grep lv2.h /usr/lib64/lv2/lv2core.lv2/lv2.h # rpm -ql lv2core-devel | grep lv2.h /usr/include/lv2.h These are 2 copies of the same file, installed by different (sub) packages, this seems wrong I think it would be better to make one of them a symlink. I'm also wondering what a C header file is doing in the main package of lv2core?
OK, that all makes sense. Given that %{_includedir}/lv2/lv2plug.in/ns/extensions and %{_includedir}/lv2/lv2plug.in/ns/ext are used for extensions that are considered part of the LV2 spec, it is save to give ownership to lv2core-devel. No guarantee future extensions will be part of those two directories so it should own ns as well. Also, +1 to giving ownership of %{_libdir}/lv2 to lv2core and moving the header into -devel (it is currently a symlink across packages)
I spoke to Dave Robbillard and he's quite against splitting up the LV2 packages for whatever reason. The spec dictates that a bundle simply owns all of the files in its directory in this case %{_libdir}/lv2/ui.lv2, thus the reason why the symlink to the header file is in %{_includedir} and not the other way around. In this case would it be possible to simply ship everything in one package? Any plugins which use the extension would then have both Requires and BuildRequires on this package.
(In reply to comment #5) > I spoke to Dave Robbillard and he's quite against splitting up the LV2 packages > for whatever reason. Ok, what does he mean by splitting up, does he mean moving the .h file to /usr/include? If that is a problem we can just keep it under %{_libdir}/lv2/ui.lv2 and put a symlink in /usr/include > The spec dictates that a bundle simply owns all of the files in its directory > in this case %{_libdir}/lv2/ui.lv2, thus the reason why the symlink to the > header file is in %{_includedir} and not the other way around. That is fine, but if the header is only used for building c-progs, and not used runtime it should go to the -devel package (as you did). So to summarize I think that what you currently have for lv2-ui is fine, except that the extensions dir should be owned by lv2core, and that you need Requires on lv2core resp. lv2core-devel for dir ownership. ### Which leaves the question of lv2core itself, I believe that /usr/lib64/lv2/lv2core.lv2/lv2.h should be part of -devel there not the main package and that /usr/include/lv2.h should be a symlink not a copy.
OK. AS for lv2core I've been having a bit of a rethink. For the sake of argument, the spec could be extended at anytime leading to the creation of more directories under %{_includedir}/lv2/lv2plug.in/ns . Changing lv2core each time the framework is extended by a new LV2 bundle seems a little unwieldy. Should we leave each plugin owning the directories it drops files into rather than giving ownership of ext/extensions to lv2core?
(In reply to comment #7) > OK. > > AS for lv2core I've been having a bit of a rethink. For the sake of argument, > the spec could be extended at anytime leading to the creation of more > directories under %{_includedir}/lv2/lv2plug.in/ns . Changing lv2core each time > the framework is extended by a new LV2 bundle seems a little unwieldy. Should > we leave each plugin owning the directories it drops files into rather than > giving ownership of ext/extensions to lv2core? My preference would be to have lv2core-devel own: %{_includedir}/lv2/lv2plug.in/ns/extensions While the plugin itself would own (in lv2-ui's case): %{_includedir}/lv2/lv2plug.in/ns/extensions/ui As for your worry that we then need to change lv2core each time we package a plugin from a new bundle. I think that you should have a reasonable idea about which bundles exist right now, so we could just make lv2core-devel create and own the necessary deps for all bundles from which we expect to package more then 1 plugin. I agree that if some rather exotic bundles pop up, that we should just let the plugins in that bundle also have shared ownership of the %{_includedir}/lv2/lv2plug.in/ns/<bundle> dir. But for something like extensions it seems sensible to me to make lv2core-devel own it rather then make a gazillion lv2-foo-devel packages have shared ownership of it.
OK, done: SPEC: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui.spec SRPM: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui-2.4-3.fc16.src.rpm And from lv2core: diff --git a/lv2core.spec b/lv2core.spec index a82a3c7..e51acec 100644 --- a/lv2core.spec +++ b/lv2core.spec @@ -47,6 +47,9 @@ header file. %install rm -rf $RPM_BUILD_ROOT DESTDIR=$RPM_BUILD_ROOT ./waf -vv install +# create LV2 spec extension directories +install -p -m 0755 -d $RPM_BUILD_ROOT%{_includedir}/lv2/lv2plug.in/ns/ext +install -p -m 0755 -d $RPM_BUILD_ROOT%{_includedir}/lv2/lv2plug.in/ns/extensions %clean rm -rf $RPM_BUILD_ROOT
Sorry Orcan, but since I've come this far anyway I'm going to steal this review from you. I think there will be plenty of other lv2 packages for you to review instead :) So here is the result of a full review: Good: - rpmlint checks return: lv2-ui.x86_64: E: no-binary lv2-ui.x86_64: W: only-non-binary-in-usr-lib lv2-ui-devel.x86_64: W: no-documentation lv2-ui-devel.x86_64: W: dangling-relative-symlink /usr/include/lv2/lv2plug.in/ns/extensions/ui ../../../../../lib64/lv2/ui.lv2 These can all be ignored. - package meets naming guidelines - package meets packaging guidelines - license (ISC) OK, matches source - spec file legible, in am. english - source matches upstream - package compiles on devel (x86) - no missing BR - no unnecessary BR - no locales - not relocatable - no duplicate files - permissions ok - %clean ok - macro use consistent - code, not content - no need for -docs - nothing in %doc affects runtime - no need for .desktop file Must Fix -------- - owns all directories that it creates As discussed before, you need to Add Requires: lv2core to the main package and Requires: lv2core-devel to the -devel package for proper directory ownership handling Should Fix ---------- - remove "Requires: pkgconfig" from the -devel pkg, having explict Requires on pkgconfig is no longer needed these days (rpm autogenerates them). Regards, Hans
Appreciate the review Hans, here's an update with suggested fixes. SPEC: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui.spec SRPM: http://bsjones.fedorapeople.org/lv2/spec/lv2-ui-2.4-4.fc16.src.rpm
Looks good now, APPROVED!
Thanks again. Thanks! New Package SCM Request ======================= Package Name: lv2-ui Short Description: An extension of the LV2 audio plugin framework Owners: bsjones Branches: f15 f16 InitialCC:
It's okay Hans. I was saving it for the weekend since I did not have time. I am glad you finished it. Cheers.
Git done (by process-git-requests).
lv2-ui-2.4-4.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/lv2-ui-2.4-4.fc16
lv2-ui-2.4-4.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/lv2-ui-2.4-4.fc15
Package lv2-ui-2.4-4.fc16: * should fix your issue, * was pushed to the Fedora 16 testing repository, * should be available at your local mirror within two days. Update it with: # su -c 'yum update --enablerepo=updates-testing lv2-ui-2.4-4.fc16' as soon as you are able to. Please go to the following url: https://admin.fedoraproject.org/updates/FEDORA-2012-0802/lv2-ui-2.4-4.fc16 then log in and leave karma (feedback).
lv2-ui-2.4-4.fc16 has been pushed to the Fedora 16 stable repository. If problems still persist, please make note of it in this bug report.
lv2-ui-2.4-4.fc15 has been pushed to the Fedora 15 stable repository. If problems still persist, please make note of it in this bug report.