Bug 233598

Summary: Review Request: elisa - Media Center
Product: [Fedora] Fedora Reporter: Matthias Saou <matthias>
Component: Package ReviewAssignee: Xavier Lamien <lxtnow>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: alexl, dwmw2, jarod, lemenkov, sdodson
Target Milestone: ---Flags: lxtnow: fedora‑review+
wtogami: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 530021 (view as bug list) Environment:
Last Closed: 2007-05-22 04:34:57 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On: 228298, 228299, 228301, 228303, 233596, 233597    
Bug Blocks:    

Description Matthias Saou 2007-03-23 08:34:21 EDT
Spec URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa.spec
SRPM URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa-0.1.4.2-1.src.rpm
Description:
Media center solution using the GStreamer multimedia framework.
Comment 1 Matthias Saou 2007-03-23 08:37:10 EDT
Note that this package depends on a few python packages as well as the pigment
toolkit, which will need to be reviewed first (see the bug dependencies).
Comment 2 Xavier Lamien 2007-03-23 12:11:27 EDT
Okay, 

i'will review all request dependencies before start this one
Comment 3 Matthias Saou 2007-04-16 07:01:33 EDT
Spec URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa.spec
SRPM URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa-0.1.5-1.src.rpm

* Mon Apr 16 2007 Matthias Saou <http://freshrpms.net/> 0.1.5-1
- Update to 0.1.5.
- Disable gst requirements which aren't part of Fedora (oops!).
- Patch out the hash-bang python from scripts not meant to be executed.
- Rip out the root user test condition to installing the desktop entry.
Comment 4 Jef Spaleta 2007-05-04 02:40:53 EDT
Quick pass over the spec file...

You'll need to add the appropriate desktop-file-install call in the install
section and add BuildRequires:  desktop-file-utils

-jef
Comment 5 Jef Spaleta 2007-05-04 02:48:51 EDT
Oh and the URL and SOURCE tags need to be updated, same thing as we went through
with pigment. 

www.fluendo.com -> elisa.fluendo.com  

Looks like elisa 0.1.6 is out according to the website. Do you want to bump up
to 0.1.6 before i start the real review?
http://elisa.fluendo.com/static/download/elisa/elisa-0.1.6.tar.gz

-jef
Comment 6 Jef Spaleta 2007-05-04 02:58:23 EDT
Does python-setuptools need to be a Requires and a BuildRequires?  The Requires
seems odd to me.

-jef
Comment 7 Matthias Saou 2007-05-04 09:02:31 EDT
Spec URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa.spec
SRPM URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa-0.1.6-1.src.rpm

Updated to 0.1.6 and changes the URLs.
IIRC the runtime dependency on python-setuptools is needed, otherwise elisa will
fail miserably (try it if you're unsure ;-))
As for the desktop file installation, I thought "processing" it was now only
required if it wasn't a sane file to begin with (i.e. missing categories, lines,
wrong encoding etc.)... did I make this up? :-/

Oh, and don't forget that coherence (bug #233596) still needs to be reviewed and
imported before elisa.
Comment 8 Matthias Saou 2007-05-07 07:23:07 EDT
Please note that the two patches I include have now been included upstream for
the next Elisa release (the desktop files one in a better form, since a check
still makes sense).
Comment 9 Matthias Saou 2007-05-07 10:42:03 EDT
Spec URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa.spec
SRPM URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa-0.1.6-2.src.rpm

* Mon May  7 2007 Matthias Saou <http://freshrpms.net/> 0.1.6-2
- Change coherence requirement to Coherence to match package name change.
Comment 10 Xavier Lamien 2007-05-07 13:06:28 EDT
Redundant Require:

python-twisted-core is required by Coherence package which is required by elisa.
Comment 11 Matthias Saou 2007-05-07 13:21:17 EDT
(In reply to comment #10)
> Redundant Require:
> 
> python-twisted-core is required by Coherence package which is required by elisa.

I wouldn't consider this a redundant require, since Coherence requires twisted,
but so does Elisa directly. If some day Coherence no longer requires it, or if
Elisa no longer requires Coherence, I want the requirement from within Elisa to
still be there.
Comment 12 Xavier Lamien 2007-05-07 14:48:10 EDT
okay,

The package python-tag (bug #228303) need to be accepted and imported first
before approved this one.
as it's a Require of elisa.

Comment 13 Matthias Saou 2007-05-09 07:18:53 EDT
Spec URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa.spec
SRPM URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa-0.1.6-3.src.rpm

* Tue May  8 2007 Matthias Saou <http://freshrpms.net/> 0.1.6-3
- Change Coherence requirement to python-Coherence to match package name change.

- python-Coherence should be imported and built shortly.
- python-tag is the only dependency left... I didn't hear back from the author
  yet :-(
Comment 14 Matthias Saou 2007-05-14 12:32:22 EDT
All dependencies are now in Fedora (python-tag has just been rebuilt).

It's definitely time for a complete review now... feel free! :-)

(Note that I also have another "Media Center" package waiting for a review :
oxine, based on the xine lib, bug #230549)
Comment 15 Xavier Lamien 2007-05-14 12:45:54 EDT
Well, good to know.

> (Note that I also have another "Media Center" package waiting for a review :
> oxine, based on the xine lib, bug #230549)

I'll check it out.

Starting review...
Comment 16 Xavier Lamien 2007-05-15 18:21:21 EDT
--------------------
From %install stage.
-------------------

you forgot to install the desktop file by using desktop-file-install command and
add desktop-file-utils as BR.

-------------------
From desktop file.
-------------------

you SHOULD Remove Application, X-Ximian-Main and X-Red-Hat-Base from Categories
entry
Comment 17 Matthias Saou 2007-05-16 14:07:35 EDT
> you forgot to install the desktop file by using desktop-file-install
> command and add desktop-file-utils as BR.

I'd prefer avoiding this. If the desktop file is sane enough and already
provided, it's fine to install it directly.

> you SHOULD Remove Application, X-Ximian-Main and X-Red-Hat-Base from
> Categories entry

Sure, I'll patch all this out. One interesting thing is that the current
rhythmbox, gnome-cd and gnome-volume-control desktop files on my FC6 system
contain the exact same lines.

Spec URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa.spec
SRPM URL: http://ftp.es6.freshrpms.net/tmp/extras/elisa/elisa-0.1.6-4.src.rpm

* Wed May 16 2007 Matthias Saou <http://freshrpms.net/> 0.1.6-4
- Patch desktop file to remove useless bits (Version and extra Categories).
Comment 18 Xavier Lamien 2007-05-17 03:22:04 EDT
>> I'd prefer avoiding this. If the desktop file is sane enough and already
>> provided, it's fine to install it directly.

By using this command you make the desktop file available on the GNOME/KDE/SFCE
menu (and/or panel).

Comment 19 Matthias Saou 2007-05-17 03:41:15 EDT
> > I'd prefer avoiding this. If the desktop file is sane enough and already
> > provided, it's fine to install it directly.
> 
> By using this command you make the desktop file available on the GNOME/KDE/SFCE
> menu (and/or panel).

Hum, I really don't think desktop-file-install has aything to do with this.
Maybe you're thinking of other tools like update-desktop-database and others,
run from scriplets, which do some "magic"?
Comment 20 Xavier Lamien 2007-05-20 09:59:26 EDT
(in reply to comment #19)
surely...^^

Well,

OK - Mock : Built on FC6 en F-7 (i386 and x86_64)
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License field in spec matches
OK - License is LGPL
OK - License match extras packaging policy licenses allowed
OK - License file is included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources SHOULD match upstream md5sum:
c9ce0b7b3519577b5f460b20c42e04c9  elisa-1.6.0.tar.gz
OK - Package has correct buildroot.
OK - BuildRequires isn't redundant.
OK - %build and %install stages is correct and work.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Changelog section is correct.

OK - Should function as described.
OK - Should package latest version

------------------------------------------------
Rpmlint output:
------------------------------------------------
OK - silent on both srpm and rpm.



==============
** APPROVED **
==============
Comment 21 Matthias Saou 2007-05-21 04:47:52 EDT
New Package CVS Request
=======================
Package Name: elisa
Short Description: Media Center
Owners: matthias@rpmforge.net
Branches: devel F-7 FC-6 EL-5
InitialCC: 
Comment 22 Matthias Saou 2007-05-22 04:34:57 EDT
The devel branch has been rebuilt, others are following now :-)
Comment 23 Alex Lancaster 2009-10-25 15:40:37 EDT
By the way, this package is being re-reviewed for a name from elisa->moovida at bug #530021.