Bug 235815 - Review Request: freetennis - Tennis simulation game
Summary: Review Request: freetennis - Tennis simulation game
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 235804 235805
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-04-10 11:12 UTC by Nigel Jones
Modified: 2007-11-30 22:12 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-05-09 03:45:48 UTC
Type: ---
Embargoed:
hdegoede: fedora-review+
jwboyer: fedora-cvs+


Attachments (Terms of Use)

Description Nigel Jones 2007-04-10 11:12:39 UTC
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 05:32:36 UTC
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 18:56:49 UTC
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 20:12:00 UTC
(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 20:17:35 UTC
(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 20:21:54 UTC
(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 20:23:01 UTC
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 10:43:49 UTC
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> 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 06:04:56 UTC
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 08:03:34 UTC
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 08:44:35 UTC
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 08:57:16 UTC
All MUST FIX items handled, approved!


Comment 12 Nigel Jones 2007-05-04 09:14:00 UTC
(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
Branches:          FC-6 devel
InitialCC:         <empty>

Comment 13 Nigel Jones 2007-05-09 03:45:48 UTC
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.