Bug 789386
Summary: | Review Request: lilv - An LV2 Resource Description Framework Library | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Brendan Jones <brendan.jones.it> |
Component: | Package Review | Assignee: | Orcan Ogetbil <oget.fedora> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | unspecified | Docs Contact: | |
Priority: | unspecified | ||
Version: | 16 | CC: | notting, oget.fedora, package-review |
Target Milestone: | --- | Flags: | oget.fedora:
fedora-review+
petersen: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Whiteboard: | |||
Fixed In Version: | lilv-0.5.0-3.fc17 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2012-03-24 00:43:03 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 797418 |
Description
Brendan Jones
2012-02-10 16:42:25 UTC
I started reviewing this one. Here are my initial findings: ! The specfile in th link above does not match the specfile that comes inside the SRPM. I started the review with the latter. * Missing BR: lv2core-devel sord-devel (You BR sord instead, which is insufficient). I suggest using mock which will tell you the BRs you need to add. * The directory /etc/bash_completion.d/ remains unowned. We can either own this directory, or add Requires: bash_completion. But looking at the multiple owners of the directory /etc/bash_completion.d/ I can say that we should also own this directory. * You can pass "--bindings" to "./waf configure" to build the Python bindings. Note that you will then also need BR: swig python-devel (Just BR: python is not sufficient) * Fedora specific compilation flags are not honored. You will need to export CFLAGS instead, as the source is written in C. ! Since you build the unit test via --test, you should probably run the test in %check. * %{_mandir}/man1/* should go into the main package, as these are manpages for stuff that goes to /usr/bin. * License tag should be MIT. ! Buildroot tag is no longer required. Does no harm though. Thanks for the help Orcan. I've addressed all of your points. The tests are currently failing - an error in the test suite that has been addressed upstream but is commented out for now. SRPM: http://bsjones.fedorapeople.org/lv2/lilv-0.5.0-2.fc16.src.rpm SPEC: http://bsjones.fedorapeople.org/lv2/lilv.spec Thank you for the update. Here is the rest of the review: * Missing BR: swig ! BR: python is redundant. Does no harm though. * BR: boost-devel and glib2-devel are redundant. It would be good to remove them so that we don't have to drag in unrelated packages during a build. ? Could you backport the fixes from the trunk and run the tests? * I am not sure we need to divide the Python modules in two subpackages. At the very least the noarch package should require the arch specific one, since lilv.py has import _lilv referring to _lilv.so. Can _lilv.so be used without lilv.py? If not, they should probably go to the same package (feel free to disagree :)). - rpmlint says: lilv.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/lilv lilv.x86_64: W: no-manual-page-for-binary lilv-bench python-lilv.noarch: W: no-documentation These can be ignored. * /usr/share/man/man3/* should go to the devel package as this is development documentation. Thanks Orcan, Addressed all your comments and worked with upstream to bring the test suite up to date. (Sure its possible to use the python lib outside the bindings but I can't see why you'd want to - merged the packages) SRPM: http://bsjones.fedorapeople.org/lv2/lilv-0.5.0-2.fc16.src.rpm SPEC: http://bsjones.fedorapeople.org/lv2/lilv.spec Scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=3849804 Disregard SRPM link in previous post, corrected here: SRPM: http://bsjones.fedorapeople.org/lv2/lilv-0.5.0-3.fc16.src.rpm SPEC: http://bsjones.fedorapeople.org/lv2/lilv.spec This is good now. Thanks for packaging this. --------------------------------------- This package (lilv) is APPROVED by oget --------------------------------------- Thank you for the review New Package SCM Request ======================= Package Name: lilv Short Description: An LV2 Resource Description Framework Library Owners: bsjones Branches: f15 f16 f17 InitialCC: Git done (by process-git-requests). lilv-0.5.0-3.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/lilv-0.5.0-3.fc16 lilv-0.5.0-3.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/lilv-0.5.0-3.fc17 Package lilv-0.5.0-3.fc17: * should fix your issue, * was pushed to the Fedora 17 testing repository, * should be available at your local mirror within two days. Update it with: # su -c 'yum update --enablerepo=updates-testing lilv-0.5.0-3.fc17' as soon as you are able to. Please go to the following url: https://admin.fedoraproject.org/updates/FEDORA-2012-3143/lilv-0.5.0-3.fc17 then log in and leave karma (feedback). lilv-0.5.0-3.fc16 has been pushed to the Fedora 16 stable repository. If problems still persist, please make note of it in this bug report. lilv-0.5.0-3.fc17 has been pushed to the Fedora 17 stable repository. If problems still persist, please make note of it in this bug report. |