Bug 426733

Summary: Review Request: Mediatomb - UPnP AV Mediaserver for Linux
Product: [Fedora] Fedora Reporter: Marc Wiriadisastra <marc>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: 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
Spec URL: http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb.spec
SRPM URL: http://mwiriadi.fedorapeople.org/packages/mediatomb/mediatomb-0.10.0-1.fc8.src.rpm
Description: MediaTomb is an open source (GPL) UPnP MediaServer with a nice web user 
interface, it allows you to stream your digital media through your home
network and listen to/watch it on a variety of UPnP compatible devices.

MediaTomb implements the UPnP MediaServer V 1.0 specification that can 
be found on http://www.upnp.org/

RPMLint: mediatomb.i386: W: no-reload-entry /etc/rc.d/init.d/mediatomb
In your init script (/etc/rc.d/init.d/your_file), you don't
have a 'reload' entry, which is necessary for good functionality.

I have spoken to upstream they do not feel that reload is needed.

Comment 1 Krzysztof Kurzawski 2007-12-25 16:17:52 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.


Comment 2 Mamoru TASAKA 2008-01-01 17:34:35 UTC
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
---------------------------------------------------------------------

Comment 3 Mamoru TASAKA 2008-01-01 17:58:23 UTC
Also, how does this application treats MP3? (I am not familiar
with this application)

http://fedoraproject.org/wiki/Multimedia/MP3

Comment 4 Marc Wiriadisastra 2008-01-01 22:21:08 UTC
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.

Comment 5 Marc Wiriadisastra 2008-01-01 22:33:55 UTC
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.

Comment 6 Mamoru TASAKA 2008-01-02 03:37:19 UTC
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.


Comment 7 Marc Wiriadisastra 2008-01-02 03:51:49 UTC
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.

Comment 8 Marc Wiriadisastra 2008-01-02 04:59:24 UTC
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

Comment 9 Mamoru TASAKA 2008-01-02 10:01:01 UTC
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.


Comment 10 Marc Wiriadisastra 2008-01-02 12:14:43 UTC
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.

Comment 11 Marc Wiriadisastra 2008-01-02 12:25:05 UTC
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.

Comment 12 Eric Tanguy 2008-01-02 16:00:00 UTC
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.


Comment 13 Sergey Bostandzhyan 2008-01-02 16:10:22 UTC
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.

Comment 14 Eric Tanguy 2008-01-02 16:20:25 UTC
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 ...

Comment 15 Sergey Bostandzhyan 2008-01-02 16:36:40 UTC
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.


Comment 16 Mamoru TASAKA 2008-01-02 16:42:59 UTC
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
------------------------------------------------------------


Comment 17 Tom "spot" Callaway 2008-01-02 22:19:57 UTC
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.

Comment 19 Mamoru TASAKA 2008-01-03 01:08:26 UTC
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.



Comment 20 Mamoru TASAKA 2008-01-03 01:09:16 UTC
(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!



Comment 21 Marc Wiriadisastra 2008-01-03 01:32:09 UTC
(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.

Comment 22 Mamoru TASAKA 2008-01-03 01:41:25 UTC
Now I should be sponsoring you. Please proceed.

Comment 23 Marc Wiriadisastra 2008-01-03 03:40:26 UTC
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

Comment 24 Kevin Fenzi 2008-01-03 04:40:28 UTC
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. 



Comment 25 Marc Wiriadisastra 2008-01-03 04:46:15 UTC
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?

Comment 26 Mamoru TASAKA 2008-01-03 07:48:14 UTC
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.

Comment 27 Marc Wiriadisastra 2008-01-03 08:24:42 UTC
Yep I saw that I'm planning on doing that anyways to test it.  Thanks for that.

Comment 28 Marc Wiriadisastra 2008-01-03 14:09:37 UTC
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?

Comment 29 Mamoru TASAKA 2008-01-03 14:51:47 UTC
(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.

Comment 30 Marc Wiriadisastra 2008-01-04 23:51:06 UTC
Package Change Request
======================
Package Name: mediatomb
New Branches: FC-7


Comment 31 Kevin Fenzi 2008-01-05 00:29:11 UTC
cvs done.