Bug 190878

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.

Erm.

My bad:
http://home.zonnet.nl/jwrdegoede/childsplay_plugins-0.80.7-alphabet-sounds.patch

Created attachment 128837 [details]
improved specfile

Similar improvements as done to childplay, ghost .pyo files, etc.

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.

(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?

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.

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.

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.

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?

(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...)

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.

(Forgot the disclaimer: the "fine grained deps" max-rpm chapter was written by
yours truly.)

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

Imported & Build.