Bug 426733
Summary: | Review Request: Mediatomb - UPnP AV Mediaserver for Linux | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Marc Wiriadisastra <marc> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | eric.tanguy, fedora-package-review, jin, kevin, mtasaka, notting |
Target Milestone: | --- | Flags: | mtasaka:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2008-01-05 00:47:06 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: |
Description
Marc Wiriadisastra
2007-12-25 15:58:41 UTC
I cannot sponsor your package, but I would like to help You. The Spec URL is dead. Rpmlint warns: mediatomb.src: W: mixed-use-of-spaces-and-tabs (spaces: line 41, tab: line 40) mediatomb.src: W: strange-permission mediatomb.spec 0600 P.S I'm looking for sponsor. spot, would you check if the following files are GPLv2 (strict) compatible? --------------------------------------------------------------------- tombupnp/upnp/src/inc/upnp_md5.h tombupnp/upnp/src/uuid/upnp_md5.c --------------------------------------------------------------------- Also, how does this application treats MP3? (I am not familiar with this application) http://fedoraproject.org/wiki/Multimedia/MP3 My understanding is that it only shows the tags listed using id3 whatever codecs you have on your computer is what it plays so technically it would only play Ogg on a default Fedora install. Currently Supported Features *browse and playback your media via UPnP *metadata extraction from mp3, ogg, flac, jpeg, etc. files. That was the word I was looking for metadata extraction it uses the driver's off your system. For 0.10.0-1 * Requires - Requires: id3lib, file, sqlite, js" are redundant. rpmbuild examines the dependencies of libraries and adds them to the rebuilt binary rpms automatically. For example, "rpm -q --requires mediatomb" contains libid3-3.8.so.3, which should pull id3lib automatically - And please verify if "mysql" itself is needed. If only "mysql-libs" is needed, "Requires: mysql" should be removed. * Conditional BuildRequires - build.log says: -------------------------------------------------------------- 327 CONFIGURATION SUMMARY ---- 328 329 sqlite3 : yes 330 mysql : yes 331 libjs : yes 332 libmagic : yes 333 inotify : yes 334 libexif : yes 335 id3lib : yes 336 taglib : disabled 337 libextractor : disabled -------------------------------------------------------------- Please explain why you don't want to support taglib or libextrator (both are already in Fedora repo) * Timestamps - To keep timestamps on installed files, please use --------------------------------------------------------------- make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p" --------------------------------------------------------------- While sometimes this does not work, this method usually works for recent Makefiles. - When using "install" or "cp" commands, please use "-p" option to keep timestamps, for example: ---------------------------------------------------------------- install -p -D -m0755 scripts/mediatomb-service-fedora %{buildroot}%{_initrddir}/mediatomb ---------------------------------------------------------------- * Unowned files/directories - $ cat -n /etc/rc.d/init.d/mediatomb returns: ---------------------------------------------------------------- 57 mkdir -p "/$MT_HOME/$MT_CFGDIR" 58 chown nobody "/$MT_HOME/$MT_CFGDIR" ---------------------------------------------------------------- So actually mediatomb service creates /etc/mediatomb/ directory with the owner nobody. However, this directory is not owned by any rpms: ---------------------------------------------------------------- $ LANG=C rpm -qf /etc/mediatomb/ file /etc/mediatomb is not owned by any package ---------------------------------------------------------------- * /etc/mediatomb must be owned by mediatomb with owner nobody * Also, IMO all files under /etc/mediatomb must be owned by mediatomb (perhaps by using %ghost) * Usually service should use its unique user/group. You can refer to: http://fedoraproject.org/wiki/Packaging/UsersAndGroups * scriptlets/Requires for services (post, preun, etc) - scriptlets/Requires for mediatomb service are wrong. Please check the section "Services" of http://fedoraproject.org/wiki/Packaging/ScriptletSnippets Also, please check the section "Syntax" "Scriptlet Ordering" of the same wiki page. * Documents - The file "INSTALL" is usually needed for people who want to install the software by themselves and is not needed when people installs it using rpm. ! Note: Please make it sure that you change the release number of spec file each time you modify your spec file. There is a choice to use sqlite or mysql I've added both as dependencies depending on which one the user wants to use. I will adjust the other errors and upload a new one shortly. I've adjusted the numbering because of the changelog that originally came from upstream I have added some patches for the ownership changes. This is a quote from the README Note: you need at least one database in order to compile and run MediaTomb - either sqlite or mysql. I know in Debian you can use || for or but I'm not to sure what it is in Fedora. http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-0.10.0-3.fc8.src.rpm http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb.spec http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-service.patch http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-conf-fedora.patch For 0.10.0-3: * Conditional BuildRequires: - Still build.log says: ----------------------------------------------------- 327 CONFIGURATION SUMMARY ---- 328 329 sqlite3 : yes 330 mysql : yes 331 libjs : yes 332 libmagic : yes 333 inotify : yes 334 libexif : yes 335 id3lib : yes 336 taglib : disabled 337 libextractor : disabled ----------------------------------------------------- A. For taglib, configure.ac says: ----------------------------------------------------- 1659 AC_ARG_ENABLE(id3lib, 1660 AC_HELP_STRING([--enable-id3lib], [compile with taglib support (default: yes, preferred over taglib)]), 1677 AC_ARG_ENABLE(taglib, 1678 AC_HELP_STRING([--enable-taglib], [compile with taglib support (default: yes, if id3lib is not available)]), ----------------------------------------------------- So as id3lib is preferred, having taglib disabled can be ignored. B. For libextractor, configure.ac says: ----------------------------------------------------- 1982 AC_ARG_ENABLE(libextractor, 1983 AC_HELP_STRING([--enable-libextractor], [compile with libextractor support (default: no)]), ----------------------------------------------------- So it seems that explicit configure option "--enable-libextractor" is needed. * Patches - Patch0 and Patch1 are not applied?? * Macros - Please use %_sysconfdir for /etc (useradd -r -g mediatomb -d /etc/mediatomb ..... ) %_localstatedir for /var - Also, for consistency please replace with make -> %__make install -> %__install rm -> %__rm * Directory ownership - Still %_sysconfdir/mediatomb/ is not owned by this package and you removed the lines to create/chown the directory %_sysconfdir/mediatomb/ by %PATCH0. Perhaps with this "service mediatomb" won't start. Also, when removing mediatomb rpm, all files under %_sysconfdir/mediatomb are left unremoved as these files are not owned by any package To - Create %_sysconfdir/mediatomb with proper ownership - Make it sure that all files under %_sysconfdir/mediatomb/ are removed properly when removing mediatomb Please add following: ------------------------------------------------------------ %install ........ %{__make} install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p" # make all files under %%_sysconfdir/mediatomb are owned by # this package %{__mkdir_p} $RPM_BUILD_ROOT%{_sysconfdir}/mediatomb touch $RPM_BUILD_ROOT%{_sysconfdir}/mediatomb/{config.xml,mediatomb.db,mediatomb.html} %{__mkdir_p} %{buildroot}%{_sysconfdir}/logrotate.d ...... %files %defattr(-,root,root,-) ...... %attr(-,mediatomb,mediatomb) %config(noreplace) %{_sysconfdir}/mediatomb.conf %attr(-,mediatomb,mediatomb) %dir %{_sysconfdir}/mediatomb/ %attr(-,mediatomb,mediatomb) %ghost %{_syscondir}/mediatomb/* %config(noreplace) %{_sysconfdir}/logrotate.d/%{name} ...... ------------------------------------------------------------ (In reply to comment #8) > Note: > > you need at least one database in order to compile and run MediaTomb - > either sqlite or mysql. > > I know in Debian you can use || for or but I'm not to sure > what it is in Fedora. - For this package it is easier to write "Requires: mysql" (as you are doing now) because mediatomb binary is already linked against both sqlite and mysql library. I have uploaded the latest versions of the spec file and actually used the patches this time. I've tested it on my computer the only issue it mediatomb doesn't have access to my home directory. That is not much of an issue since if it had permission it would be a security risk. http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-0.10.0-4.fc8.src.rpm http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb.spec http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-conf-fedora.patch http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-service.patch Thanks so much for your effort I appreciate it since I'm learning a lot from it. Just for a bit of information I have spoken to upstream and he has read this thread and his response to the questions is below. Regarding the md5sum implementation, it comes with libupnp and we were pointed to the license issues by Debian people; we received a patch that replaces this md5sum implementation with a free one, it will be applied shortly. For Fedora it would make sense to compile a version with taglib support (because of their no mp3 policy it would make sense to take a library that is capable of parsing ogg metadata). Only mysql client libraries are needed for compilation, but mysql server is needed to run MediaTomb in mysql mode (however the mysql server can be on a remote system as well; in our binary packages we usually compile with sqlite and mysql support so people can easily switch to whatever they like best. It seems Mediatomb uses it's own private copy of libupnp and is not the last one. libupnp is already part of fedora (1.6.2 in stable and 1.6.3 pending) so please patch Mediatomb to use the libupnp provides by fedora. If you have contact upstream i will be please to know why they use a private copy of libupnp. I'm one of the MediaTomb devs; we use a heavily patched version of libupnp, partially bugfixes, partially feature extensions, we branched off long time ago and did quite a few changes so it's very difficult to merge with 1.6.2 or later. We plan to get rid of libupnp soon, we will replace it by our own code, work on that will start after the upcoming 0.11.0 version of MediaTomb is released. Your bugfixes and feature extensions have been submitted to pupnp project which is an active one since 1.6.0 ? Do you know also libdlna (http://libdlna.geexbox.org/) which is dlna extension and is part of livna ? I'm not sure using your own code will be the best solution but it's up to you ... The changes we made to libupnp are under LGPLv2 while libupnp itself is under BSD (I checked with the FSF regarding this approach and it's OK). I asked libupnp people if they would like to move to LGPL, however the thread faded away without any results... basically meaning that our code can not be used in their tree. I am aware of libdlna, however it is my understanding that it only adds DLNA extensions, it does not replace libupnp. For 0.10.0-4: Well, now for me your spec file is okay except for - upnp_md5.{h.c} license issue - Some cosmetic issues * Well I was a bit just lazy but please put macros in braces for consistency... * And also for consistency please choose $RPM_BUILD_ROOT or %{buildroot}, not using both. Then as this is NEEDSPONSOR ticket: ------------------------------------------------------------- NOTE: Before being sponsored: This package will be accepted with another few work. But before I accept this package, someone (I am a candidate) must sponsor you. Once you are sponsored, you have the right to review other submitters' review requests and approve the packages formally. For this reason, the person who want to be sponsored (like you) are required to "show that you have an understanding of the process and of the packaging guidelines" as is described on : http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored Usually there are two ways to show this. A. submit other review requests with enough quality. B. Do a "pre-review" of other person's review request (at the time you are not sponsored, you cannot do a formal review) When you have submitted a new review request or have pre-reviewed other person's review request, please write the bug number on this bug report so that I can check your comments or review request. Fedora package collection review requests which are waiting for someone to review can be checked on: http://fedoraproject.org/PackageReviewStatus/NEW.html (NOTE: please don't choose "Merge Review") Review guidelines are described mainly on: http://fedoraproject.org/wiki/Packaging/ReviewGuidelines http://fedoraproject.org/wiki/Packaging/Guidelines http://fedoraproject.org/wiki/Packaging/ScriptletSnippets ------------------------------------------------------------ Strictly speaking, this would normally be a problem, as those files are licensed BSD with advertising (which is GPL incompatible). However, the RSA MD5 implementation is a special case, where RSA has given everyone permission to use their implementations "without license from RSA", so that is how Fedora will use that code. You should advise upstream to read: http://www.redhat.com/archives/fedora-legal-list/2008-January/msg00002.html Upstream should also be advised to strongly consider "using" that code without the RSA license, to avoid this sort of licensing confusion. Lifting FE-Legal. Updated Spec file and SRPM. The patches are there for completeness nothing has been altered from previous patches. http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb.spec http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-0.10.0-6.fc8.src.rpm http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-conf-fedora.patch http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-service.patch I have several review requests main reason they have not been approved is due to upstream. https://bugzilla.redhat.com/show_bug.cgi?id=389971 https://bugzilla.redhat.com/show_bug.cgi?id=372161 https://bugzilla.redhat.com/show_bug.cgi?id=333491 Well, Please pull "BuildRequires: mysql-libs" back to "BuildRequires: mysql-devel" to make mediatomb binary linked againt mysql library (Note: installing mysql-devel auto installs mysql, so also mysql-libs) Other things are okay (I saw 3 your other review requests quickly) ------------------------------------------------------------------- This package (mediatomb) is APPROVED by me ------------------------------------------------------------------- Please follow the procedure according to: http://fedoraproject.org/wiki/PackageMaintainers/Join from "Get a Fedora Account". At a point a mail should be sent to sponsor members which notifies that you need a sponsor (at the stage, please also write on this bug for confirmation that you requested for sponsorship) Then I will sponsor you. If you want to import this package into Fedora 7/8, you also have to look at http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT (after once you rebuilt this package on Fedora rebuilding system). If you have questions, please ask me. (In reply to comment #17) > Strictly speaking, this would normally be a problem, as those files are licensed > BSD with advertising (which is GPL incompatible). > > However, the RSA MD5 implementation is a special case, where RSA has given > everyone permission to use their implementations "without license from RSA", so > that is how Fedora will use that code. > > You should advise upstream to read: > http://www.redhat.com/archives/fedora-legal-list/2008-January/msg00002.html > > Upstream should also be advised to strongly consider "using" that code without > the RSA license, to avoid this sort of licensing confusion. > > Lifting FE-Legal. Okay, thank you for clarifying! (In reply to comment #19) > Well, Please pull "BuildRequires: mysql-libs" back to > "BuildRequires: mysql-devel" to make mediatomb binary linked againt mysql > library > (Note: installing mysql-devel auto installs mysql, so also mysql-libs) > > Other things are okay (I saw 3 your other review requests quickly) > > ------------------------------------------------------------------- > This package (mediatomb) is APPROVED by me > ------------------------------------------------------------------- > Done http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb.spec http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-0.10.0-7.fc8.src.rpm http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-conf-fedora.patch http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-service.patch > Please follow the procedure according to: > http://fedoraproject.org/wiki/PackageMaintainers/Join > from "Get a Fedora Account". > At a point a mail should be sent to sponsor members which notifies > that you need a sponsor (at the stage, please also write on > this bug for confirmation that you requested for sponsorship) > Then I will sponsor you. I have done that already. It's listed as unapproved. > > If you want to import this package into Fedora 7/8, you also have > to look at > http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT > (after once you rebuilt this package on Fedora rebuilding system). > > If you have questions, please ask me. > > I will look into it thanks again for the help. I also do need a sponsor. Now I should be sponsoring you. Please proceed. New Package CVS Request ======================= Package Name: mediatomb Short Description: MediaTomb is an open source (GPL) UPnP MediaServer Owners: mwiriadi Branches: F8 F9 InitialCC: mwiriadi Cvsextras Commits: yes cvs done. I assume you wanted F-8 and devel branches? There isn't a F-9 branch yet, and won't be until much later in this devel cycle. Yep thanks Kevin I'm not to sure about F-7 is there a way apart from doing the build to test whether it functions fine? Well, if you don't want to maintain your package on F-7 it is okay. By the way I have only rawhide machine but I maintain my packages on devel, F-8 and F-7 :) Anyway please try to rebuild this rpm by koji on devel, F-8. For F-8, after rebuilding this rpm please also refer to Bodhi-info-DRAFT wiki. Yep I saw that I'm planning on doing that anyways to test it. Thanks for that. I have just received an email from upstream who will send me a patch which will fix x86_64 I'm inquiring further about ppc. If the package is fixed for x86_64 do I need to resubmit it for review? Do I just change the spec file and resubmit it to CVS? (In reply to comment #28) > I have just received an email from upstream who will send me a patch which will > fix x86_64 I'm inquiring further about ppc. > > If the package is fixed for x86_64 do I need to resubmit it for review? Do I > just change the spec file and resubmit it to CVS? You can fix your srpm on koji. Resubmit is not needed. Package Change Request ====================== Package Name: mediatomb New Branches: FC-7 cvs done. |