This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 248647 - Review Request: kdegames3 - KDE 3 games not ported to KDE 4
Review Request: kdegames3 - KDE 3 games not ported to KDE 4
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 248648
  Show dependency treegraph
 
Reported: 2007-07-17 17:10 EDT by Kevin Kofler
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-11-29 22:23:52 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
hdegoede: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Kevin Kofler 2007-07-17 17:10:49 EDT
Spec URL: http://repo.calcforge.org/f8/kdegames3.spec
SRPM URL: http://repo.calcforge.org/f8/kdegames3-3.5.7-1.fc7.src.rpm
Description:
Games and gaming libraries for the K Desktop Environment which have not been
ported to KDE 4 yet.
Included with this package are: atlantik, kasteroids, kenolaba, kfouleggs,
klickety, kpoker, ksmiletris, ksnake, ksokoban, ktron.

NOTE: This package is only targeted for F8 and above.
Comment 1 Hans de Goede 2007-07-27 04:43:55 EDT
Reviewing as promised
Comment 2 Hans de Goede 2007-07-27 05:26:26 EDT
Here we go, starting directly wiht a full review.

MUST
----

0 rpmlint output is:

W: kdegames3 macro-in-%changelog _bindir
This mist be fixed, always use %% in the changelog

W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/ktron/common ../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/kpoker/common
../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/ksmiletris/common
../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/ksokoban/common
../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/kenolaba/common
../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/kfouleggs/common
../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/atlantik/common
../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/kasteroids/common
../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/ksnake/common
../common
W: kdegames3 dangling-relative-symlink /usr/share/doc/HTML/en/klickety/common
../common

These are normal for KDE apps.

E: kdegames3 file-in-usr-marked-as-conffile /usr/share/config.kcfg/kasteroids.kcfg
W: kdegames3 conffile-without-noreplace-flag /usr/share/config.kcfg/kasteroids.kcfg
E: kdegames3 file-in-usr-marked-as-conffile /usr/share/config.kcfg/kfouleggs.kcfg
W: kdegames3 conffile-without-noreplace-flag /usr/share/config.kcfg/kfouleggs.kcfg
E: kdegames3 file-in-usr-marked-as-conffile /usr/share/config.kcfg/ksnake.kcfg
W: kdegames3 conffile-without-noreplace-flag /usr/share/config.kcfg/ksnake.kcfg
E: kdegames3 file-in-usr-marked-as-conffile /usr/share/config.kcfg/ktron.kcfg
W: kdegames3 conffile-without-noreplace-flag /usr/share/config.kcfg/ktron.kcfg

Either these are config files and should be moved to /etc/kde and be made
%config(noreplace) or they are merely defaults / template files and should not
be %config at all. kdebase puts files under /usr/share/config.kcfg and doesn't
mark them %config.

E: kdegames3 invalid-desktopfile /usr/share/applications/kde/kpoker.desktop
This is caused by the faulty / obsolete "Miniicon=kpoker" line, remove this and
all is fine.

* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License ok
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel x86_64
* BR: ok
* locales handled properly
* Shared libraries, ldconfig run as required
* Not relocatable
0 Package owns or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* -devel package as needed
* .desktop file as required

MUST _really_ FIX
-----------------
* this one is so important I just invented a new catagory for it :)
  Currently the specfile contains:
%if 0%{?rhel} > 1
%define patch_name -patched
%else
%define patch_name %{nil}   
%endif

  And:
Source:
ftp://ftp.kde.org/pub/kde/stable/%{version}/src/kdegames-%{version}%{patch_name}.tar.bz2

  However there is no kdegames-3.5.7-patch.tar.bz2 on the kde site, so RHEL uses
  a different tarbal, which doesn't come from upstream, which leads to the 
  following:
  1) Why?
  2) Please do this with patches and or binary (small tarbal) overlays, since
     its fine for Fedora, there isn't a legal problem so no reason to use a 
     modified tarbal
  3) If you must use a modified tarbal, then
     a) Add a comment above the Source tag explaining in detail what was changed
     b) make the Source tag just the filename instead of a non existing URL


MUST fix
--------

* Various rpmlint errors, see above
* Add Requires: hicolor-icon-theme for /usr/share/icons/hicolor/*/* dir
  ownership
* unowned directory /usr/share/config.kcfg (own, or maybe better file a bug 
  against kdelibs3 that it should own it).


Should fix
----------
* This would seem like a good moment to prune the changelog a bit
* Are the debug final and cvs defines + conditional code still needed?
Comment 3 Kevin Kofler 2007-07-27 05:33:20 EDT
AFAIK, the strange "-patched" tarball is because RH lawyers historically had a 
problem with KSokoban, probably because it used to contain the levels ported 
from the original game (arguably a copyright violation). This is no longer the 
case (it only comes with its own original (in the other sense of this confusing 
word) levels now), so IMHO this point is moot. The only remaining possible 
issue is a trademark issue, which if it is really an issue, can be solved by 
rebranding like you did with Gnometris.

So I'm going to zap the strange RHEL conditionals.
Comment 4 Kevin Kofler 2007-07-27 05:44:13 EDT
> * unowned directory /usr/share/config.kcfg (own, or maybe better file a bug 
>   against kdelibs3 that it should own it).

This is owned by kde-filesystem now. I'm going to add a Requires on it.
Comment 5 Kevin Kofler 2007-07-27 05:48:20 EDT
> * Are the debug final and cvs defines + conditional code still needed?

Yes, KDE 3 needs these. Disabling final speeds up compilation and can work 
around compiler bugs where the compiler just crashes on the big source file 
from --enable-final on some arches (one of the 3.5.7 packages triggered that on 
ppc64).
Comment 6 Kevin Kofler 2007-07-27 06:13:41 EDT
Updated (should fix all the MUST issues you found):

Spec URL: http://repo.calcforge.org/f8/kdegames3.spec
SRPM URL: http://repo.calcforge.org/f8/kdegames3-3.5.7-2.fc7.src.rpm

- drop RHEL conditional using patched tarball
- fix macro in changelog
- don't mark %%{_datadir}/config.kcfg/* as %%config
- add Requires on hicolor-icon-theme and kde-filesystem for dir ownership
- patch out obsolete Miniicon= line from kpoker.desktop

(Note: Don't get confused by the SRPM disttag, this is still targeted at F8+ 
only, possibly even F9+ only if we decide not to ship kdegames 4 in F8 yet.)
Comment 7 Hans de Goede 2007-07-27 06:20:09 EDT
I haven't looked at your new version yet, but I just hit a snag with your old
version, I tried to actual install it and I got:
        file /usr/bin/ksokoban from install of kdegames3-3.5.7-1.fc8 conflicts
with file from package kdegames-3.5.7-1.fc8

And many more of these. I think this needs a "Conflicts: kdegames < 4" adding a
"Obsoletes: kdegames < 4" would solve my problem, but might result in people
updating only getting kdegames3-3.5.7 and not kdegames-4.0.0, depending on the
order things get done. So a conflict is probably the best solution.

Comment 8 Kevin Kofler 2007-07-27 06:23:33 EDT
That should be:
Conflicts: kdegames < 6:3.80
actually. The kdegames package has Epoch 6 and the first KDE 4 alpha was 
numbered 3.80.1.
I'm adding it in that form.
Comment 9 Kevin Kofler 2007-07-27 06:32:03 EDT
Updated (adds the Conflicts):

Spec URL: http://repo.calcforge.org/f8/kdegames3.spec
SRPM URL: http://repo.calcforge.org/f8/kdegames3-3.5.7-3.fc7.src.rpm

- Conflicts with pre-KDE-4 kdegames
Comment 10 Hans de Goede 2007-07-27 06:33:22 EDT
Approved!
Comment 11 Kevin Kofler 2007-10-24 22:48:46 EDT
New Package CVS Request
=======================
Package Name: kdegames3
Short Description: KDE 3 games not ported to KDE 4
Owners: kkofler,rdieter,than
Branches: F-8 F-7
InitialCC: 
Cvsextras Commits: no
Comment 12 Kevin Kofler 2007-10-26 12:28:41 EDT
Ping? Still waiting on CVS directory creation.

Meanwhile, I updated my SRPM (it has been a while since the successful review):
Spec URL: http://repo.calcforge.org/f9/kdegames3.spec
SRPM URL: http://repo.calcforge.org/f9/kdegames3-3.5.8-1.fc7.src.rpm

* Fri Oct 26 2007 Kevin Kofler <...> - 3.5.8-1
- update to 3.5.8
- fix/update trademarks patch (Rex Dieter)
- License: GPLv2, libs/devel License: LGPLv2
- libs subpkg (more multilib friendly)
- include ksirtet (has been omitted from KDE 4.0)
- drop kpoker-desktop patch (fixed upstream)

* Wed Aug 8 2007 Kevin Kofler <...> - 3.5.7-4
- patch out the trademarked names of a certain box pushing game and a certain
  snake-like duel game

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