Bug 291371 - Review Request: teg - Turn based strategy game
Review Request: teg - Turn based strategy 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 Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-14 14:06 EDT by josef radinger
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-30 11:13:50 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description josef radinger 2007-09-14 14:06:22 EDT
Spec URL: http://www.nosuchhost.net/~cheese/fedora/packages/teg.spec
SRPM URL: http://www.nosuchhost.net/~cheese/fedora/packages/teg-0.11.2-4.fc7.src.rpm
Description: Tenes Emapandas Graciela (TEG) is a clone of 'Plan Táctico y Estratégico
de la Guerra', which is a pseudo-clone of Risk, a multiplayer turn-based
strategy game. Some rules are different.


I need a Sponsor.
Should I convert the original AUTHORS and README-files to UTF8 or leave them as they were upstream???
Comment 1 Mamoru TASAKA 2007-10-10 09:39:35 EDT
Well, general packaging guidelines are written on

http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

If you have some questions about packaging issue, please
check these URLs first.

Some random notes for 0.11.2-4:
* SourceURL
  - Source tarball must be written in full URL.
    http://fedoraproject.org/wiki/Packaging/SourceURL

* perl module BuildRequires
  - perl module BuildRequires must be written not by
    their rpm names but theire perl module names.
    http://fedoraproject.org/wiki/Packaging/Perl

    In short, "BuildRequires: perl-XML-Parser" must be
    "BuildRequires: perl(XML::Parser)".

* Macros
  - Use macros when possible. /etc must be %_sysconfdir.

* %makeinstall
  - Please don't use %makeinstall when possible
    (check the section 
     "Why the %makeinstall macro should not be used" of
      http://fedoraproject.org/wiki/Packaging/Guidelines)

* desktop file
  - Desktop file must be installed by desktop-file-install
    (check the section "Desktop files" of the URL above)

  - Also, please consider below.
------------------------------------------------------------
./teg.desktop: warning: boolean key "Terminal" in group "Desktop Entry" has
value "0", which is deprecated: boolean values should be "false" or "true"
------------------------------------------------------------

* gconf schemas file
  - Would you explain why you want to remove gconf
    schemas file installed under %_sysconfdir/gconf/schemas ?

    Also you may want to see general guideline to handle
    schemas file, in the section "GConf" of
    http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

* update-desktop-database
  (check the section "desktop-database" of scriptlet wiki page)
  - update-desktop-database must be called when the desktop
    file has a MimeType key, but teg.desktop does not have.

* Directory ownership issue
  - The directories created by this package must be owned
    by this directory.

    ! Example (note: This is a example. There are some directories 
                     which are not correctly owned by this package.)
------------------------------------------------------------
[tasaka1@localhost ~]$ rpm -qf /usr/share/pixmaps/teg_pix/color_player_black.png 
teg-0.11.2-4.fc8
[tasaka1@localhost ~]$ rpm -qf /usr/share/pixmaps/teg_pix
file /usr/share/pixmaps/teg_pix is not owned by any package
------------------------------------------------------------
      Here this package installs some file under
      %_datadir/pixmaps/teg_pix and those files are owned by this
      package. But the directory %_datadir/pixmaps/teg itself is
      not owned by this package.

    ! Note:
      When you write
------------------------------------------------------------
%files
foo/
------------------------------------------------------------
      (where foo/ is a directory) in %files entry, this means
      the directory foo/ itself and all files/directories/etc
      under foo/, while
------------------------------------------------------------
%files
%dir foo/
------------------------------------------------------------
      means the directory foo/ only.

* rpmlint issue
------------------------------------------------------------
$ rpmlint teg
teg.i386: W: file-not-utf8 /usr/share/doc/teg-0.11.2/README
teg.i386: W: file-not-utf8 /usr/share/doc/teg-0.11.2/AUTHORS
teg.i386: W: summary-not-capitalized teg is a clone of a clone of Risk
------------------------------------------------------------
  - README, AUTHORS should be converted into UTF-8.
  - Summary must begin with capital letter.
    ! Note: "teg is" in summary is redundant.

* Documents
  - It seems that there are some other files which I think should
    also be included as %doc, such as
------------------------------------------------------------
ChangLog PEOPLE
------------------------------------------------------------
    Maybe "TODO" can also be included as %doc.
Comment 2 Jason Tibbitts 2007-10-10 11:10:02 EDT
Also, please note that the word "Risk" is trademarked in this context, and as
such you should not mention it in the summary or description of your package,
nor in things like the desktop file.
Comment 3 josef radinger 2007-10-14 12:21:30 EDT
thanks for your input. created new files, but i'm not yet satisfied, and will
them release later  for inspection. 
concerning schema-files: i could not figure what they where for, and teg seems
to work correct without them. 
Comment 4 Mamoru TASAKA 2007-10-25 13:02:32 EDT
GConf schemas files are used to customize application.
Perhaps you can get teg work without gconf setting (i.e.
by default), however with gconf file you can customize teg (perhaps).
Comment 5 Mamoru TASAKA 2007-11-07 03:00:25 EST
ping?
Comment 6 josef radinger 2007-11-08 02:51:12 EST
pong.
will release new files next week
Comment 7 Hans de Goede 2007-11-17 12:05:03 EST
I see that you are looking for someone to sponsor you, and as it happens I'm
allowed to sponsor people. Here is what I would like to do: I see that you've
submitted 3 packages for review, I will review all 3 of them and then after one
or two iterations the can hopefully be approved, assuming that process go well
I'll sponsor you when all 3 are approved, if you agree with this "procedure",
please let me know and I'll start reviewing all 3.


Comment 8 josef radinger 2007-11-17 17:29:21 EST
i would appreciate your sponsorship.
just finished a new packagebuild from teg.
can be downloaded at:
http://www.nosuchhost.net/~cheese/fedora/packages/8/teg.spec
http://www.nosuchhost.net/~cheese/fedora/packages/8/teg-0.11.2-6.fc8.src.rpm
http://www.nosuchhost.net/~cheese/fedora/packages/8/teg-0.11.2-6.fc8.i386.rpm

thanks go to Mamoru Tasaka for his long comment and to Jason Tibbitts for his
note about the mentioning of Risk .
Comment 9 Mamoru TASAKA 2007-11-20 06:56:04 EST
Well, some comments for 0.11.2-6:

* Again sourceURL
  - For sourceforge tarball, please again refer to
    http://fedoraproject.org/wiki/Packaging/SourceURL

* Timestamp
  - Please try to add 'INSTALL="install -p"' option to
    'make install' to keep timestamps on installed files.
    Usually this method works for recent Makefiles.

* Duplicate file entry
  - Well, as I said in my comment 1, the %files entry
------------------------------------------------------
%files
%{_datadir}/pixmaps/teg_pix/
------------------------------------------------------
    contains the directory %{_datadir}/pixmaps/teg_pix itself
    and all files/directories/etc under the directory.
    So the additional %files entry
------------------------------------------------------
%{_datadir}/pixmaps/teg_pix/*
------------------------------------------------------
    is not needed, actually this causes the warnings in build.log
    like:
------------------------------------------------------
  1287  warning: File listed twice:
/usr/share/pixmaps/teg_pix/color_player_black.png
  1288  warning: File listed twice: /usr/share/pixmaps/teg_pix/color_player_blue.png
  1289  warning: File listed twice:
/usr/share/pixmaps/teg_pix/color_player_green.png
  1290  warning: File listed twice: /usr/share/pixmaps/teg_pix/color_player_pink.png
------------------------------------------------------

* GConf schemas file
  - We don't mark gconf schemas file as %config file (even if rpmlint
    warns about it).
Comment 11 Hans de Goede 2007-11-21 14:02:41 EST
Okay, I've done a full review, looks good, many thanks to Mamoru for getting it
into ints current shape.

There are only 2 things which I would prefer to be changed (but this is not a
blocker):

1) please move /usr/share/pixmaps/teg_icono.png to 
   /usr/share/icons/hicolor/48x48/apps/teg.png

   And then change the Icon line in the .desktop to just "Icon=teg"
   And add the necessary scripts to update the icon cache (see the 
   scripletsnippets page in the wiki)

2) You really should change the encoding of the docs in %prep, not at the end
   of %install


I'll take a look at libopensync-plugin-foo and -bar next, once those 2 are
approved too I'll sponsor you.
Comment 13 josef radinger 2007-11-27 13:43:18 EST
New Package CVS Request
=======================
Package Name: teg
Short Description: Turn based strategy game
Owners: cheese
Branches: F-8 F-7
InitialCC: cheese
Cvsextras Commits: yes
Comment 14 Kevin Fenzi 2007-11-27 14:43:32 EST
cvs done.

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