Bug 235815 - Review Request: freetennis - Tennis simulation game
Review Request: freetennis - Tennis simulation game
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Package Reviews List
:
Depends On: 235804 235805
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-10 07:12 EDT by Nigel Jones
Modified: 2007-11-30 17:12 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-08 23:45:48 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
jwboyer: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nigel Jones 2007-04-10 07:12:39 EDT
Spec URL: http://dev.nigelj.com/SRPMS/freetennis.spec
SRPM URL: http://dev.nigelj.com/SRPMS/freetennis-0.4.8-1.src.rpm
Description: 
Free Tennis is a free software tennis simulation game.  The game can be
played against an A.I. or human-vs-human via LAN or internet.

rpmlint is clean on all rpms (incl src).

Potential problems that I'm aware of that may/may not need fixing:
1. As noted in both blockers to this, the game has a buildrequires on -devel packages, but after build does not require library files, I am not sure if I have done something in error, (i.e. the libraries are been statically built), or this is how ocaml treat libraries etc, I've noted that the Debian freetennis package does not require ocamlSDL or camlimages
2. I'm a bit worried if I went a bit OTT on the directory ownership.
3. README and LICENSE are duplicated in both the main package and -data, this is because -data may be used in another package, and it may be possible to provide a seperate datafile, using the same directory format.
4. I think the summaries need a touch up before importing into Fedora, but I have time because this can be even uploaded (due to the blocking Reviews).
Comment 1 Hans de Goede 2007-05-02 01:32:36 EDT
Nice job, I was hoping someone would tackle this caml stuff, and here you are.

I'll try todo some reviews on this, starting with the caml deps. If someone
wants to beat me to it, thats fine. As long as I haven't assigned the bug to me,
I haven't actually started reviewing.

Comment 2 Hans de Goede 2007-05-02 14:56:49 EDT
Initial list with some things to fix after a quick initial look.

- The main "%files" is missing a %defattr, this should be added as the first
line after %files

- %files-data can be written mich shorter, it can (and should be) as simple as:

%files data
%defattr(-,root,root,-)
%doc COPYING CHANGES.txt AUTHORS
%{_datadir}/%{name}

  Also notice that %defattr is the first line after %files data, putting 
  %defattr at the end has no effects as %defattr is only applied to files listed
  below it, but read on...

- The data subpackage should require the main package, so that it gets removed 
  with it, but read on...

- The data package doesn't need to double some of the docs from the main package
  as it requires it, but read on...

- However since the data is in the same upstream tarbal as the sources, and
  this cannot be in a seperate srpm there is no use (what so ever) in having
  it in a subpackage, so just completely remove the data sub-package,
  and add the single line "%{_datadir}/%{name}" to the main %files list

- Please add an icon, just take any suitable graphics included and resize / edit
  it as nescesarry then install it under /usr/share/icons/hicolor/<size>/apps
  and add the necessary scripts to update the icon-cache. (See scriptlets wiki
  page)


p.s. Do you still need a sponsor? Once this and the deps have been reviewed I
can sponsor you if needed.

Comment 3 Nigel Jones 2007-05-02 16:12:00 EDT
(In reply to comment #2)
> Initial list with some things to fix after a quick initial look.
> 
> - The main "%files" is missing a %defattr, this should be added as the first
> line after %files
> 
> - %files-data can be written mich shorter, it can (and should be) as simple as:
> 
> %files data
> %defattr(-,root,root,-)
> %doc COPYING CHANGES.txt AUTHORS
> %{_datadir}/%{name}
> 
>   Also notice that %defattr is the first line after %files data, putting 
>   %defattr at the end has no effects as %defattr is only applied to files listed
>   below it, but read on...
Okay shall do...
> - However since the data is in the same upstream tarbal as the sources, and
>   this cannot be in a seperate srpm there is no use (what so ever) in having
>   it in a subpackage, so just completely remove the data sub-package,
>   and add the single line "%{_datadir}/%{name}" to the main %files list
> 
Right, I'll do that (I'm going to wait until I can convince camlimages to build
in mock though)
> - Please add an icon, just take any suitable graphics included and resize / edit
>   it as nescesarry then install it under /usr/share/icons/hicolor/<size>/apps
>   and add the necessary scripts to update the icon-cache. (See scriptlets wiki
>   page)
Okay
> 
> 
> p.s. Do you still need a sponsor? Once this and the deps have been reviewed I
> can sponsor you if needed.
> 
Nope, I'm in cvsextras already (was sponsored by Kevin with package windowlab)
and removed the FE-NEEDSPONSOR stuff already (anyway, how could I have approved
your other package? ;))
> 

Comment 4 Hans de Goede 2007-05-02 16:17:35 EDT
(In reply to comment #3)
> Right, I'll do that (I'm going to wait until I can convince camlimages to build
> in mock though)

If you need help with this (I can always try) let me know.

> > p.s. Do you still need a sponsor? Once this and the deps have been reviewed I
> > can sponsor you if needed.
> > 
> Nope, I'm in cvsextras already (was sponsored by Kevin with package windowlab)
> and removed the FE-NEEDSPONSOR stuff already (anyway, how could I have approved
> your other package? ;))
> > 
> 

True, back in the old days when we didn't use flags, anyone with bugzilla edit
rights could approve a package. Also you're not in owners.list (I guess that
windowlab isn't approved / imported into cvs yet?) or maybe I just haven't done
a cvs update in my owners dir for a while....

 

Comment 5 Nigel Jones 2007-05-02 16:21:54 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > Right, I'll do that (I'm going to wait until I can convince camlimages to build
> > in mock though)
> 
> If you need help with this (I can always try) let me know.
I think I've just got it, I'll leave it for now, and test in mock tonight (I
need to leave for town in a few mins so yeah)
> 
> > > p.s. Do you still need a sponsor? Once this and the deps have been reviewed I
> > > can sponsor you if needed.
> > > 
> > Nope, I'm in cvsextras already (was sponsored by Kevin with package windowlab)
> > and removed the FE-NEEDSPONSOR stuff already (anyway, how could I have approved
> > your other package? ;))
> > > 
> > 
> 
> True, back in the old days when we didn't use flags, anyone with bugzilla edit
> rights could approve a package. Also you're not in owners.list (I guess that
> windowlab isn't approved / imported into cvs yet?) or maybe I just haven't done
> a cvs update in my owners dir for a while....
Quite possible, but the merge is happening now
https://www.redhat.com/archives/fedora-extras-commits/2007-April/msg02380.html
> 
>  
> 
> 

Comment 6 Nigel Jones 2007-05-02 16:23:01 EDT
Oh, I meant to say as well, that I'll have updated packages in about 18 hours
time (if camlimages keeps a sane head).
Comment 7 Nigel Jones 2007-05-03 06:43:49 EDT
Spec URL: http://dev.nigelj.com/SRPMS/freetennis.spec
SRPM URL: http://dev.nigelj.com/SRPMS/freetennis-0.4.8-2.src.rpm

(Uploading now)

Still rpmlint clean, changelog:

* Thu May 03 2007 Nigel Jones <dev@nigelj.com> 0.4.8-2
- Add freetennis.png and alter freetennis.desktop to show icon
- Change builddep to ocaml-SDL-devel
Comment 8 Nigel Jones 2007-05-04 02:04:56 EDT
Spec URL: http://dev.nigelj.com/SRPMS/freetennis.spec
SRPM URL: http://dev.nigelj.com/SRPMS/freetennis-0.4.8-3.src.rpm

Only change is camlimages->ocaml-camlimages
Comment 9 Hans de Goede 2007-05-04 04:03:34 EDT
MUST:
=====
* rpmlint output is clean
* 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 i386
* BR: ok
* No locales
* No shared libraries, ldconfig not needed
* Not relocatable
* 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
* no .desktop file needed

MUST FIX
========
* Change Source0 to the generic sf download url, and use %{version}
* Change "Categories=Game" to "Categories=Game;SportsGame;" in the .desktop
  file, so that it will show up under the Games->"Sport Games" submenu for
  people who have the games-menus package installed (try it).
Comment 10 Nigel Jones 2007-05-04 04:44:35 EDT
Spec URL: http://dev.nigelj.com/SRPMS/freetennis.spec
SRPM URL: http://dev.nigelj.com/SRPMS/freetennis-0.4.8-1.src.rpm

Update to include MUST FIX items
Comment 11 Hans de Goede 2007-05-04 04:57:16 EDT
All MUST FIX items handled, approved!
Comment 12 Nigel Jones 2007-05-04 05:14:00 EDT
(In reply to comment #11)
> All MUST FIX items handled, approved!
> 

Thanks,

New Package CVS Request
=======================
Package Name:      freetennis
Short Description: Tennis simulation game
Owners:            dev@nigelj.com
Branches:          FC-6 devel
InitialCC:         <empty>
Comment 13 Nigel Jones 2007-05-08 23:45:48 EDT
Excluding arch ppc64 so it will build in F7 (already built on FC6), for those
interested, the new bug is Bug #239521

Closing...


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