Bug 438608 - Review Request: elisa-plugins-good - Good Plugins for the Elisa Media Center
Review Request: elisa-plugins-good - Good Plugins for the Elisa Media Center
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On: 457196
Blocks: 429590
  Show dependency treegraph
 
Reported: 2008-03-22 15:54 EDT by Matthias Saou
Modified: 2009-02-28 09:42 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-28 09:42:33 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
michel: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Matthias Saou 2008-03-22 15:54:11 EDT
Spec URL: http://thias.fedorapeople.org/review/elisa-plugins-good/elisa-plugins-good.spec
SRPM URL: http://thias.fedorapeople.org/review/elisa-plugins-good/elisa-plugins-good-0.3.5-1.src.rpm
Description:
This package contains the good set of plugins for the Elisa Media Center.

Note: This package is split off from the previous "elisa" package, which now requires it.
Comment 1 Thomas Vander Stichele 2008-04-20 03:51:05 EDT
I suggest require'ing libvisual-plugins - the default config uses libvisual_jess
as visualisation and Elisa doesn't say anything about the vis plugin being
missing atm.
Comment 2 Matthias Saou 2008-07-30 04:43:54 EDT
Adding dependency on the pymetar review, since the weather plugin in 0.5 seems
to have switched from python-metar to pymetar.
Comment 3 Matthias Saou 2008-07-30 04:45:22 EDT
Note that the package has now been updated to 0.5.3, and is still available here :
http://thias.fedorapeople.org/review/elisa-plugins-good/
Comment 4 Peter Robinson 2008-07-31 09:59:04 EDT
I can't build this package. There are circular dependencies. The elisa base
package provides elisa and elisa-common (which in turn provides a virtual
elisa-devel) but they both require elisa-plugins-*. elisa-plugins-good requires
elisa-devel. Is there even a need for an elisa-common package? Can elisa or
anything else run without the common package? Can you use some other tool with
elisa-common without the elisa package? (I know not really part of this review).
Comment 5 Matthias Saou 2008-08-08 07:12:25 EDT
To reply to the last comment, the "elisa-common" package is a hack to break the circular dependency that exists.

Here's the problem : You need to install elisa before you can "build" and install the plugins. Installing from source, this isn't really a problem. But from packages, it is, as normal users will want to install only "elisa" and have it pull in all the plugins it needs to run. So for packages, we need elisa to require the plugins, but we need the plugins to buildrequire elisa. Circular dependency.

To break that circular dependency, I've split out most of the "elisa" files into a bogus "elisa-common" package, which doesn't require the plugins. To build the plugins, this is fine. To make it more obvious, I've used a virtual provides/requires of "elisa-devel", but haven't named the package "devel" since it will be installed for normal runtime.

I've double checked, and the "elisa-common" package doesn't require the plugins, and it shouldn't as explained above. I don't know how you ran into a problem here.

The only drawback I can see here is that after removing "elisa", the plugins will be gone, but not the "elisa-common" package. The only solution I can think of would be to duplicate the files currently in "elisa-common" in "elisa" and rename the "common" to "devel". We'd have two packages nearly identical, with one used only for building the plugins.
Comment 6 Peter Robinson 2008-08-08 12:14:11 EDT
Hi,

The problem with "To break that circular dependency, I've split out most of the "elisa" files into a bogus "elisa-common" package, which doesn't require the plugins." is that the elisa-common rpm is built from the elisa src rpm so even by splitting it out you still end up with the circular dependency. From the quick test I did it seems you can build elisa fine without the plugins installed (and it will even run but it just won't be of any use). So I think to fix the issue you could depend on it for install but not for build or something similar but I'm not sure whether that would word either.

Peter
Comment 7 Matthias Saou 2008-08-11 14:10:32 EDT
I'm really confused as to what the problem you've encountered is, because things should be as follows, thus avoiding any circular dependency :
 * The "elisa" package DOES NOT "BuildRequires" any of the plugins
 * The "elisa" package DOES "Requires" the plugins
 * The "elisa-common" package DOES NOT "Requires" the plugins
   (and provides "elisa-devel")
 * The plugins DO NOT "BuildRequire" "elisa" (to avoid the circular dep!)
 * The plugins DO "BuildRequire" "elisa-devel" (provided by "elisa-common")
 * The plugins DO "Require" "elisa"

So as long as you rebuild "elisa" first, then the plugins (in any order), things should "just work". Obviously, after rebuilding only "elisa", you can install "elisa-common" (aka "elisa-devel") BUT NOT YET "elisa".

Would you care to detail what your problem is? Maybe I have something busted in my build environment which is hiding a problem from me.
Comment 8 Michel Alexandre Salim 2008-08-26 13:22:11 EDT
(In reply to comment #5)
> The only drawback I can see here is that after removing "elisa", the plugins
> will be gone, but not the "elisa-common" package. The only solution I can think
> of would be to duplicate the files currently in "elisa-common" in "elisa" and
> rename the "common" to "devel". We'd have two packages nearly identical, with
> one used only for building the plugins.

I was about to suggest combining elisa with elisa-plugins-good, but if the GUI itself is in elisa-plugins-bad...

How about making the %postun of elisa be 'rpm -e elisa-common' ? Not that it is that big of a deal.
Comment 9 Matthias Saou 2008-08-26 13:36:06 EDT
(In reply to comment #8)
> How about making the %postun of elisa be 'rpm -e elisa-common' ? Not that it is
> that big of a deal.

Eek, that's waaaay too ugly :-)
If the current way is to be changed, it would be to have the two nearly identical packages, wasting a little bit of mirror server space (less than 1MB).
Comment 10 Michel Alexandre Salim 2008-08-26 14:48:16 EDT
That might be a good idea -- 1 MB is nothing, really nowadays. So elisa-common can be called elisa-devel, and should only be used for building plugins.

Will proceed with the review (reviewing -bad as well) once I could actually install the whole thing. I commented on the twisted-web2 review asking if you could upload the twisted-core package you used for building -web2.
Comment 11 Valent Turkovic 2008-10-04 06:24:23 EDT
Will these new elisa packages go into Fedora 9? Please update Elisa for F9 because it currently is broken :(
Comment 12 Matthias Saou 2008-10-07 08:21:51 EDT
The "new" elisa sub-packages can't go into Fedora until they get reviewed and approved...
Comment 13 Michael Schwendt 2008-10-07 10:57:09 EDT
* The original elisa-common split was fine, IMO. No need to duplicate
files. And no need to add a virtual elisa-devel. We have tools in
non-devel packages that are BuildRequires. elisa-common can be BR,
too.

* The %description could explain briefly what "good set of plugins"
means and why it matters.

* 0.5.13 is available.

Packaging-wise this is fine. Could be imported and developed further
in cvs until the deps are ready, too.
Comment 14 Matthias Saou 2008-10-10 08:19:46 EDT
(In reply to comment #13)
> * The original elisa-common split was fine, IMO. No need to duplicate
> files. And no need to add a virtual elisa-devel. We have tools in
> non-devel packages that are BuildRequires. elisa-common can be BR,
> too.

Indeed. What about using the "elisa-base" name instead of common or devel? I just thought of it, as those files are indeed common to the runtime and the build of elisa, but not to be required by anything else, only build required. So the name "elisa-base" seems better suited, since those files are the basic elisa files. Sounds good?

> * The %description could explain briefly what "good set of plugins"
> means and why it matters.

I'll update to :

This package contains the good set of plugins for the Elisa Media Center,
plugins which are considered stable and do not present any licensing issues.

> * 0.5.13 is available.
> 
> Packaging-wise this is fine. Could be imported and developed further
> in cvs until the deps are ready, too.

I'm assuming you mean the main "elisa" package. Seems like a good idea, I'll do that.

Thanks for all your comments.
Comment 15 Michael Schwendt 2008-10-17 06:17:15 EDT
elisa-base or elisa-common, both names sound reasonable.

And no, the bottom of my comment 13 referred to elisa-plugins-good
actually, not the main elisa package.
Comment 16 Matthias Saou 2008-10-20 05:41:34 EDT
(In reply to comment #15)
> And no, the bottom of my comment 13 referred to elisa-plugins-good
> actually, not the main elisa package.

Do you mean importing the plugins spec files into the "elisa" CVS directory? That seems like abusing the CVS structure to me, so maybe I'm missing something.
Comment 17 Michael Schwendt 2008-10-20 08:01:27 EDT
This is the review of _elisa-plugins-good_ and I suggested:

> Packaging-wise this is fine. Could be imported and developed
> further in cvs until the deps are ready, too.

That translates to "I think this could be approved, so further
development [until the deps are ready] can be done in cvs".

Note that the review ticket is not assigned to me.
Comment 18 Peter Robinson 2008-10-20 08:16:23 EDT
(In reply to comment #17)
> This is the review of _elisa-plugins-good_ and I suggested:

Package reviews must follow the guidelines outlined here https://fedoraproject.org/wiki/Packaging/ReviewGuidelines

> > Packaging-wise this is fine. Could be imported and developed
> > further in cvs until the deps are ready, too.
> 
> That translates to "I think this could be approved, so further
> development [until the deps are ready] can be done in cvs".

How can it be imported and developed in the RPM cvs if it won't build against rawhide due to the lack of deps which is one of the requirements of the package review?
Comment 19 Michael Schwendt 2008-10-20 10:43:55 EDT
What is your problem?


cvs-import.sh takes src.rpm packages. There is no requirement
for imported packages to be built in koji immediately. "elisa"
in rawhide cvs is up-to-date, though, but it would not make
sense to publish an incomplete set of builds. It's just more
convenient to work on pkgs cvs rather than downloading a full
set of src.rpms from an external web site.
Comment 20 Alex Lancaster 2008-12-14 03:40:02 EST
Has this review stalled?  elisa 0.5.20 is now in rawhide but has broken deps because it requires elisa-plugins-good, see bug #429590 comment #43.
Comment 21 Matthias Saou 2008-12-30 15:52:39 EST
Michel : Could you please check the latest package and resume the review? Now that web2 has been approved, the new plugins packages should be able to go in easily.

http://thias.fedorapeople.org/review/elisa-plugins-good/elisa-plugins-good.spec
http://thias.fedorapeople.org/review/elisa-plugins-good/elisa-plugins-good-0.5.22-1.src.rpm
Comment 22 Michael Schwendt 2009-01-02 11:42:06 EST
> -License: GPLv3
> +# Each plugin has its license information in its setup.py
> +License: GPLv3 and MIT

Doubtful. While the tiny gnome plugins setup.py file lists the MIT licence and Benjamin Kampmann as the author (and using a non-Fluendo address that doesn't appear anywhere else in Elisa), the main plugin code uses the GPL v3 and lists Gernot Klimscha as the author. Enough reason to believe this thing is just GPLv3.
Comment 23 Matthias Saou 2009-01-02 12:34:17 EST
You're absolutely right. I'll report this so that the PKG-INFO file gets fixed. I'm sure it was different before, the field was wrong because the files were cleaned up recently. But I've now double checked and changed to plain "GPLv3". Fixed package available.
Comment 24 Valent Turkovic 2009-01-02 14:41:33 EST
What is your estimated time frame of when working version of elisa will be in Fedora 10 repos? Is it few days, weeks or months? 

I'm just not a developer so I can't keep track of your conversations but would like to test it as soon as it gets into testing Fedora repos. Thank you very much for your hard work!
Comment 25 Matthias Saou 2009-01-08 13:00:55 EST
(In reply to comment #10)
> Will proceed with the review (reviewing -bad as well) once I could actually
> install the whole thing. I commented on the twisted-web2 review asking if you
> could upload the twisted-core package you used for building -web2.

All requirements are now in F-9 and F-10 too. Testing for the review should be very easy now! Michel : Could you resume the review? TIA!
Comment 26 Alex Lancaster 2009-01-22 23:07:56 EST
Is this review going anywhere?   The lack of this package is still causing broken deps in rawhide:

Broken deps for i386
----------------------------------------------------------
	elisa-0.5.20-1.fc11.noarch requires elisa-plugins-good = 0:0.5.20
	elisa-0.5.20-1.fc11.noarch requires elisa-plugins-bad = 0:0.5.20
	elisa-0.5.20-1.fc11.noarch requires mgopen-fonts

If this review is abandoned, I will try and pick it up.
Comment 27 Michel Alexandre Salim 2009-01-24 20:38:36 EST
The URL has changed, presumably?

http://thias.fedorapeople.org/review/elisa-plugins-good/elisa-plugins-good-0.5.23-1.src.rpm

Reviewing right now.
Comment 28 Michel Alexandre Salim 2009-01-25 12:51:16 EST
ReviewTemplate

PASSED

• rpmlint:
  Clean, but no documentation. See licensing comments below
• package name
• spec file name
• package guideline-compliant
• license complies with guidelines:
  Yes
• license field accurate
• license file not deleted
• spec in US English
• spec legible
• source matches upstream
• builds under >= 1 archs, others excluded
  noarch
• build dependencies complete
• own all directories
• no dupes in %files
• permission
• %clean RPM_BUILD_ROOT
• macros used consistently
• Package contains code
• clean buildroot before install
• filenames UTF-8

SHOULD
• if license text missing, ask upstream to include it
  License file seems to come with the base elisa distribution. Could upstream
  be asked to ship this with the plugins tarball as well?
• package build in mock on all architectures
• package functioned as described:
  Works, tested with audio playback on F10/x86_64. Note that video playback locks up the machine, but that is probably an Elisa / Elisa-plugins-bad problem 
• scriplets are sane
• other subpackages should require versioned base
• require package not files

Approved.
Comment 29 Matthias Saou 2009-01-26 04:54:31 EST
Thanks a lot for the review!!!!

(In reply to comment #28)
> SHOULD
> • if license text missing, ask upstream to include it
>   License file seems to come with the base elisa distribution. Could upstream
>   be asked to ship this with the plugins tarball as well?

Since there are circular deps between the plugins and the base, the plugins can't be installed without the base, so I'd consider this very minor and wouldn't even bother with it.

> • other subpackages should require versioned base

That's a tricky bit. All of the packages are usually released with the same version, but it has happened to see only one be immediately replaced with an "n.1" version, making the whole "Requires: foo = %{version}" (strict version) quite tricky, as it could involve useless rebuilds.

Maybe having all of the defaults be "Requires: foo >= %{version}" then exceptionally hardcoding to "Requires: foo >= 0.5.24" in a single packages updated to 0.5.24.1?

> • require package not files

Where is there a file requirement?

Again, thanks for the review, we're now one step closer to get the Fedora elisa back into shape at last!
Comment 30 Matthias Saou 2009-02-17 04:53:46 EST
New Package CVS Request
=======================
Package Name: elisa-plugins-good
Short Description: Good Plugins for the Elisa Media Center
Owners: thias
Branches: F-9 F-10
InitialCC:
Comment 31 Kevin Fenzi 2009-02-18 14:53:58 EST
cvs done.
Comment 32 Matthias Saou 2009-02-28 09:42:33 EST
Thanks a lot Kevin. The package has been imported and built fine. We only have but #438609 to go before elisa is usable again in Fedora!

Note You need to log in before you can comment on or make changes to this bug.