Bug 190878 - Review Request: childsplay_plugins - Plugins for childsplay (educational games for young children)
Review Request: childsplay_plugins - Plugins for childsplay (educational game...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Wart
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 190876
  Show dependency treegraph
 
Reported: 2006-05-05 16:49 EDT by Hans de Goede
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-05-11 15:49:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
improved specfile (2.07 KB, text/plain)
2006-05-10 04:05 EDT, Hans de Goede
no flags Details
improved specfile (2.42 KB, text/plain)
2006-05-11 09:40 EDT, Hans de Goede
no flags Details
improved specfile (2.50 KB, text/plain)
2006-05-11 10:21 EDT, Hans de Goede
no flags Details
improved specfile (2.37 KB, text/plain)
2006-05-11 13:06 EDT, Hans de Goede
no flags Details

  None (edit)
Description Hans de Goede 2006-05-05 16:49:09 EDT
Spec URL: http://home.zonnet.nl/jwrdegoede/childsplay_plugins.spec
Description:
Plugins (games) for Childsplay a 'suite' of educational games for young
children. The aim is to be educational and at the same time be fun to play.

---

Sorry no SRPM, because the srpm size exceeds my homepage's max filesize. There are no patches or anything special though, so all you need is the spec and the upstream tarball which you can get upstream.

These plugins require childsplay to operate see bug 190876
Comment 1 Wart 2006-05-09 23:55:26 EDT
No patches?  Then what's "Patch0: ..." doing there?  ;)

If you can give me a url for the patch, or tell me that it's not needed, then
I'll continue my review of childsplay and this package.
Comment 3 Hans de Goede 2006-05-10 04:05:25 EDT
Created attachment 128837 [details]
improved specfile

Similar improvements as done to childplay, ghost .pyo files, etc.
Comment 4 Wart 2006-05-10 20:30:48 EDT
Since rawhide is borked right now I haven't been able to test mock builds, which
is also how I verify the BR: on packages.  I'll get to that as soon as rawhide
is working again.

MUST
====
* rpmlint output clean
* GPL license ok
* Spec file legible and in Am. English
* Upstream source used during review
  d3ea05d2a1fb373d9c4836845b199a76  childsplay_plugins-0.80.7.tgz
* $RPM_BUILD_ROOT cleaned correctly
* No -devel package needed
* No shared libs
* No locales
* No need for -docs subpackage
* No .desktop file needed

MUSTFIX
=======

* Source0: url points to the Sourceforge mirror selection page.  Better
  to use dl.sourceforge.net or hardcode a mirror so that tools like
  spectool can be used to download the source files. (the same is true for
childsplay, which I failed to notice earlier)

* 'yum remove childsplay childsplay_plugins' left two dangling directories
  on the filesystem:
      - /usr/share/childsplay/plugins
      - /usr/share/childsplay/Data/icons
  This is because yum removed childsplay before childsplay_plugins, and since
  the directories weren't empty when childsplay was removed, and they weren't
  owned by childsplay_plugins, they got left behind.

SHOULD
======
* Even though upstream uses an underscore in the name, I think it's better
  to use a dash '-' here.

* Request that upstream include the GPL license file in the tarball as they
  already do for the base childsplay package.
Comment 5 Paul Howarth 2006-05-11 01:54:02 EDT
(In reply to comment #4)
> Since rawhide is borked right now I haven't been able to test mock builds, which
> is also how I verify the BR: on packages.  I'll get to that as soon as rawhide
> is working again.

You know you can use mock with an FC5 root rather than rawhide to do this, right?
Comment 6 Hans de Goede 2006-05-11 09:40:32 EDT
Created attachment 128885 [details]
improved specfile

(In reply to comment #4)
> MUSTFIX
> =======
> 
> * Source0: url points to the Sourceforge mirror selection page.  Better
>   to use dl.sourceforge.net or hardcode a mirror so that tools like
>   spectool can be used to download the source files. (the same is true for
> childsplay, which I failed to notice earlier)
> 

Fixed

> * 'yum remove childsplay childsplay_plugins' left two dangling directories
>   on the filesystem:
>	- /usr/share/childsplay/plugins
>	- /usr/share/childsplay/Data/icons
>   This is because yum removed childsplay before childsplay_plugins, and since

>   the directories weren't empty when childsplay was removed, and they weren't

>   owned by childsplay_plugins, they got left behind.
> 

I've added:
Requires(postun): /usr/share/childsplay/plugins
Requires(postun): /usr/share/childsplay/Data/icons

Which should enforce proper uninstall order.

> SHOULD
> ======
> * Even though upstream uses an underscore in the name, I think it's better
>   to use a dash '-' here.
> 

Won't fix, this means that %{name} can't be used in the Source URL, and that I
need to pass -n to %setup, etc. Now if upstreams name was really ugly I would
find that worth the trouble but for this I would rather be consistent with what
upstream uses.

> * Request that upstream include the GPL license file in the tarball as they
>   already do for the base childsplay package.
> 

Will do.
Comment 7 Hans de Goede 2006-05-11 10:21:42 EDT
Created attachment 128892 [details]
improved specfile

(In reply to comment #4)
> MUSTFIX
> =======
> 
> * Source0: url points to the Sourceforge mirror selection page.  Better
>   to use dl.sourceforge.net or hardcode a mirror so that tools like
>   spectool can be used to download the source files. (the same is true for
> childsplay, which I failed to notice earlier)
> 

Fixed

> * 'yum remove childsplay childsplay_plugins' left two dangling directories
>   on the filesystem:
>	- /usr/share/childsplay/plugins
>	- /usr/share/childsplay/Data/icons
>   This is because yum removed childsplay before childsplay_plugins, and since

>   the directories weren't empty when childsplay was removed, and they weren't

>   owned by childsplay_plugins, they got left behind.
> 

I've added:
Requires(postun): /usr/share/childsplay/plugins
Requires(postun): /usr/share/childsplay/Data/icons

Which should enforce proper uninstall order. Unfortunatly this doesn't seem to
work any bright ideas?


> SHOULD
> ======
> * Even though upstream uses an underscore in the name, I think it's better
>   to use a dash '-' here.
> 

Won't fix, this means that %{name} can't be used in the Source URL, and that I
need to pass -n to %setup, etc. Now if upstreams name was really ugly I would
find that worth the trouble but for this I would rather be consistent with what
upstream uses.

> * Request that upstream include the GPL license file in the tarball as they
>   already do for the base childsplay package.
> 

Will do.
Comment 8 Ville Skyttä 2006-05-11 11:40:40 EDT
Use of Requires(foo) to try to force package installation/erase order (as
opposed to their real purpose, dependencies of the corresponding %foo
_scriptlet_) is abuse.  The real problem is bug 89500 which is reportedly fixed
in rpm CVS; hopefully a fixed version will trickle down to FC soon.

If you want the left-behind dirs problem worked around in the meantime, owning
those dirs in all affected packages is one solution.  Ignoring it is another;
there are loads of packages in FC/FE that are affected and will be automatically
fixed without any per-package kludges when the fixed rpm hits the distro repos.
Comment 9 Hans de Goede 2006-05-11 12:23:56 EDT
Ville,

Thanks for the comment, but bug 89500 is about PreReq's which this package
doesn't use. childsplay Requires: childsplay_plugins and childsplay_plugins
Requires: childsplay. Assuming rpm is indeed fixed todo proper erase ordening
how do a specify that I want childsplay_plugins to be removed first?

Mike,

I tend to not fixing this atleast not in a way working around soon to be fixed
rpm bugs. Can you live with that?
Comment 10 Ville Skyttä 2006-05-11 12:40:56 EDT
(In reply to comment #9)

> childsplay Requires: childsplay_plugins and childsplay_plugins
> Requires: childsplay. Assuming rpm is indeed fixed todo proper erase ordening
> how do a specify that I want childsplay_plugins to be removed first?

Ah, so you have a dependency loop, I didn't know that.  It's possible that bug
89500 isn't probably going to change anything then.  Preferred fix: get rid of
the loop and use plain Requires.  Other ideas that have worked at least sometime:
http://rpm.org/max-rpm-snapshot/s1-rpm-depend-manual-dependencies.html#S3-RPM-DEPEND-FINE-GRAINED
(...and owning the dirs in both packages is still an option...)
Comment 11 Hans de Goede 2006-05-11 13:06:09 EDT
Created attachment 128902 [details]
improved specfile

> Ah, so you have a dependency loop, I didn't know that.  It's possible that
bug
> 89500 isn't probably going to change anything then.  Preferred fix: get rid
of
> the loop and use plain Requires.  Other ideas that have worked at least
sometime:
>
http://rpm.org/max-rpm-snapshot/s1-rpm-depend-manual-dependencies.html#S3-RPM-DEPEND-FINE-GRAINED

> (...and owning the dirs in both packages is still an option...)

Quoting from the above URL:
"A plain Requires is enough to ensure proper installation order if there are no
dependency loops present in the transaction. If dependency loops are present
and cannot be avoided, packagers should strive to construct them in a way that
the order of installation of the the this way interdependent packages does not
matter."

So owning dirs in both packages indeed seems the best idea, new -3 release
doing just that attached.
Comment 12 Ville Skyttä 2006-05-11 13:35:29 EDT
(Forgot the disclaimer: the "fine grained deps" max-rpm chapter was written by
yours truly.)
Comment 13 Wart 2006-05-11 14:00:38 EDT
The multiple ownership of the dirs is fine with me.  Removal of the packages no
longer leaves dangling directories.

All other MUST items fixed, and SHOULD items adequately addressed/explained.

APPROVED
Comment 14 Hans de Goede 2006-05-11 15:49:21 EDT
Imported & Build.

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