Bug 189713

Note: This is not a formal review.

1. The spec file MUST handle locales properly. This is done by using the
%find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
2. Normally, the Source should be a full url, but I'm assuming your using CVS. 
It should probably marked as such.
3. Your missing quite a few BuildRequires.  You should use Mock to verify that
your package will build correctly.  http://fedoraproject.org/wiki/Extras/MockTricks
4. Sub-packages Requires is incorrect.  It should be %{name} = %{version}-%{release}
5. The sub-packages needs %defattr(-,root,root,-)
6. Scriptlet for TexInfo is incorrect.
http://fedoraproject.org/wiki/ScriptletSnippets#head-117e9450bc166ceb4251bf8d87a9dd4e862442a4

That's probably more than enough stuff for you to work on for now.  I would
suggest fully reading the wiki, since almost all of these issues are addressed
there. http://fedoraproject.org/wiki/Extras

Joost,

I noticed you put this game on the Games SIG review list (good!). But thats not
entirely how it works, we (the Games SIG) have got this sortof unwritten rule of
a review for a review. So shall we exchange reviews?

I currently need a reviewer for:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=192060

Also, before I do a review I would like you to post a new version fixing the
issues mentioned in comment 1

* Wed May 31 2006 - Joost Soeterbroek <fedora>
- added find_lang macro
- added full URL to Source
- added BuildReqs.
- added correct Reqs. to sub-packages
- added defattr to sub-packages' files section
- added correct TexInfo scriptlet for post and preun

Spec URL: http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg.spec
SRPM URL:
http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg-20060530-1.src.rpm

rpmlint on  gnubg-20060530-1.src.rpm gives
E: gnubg configure-without-libdir-spec

No formal review, just the results of a quick scan, which has turned up enough
for you to work on for now:

Why call autogen.sh? thats concidered bad practice unless absolutly nescesarry,
and if you must you should do so from %setup not %install

Also BR: automake implies autoconf, so the BR: autoconf is extranous and should
be removed. (Notice if you remove the autogen.sh call that you then should also
remove the autoxxx and libtool BR's).

Also BR:L glib2 is plain wrong this is a gtk1 app right, so it should be
glib-devel, which is implied by BR: gtk+-devel, please remove the BR glib2.

Don't call ./configure yourself instead use %configure (that will fix the
rpmlint error) you can call it with your own args added like this:

%configure --with-python \
	--without-gdbm \
	--without-guile \
	--without-timecontrol \
	--without-board3d

"make prefix=$RPM_BUILD_ROOT%{_prefix} install-strip"
is dead wrong, use:
make install DESTDIR=$RPM_BUILD_ROOT
Stripping is wrong, rpmbuild will do this for you, and if you DIY, the
-debuginfo package will be useless.

Unowned directories:
%{_datadir}/gnubg
%{_datadir}/gnubg/met
%{_datadir}/gnubg/doc
%{_datadir}/gnubg/scripts
%{_datadir}/gnubg/sounds

(In reply to comment #5)

> Why call autogen.sh? thats concidered bad practice unless absolutly 

I am calling autogen.sh because the source is nightly tarball from CVS not
containing a configure file. 
The latest available source I found is contained in a SRPM more than a year old
(http://www.acepoint.de/GnuBG/redhat/gnubg-0.15-1.src.rpm). The nightly CVS
tarballs seem to be the only recent published sources available.

(In reply to comment #6)
> (In reply to comment #5)
> 
> > Why call autogen.sh? thats concidered bad practice unless absolutly 
> 
> I am calling autogen.sh because the source is nightly tarball from CVS not
> containing a configure file. 
> The latest available source I found is contained in a SRPM more than a year old
> (http://www.acepoint.de/GnuBG/redhat/gnubg-0.15-1.src.rpm). The nightly CVS
> tarballs seem to be the only recent published sources available.

Yes,

Thats a good reason, I should have spotted that. myself :) Anyways all the other
points still remain to be fixed.

* Thu Jun  1 2006 - Joost Soeterbroek <fedora> - 20060530-2
- moved autogen.sh from %build to %setup
- changed ./configure to %configure macro
- removed install-strip
- added directories to files sections
- removed BuildReqs glib2 and autoconf
- added BuildReqs (mock): gettext-devel, gtk2-devel, texinfo, python-devel,
  netpbm-progs, gnuplot, ghostscript, info

Spec URL: http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg.spec
SRPM URL:
http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg-20060530-2.src.rpm

MUST:
=====
* rpmlint output is:
   W: gnubg file-not-utf8 /usr/share/man/man6/gnubg.6.gz
   W: gnubg-databases no-documentation
   W: gnubg-sounds no-documentation
 You must fix the not utf8 error, you can do so by running iconv
 on the uncompressed manpage, see gcompris for an example. Notice that the
 original encoding may be different then in gcompris though (probably not).
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License (GPL) ok, license file included
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel-x86_64
* BR: see below
* No locales
* no shared libs
* 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
* no -devel needed
* gui .desktop required but none included! must fix!


MUST fix:
=========
* building on devel gives:
 error: Installed (but unpackaged) file(s) found:
   /usr/share/info/dir
 This file should be removed at the end of %install
 Be sure to use "rm -f" for the removal since in my experience this file
 doesn't get created on all Fedora versions.
* Remove bogus "BuildRequires: gtk+-devel" this is a gtk2 app, gtk+-devel
 is for gtk1 apps.
* gtk2-devel implies freetype-devel, remove Extranous freetype-devel BR.
* esound-devel implies audiofile-devel, remove Extranous audiofile-devel BR.
* Add a desktop file and icon, see njam(.spec) for an example on howto
  properly install the .desktop file and the icon and howto properly update the
  icon cache on install / remove.


Should fix:
===========
* I get this during the build:
   /bin/sh: gnubg.weights: No such file or directory
   make[2]: [gnubg.wd] Error 1 (ignored)
 Yet you've included a a gnubg.weights as Source1, appearantly its not put
 in the right place.
* And this:
   cp: cannot stat /usr/share/texinfo/texinfo.dtd': No such file or directory
 But this one can brobably be ignored I can't find texinfo.dtd anywhere on
 my fairly complete install. Notice that gnubg itself does contain and
 installs a texinfo.dtd
* You can write things like:
   %dir %{_datadir}/gnubg/met 
   %{_datadir}/gnubg/met/*    
 As just:
   %{_datadir}/gnubg/met
 And the same for:
   %dir %{_datadir}/gnubg/scripts
   %{_datadir}/gnubg/scripts/* 
 Which can be just:
   %{_datadir}/gnubg/scripts
 Etc.
 And probably the same for:
   %dir %{_datadir}/gnubg/doc 
   %{_datadir}/gnubg/doc/*.png
   %{_datadir}/gnubg/doc/texinfo.dtd
   %{_datadir}/gnubg/doc/gnubg.xml
 Which can probably be written as just:
   %{_datadir}/gnubg/doc

Explain:
========
* Why add --without-board3d, gtkglext(-devel) is available in FE.
* Why the seperate database and soundpackages, does the main package
  function without these?

And a MUST fix I missed initially:
* You must %ghost the .pyo files in 
  /usr/share/gnubg/scripts/
  You can do this by adding this line to %files:
  %ghost %{_datadir}/gnubg/scripts/*.pyo

Notice about the ghosting, this means that you cannot use
%{_datadir}/gnubg/scripts
or
%{_datadir}/gnubg/scripts/*

in %files, the other lines in %files must not in any way include the .pyo files.

* Sun Jun  5 2006 - Joost Soeterbroek <fedora> - 20060530-3
- fixed utf8 error in /usr/share/man/man6/gnubg.6.gz
- removed BuildReqs gtk+-devel, freetype-devel, audiofile-devel
- added BuildReqs gtkglext-devel
- remove /usr/share/info/dir
- removed subpackages database- and sound-
- removed configure option without-board3d
- sanitised %files section
- ghost the .pyo files /usr/share/gnubg/scripts/
- added desktop file and icon

Spec URL: http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg.spec
SRPM URL:
http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg-20060530-3.src.rpm

* Sun Jun  5 2006 - Joost Soeterbroek <fedora> - 20060530-4
- added BuildReqs desktop-file-utils

Spec URL: http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg.spec
SRPM URL:
http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg-20060530-4.src.rpm

Hi, just a few quick remarks I take a closer look soon:
* # convert man page from UTF8 to ISO-8859-1
 Actually its the other way around :)
* 
/usr/bin/iconv -f ISO-8859-1 -t UTF8 $RPM_BUILD_ROOT/usr/share/man/man6/gnubg.6
> /tmp/foo
/bin/mv /tmp/foo $RPM_BUILD_ROOT/usr/share/man/man6/gnubg.6

Don't use a file in the global temp dir for this and don't do this in %install
do it in %prep and then use gnubg.6.tmp as tempfile name, what happens when
using the global /tmp dir and another packages which gets build at the same time
does the same? Also by using a fixed name in /tmp you're creating a tempfile
security vulnerability (on the buildsys).

* I see you've put the png ion the 32x32 dir is it (circa) 32x32 pixels? if not
it should probably be in another dir (for example 48x48 or ... see the ones on
your system).

* Tue Jun  6 2006 - Joost Soeterbroek <fedora> - 20060530-5
- minor change in man file conversion, move from install to prep

Spec URL: http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg.spec
SRPM URL:
http://www.soeterbroek.com/linux/fedora/extras/gnubg/gnubg-20060530-5.src.rpm

(In reply to comment #14)

> * I see you've put the png ion the 32x32 dir is it (circa) 32x32 pixels? if not
> it should probably be in another dir (for example 48x48 or ... see the ones on
> your system).

gnubg.png is is 32x32 pixels exacltly.

Looks good now, approved!

 10588 (gnubg): Build on target fedora-development-extras succeeded.
     Build logs may be found at
http://buildsys.fedoraproject.org/logs/fedora-development-extras/10588-gnubg-20060530-5.fc6/