Notice that the review request for the required xblast-data is in bug 229477

I will review this later.
By the way I will appreciate it if you would review my
mecab related review request (bug 229927 and bug 229929).
Thanks!

Well,

A. First for xblast-2.10.4-1:

* File dependency
  - Writing the package which provides the file is recommended
    expect you have somewhat strong reason to write file dependency
    (for vera font). Please check:
    http://fedoraproject.org/wiki/PackagingDrafts/FileDeps
    (This is a draft)

* Source URL
  - Please use http://downloads.sourceforge.net/<package_name>/XXXX.tar.gz
    if it is possible. Please check:
    http://fedoraproject.org/wiki/PackagingDrafts/SourceUrl
    (This is a draft).
  - Please specify the URL of xblast.png if possible.

* Timestamps
----------------------------------------------------------
install -m 755 %{SOURCE3} $RPM_BUILD_ROOT%{_bindir}/%{name}
----------------------------------------------------------
  - This is only a wrapper script and keeping timestamp
   (i.e. install -p) is recommended.

* Documentation
  - Perhaps the following files can be used.
----------------------------------------------------------
./xblast.man
----------------------------------------------------------

* Functionality
  - xblast-x11 cannot be launched for me.
----------------------------------------------------------
[tasaka1@localhost xblast]$ xblast-x11 
could not load font 24
could not load font 18
could not load font 14
X Error of failed request:  BadFont (invalid Font parameter)
  Major opcode of failed request:  56 (X_ChangeGC)
  Resource id in failed request:  0x800010
  Serial number of failed request:  519
  Current serial number in output stream:  541
-----------------------------------------------------------

* Directory/file ownership
  - Well as the build log says:
-----------------------------------------------------------
-DGAME_DATADIR=\"/usr/share/xblast\"
-----------------------------------------------------------
    I think that %{_datadir}/xblast should be owned by
    xblast-common, not by xblast-data because xblast requires
    that the files are installed under %{_datadir}/xblast.

  - And currently the location of gettext mo files are
    not correct because build log says:
-----------------------------------------------------------
-DLOCALEDIR=\"/usr/share/xblast/locale\"
-----------------------------------------------------------
    This should be moved to %{_datadir}/locale (well, some
    messages are corrupted on both fr_FR and de_DE, perhaps
    due to ISO-8859 style vs UTF-8 style).

(In reply to comment #2)
> I will review this later.
> By the way I will appreciate it if you would review my
> mecab related review request (bug 229927 and bug 229929).
> Thanks!

Well me reviewing some packages in return is only fare, so sure I'll take a
stab. But tomorrow I'm going to fosdem 2007 in Brussels, so don't expect a full
review this weekend. Probably somewhere next week.

(In reply to comment #3)
> Well,
> 
> A. First for xblast-2.10.4-1:
> 
> * File dependency
>   - Writing the package which provides the file is recommended
>     expect you have somewhat strong reason to write file dependency
>     (for vera font). Please check:
>     http://fedoraproject.org/wiki/PackagingDrafts/FileDeps
>     (This is a draft)
> 

Fonts have been known to move from one location to another as packaging insights
surrounding fonts change, since xblast opens the font through an absolute patch
I want xblast to break when this happens.

> * Source URL
>   - Please use http://downloads.sourceforge.net/<package_name>/XXXX.tar.gz
>     if it is possible. Please check:
>     http://fedoraproject.org/wiki/PackagingDrafts/SourceUrl
>     (This is a draft).

This Draft is clearly written by someone with not so much sourceforge
experience, downloads.sourceforge.net (or dl.sf.net which is a shorter alias but
otherwise exactly the same) will do a dumb redirect to a mirror, I say a dumb
redirect as it will take the file location after the hostname as is without any
checking and then postfix this to the choosen mirrors hostname to get the URL to
redirect to.

Now most mirrors will work fine with dl.sf.net/%{name}/xxx, but some mirrors
will only work with dl.sf.net/sourceforge/%{name}/xxx, notice that this longer
version will also work on mirrors which accept dl.sf.net/%{name}/xxx, as they
seem to have a symlink to / called sourceforge :)

Thus the draft is wrong. I've added a comment to this extend to the draft.

>   - Please specify the URL of xblast.png if possible.
> 

I took this from a suse srom, so no URL I'm afraid.


> * Timestamps
> ----------------------------------------------------------
> install -m 755 %{SOURCE3} $RPM_BUILD_ROOT%{_bindir}/%{name}
> ----------------------------------------------------------
>   - This is only a wrapper script and keeping timestamp
>    (i.e. install -p) is recommended.
> 

I will fix this as soon as the other points are clear.

> * Documentation
>   - Perhaps the following files can be used.
> ----------------------------------------------------------
> ./xblast.man
> ----------------------------------------------------------
> 

Good catch, it needs some work amongst others the trademark bomberman name must
be stripped from it, but its salvegable :) I will fix this as soon as the other
points are clear.


> * Functionality
>   - xblast-x11 cannot be launched for me.
> ----------------------------------------------------------
> [tasaka1@localhost xblast]$ xblast-x11 
> could not load font 24
> could not load font 18
> could not load font 14
> X Error of failed request:  BadFont (invalid Font parameter)
>   Major opcode of failed request:  56 (X_ChangeGC)
>   Resource id in failed request:  0x800010
>   Serial number of failed request:  519
>   Current serial number in output stream:  541
> -----------------------------------------------------------
> 

Looks like it will need a dep on some not by default installed X11 fonts which I
have installed and you don't. Can you try installing "xorg-x11-fonts-misc" ?

> * Directory/file ownership
>   - Well as the build log says:
> -----------------------------------------------------------
> -DGAME_DATADIR=\"/usr/share/xblast\"
> -----------------------------------------------------------
>     I think that %{_datadir}/xblast should be owned by
>     xblast-common, not by xblast-data because xblast requires
>     that the files are installed under %{_datadir}/xblast.
> 

Erm, why xblast and xblast-x11 need files from under this dir, but they place no
files there themselves, since they need the files they require xblast-data,
which has the files and does the dir. Whats the use of owning a dir you don't
put files in? Actually by doing things that way, combined with the circular deps
this has chances are that the directory will not be removed, because xblast-data
could be removed after xblast-data at which moment it will not be empty.


>   - And currently the location of gettext mo files are
>     not correct because build log says:
> -----------------------------------------------------------
> -DLOCALEDIR=\"/usr/share/xblast/locale\"
> -----------------------------------------------------------
>     This should be moved to %{_datadir}/locale (well, some
>     messages are corrupted on both fr_FR and de_DE, perhaps
>     due to ISO-8859 style vs UTF-8 style).

I will fix the location as soon as the other points are clear. And I'll look
into the encodings, but that shouldn't be a problem as .po /.mo files should
have the encoding specified in their header and on the fly conversion will be
done by gettext when nescesarry. Did you see these problems with xblast-sdl,
xblast-x11 or both?

Well,

* x11 version problem
----------------------------------------------------------
[tasaka1@localhost xblast]$ xblast-x11 
could not load font 24
could not load font 18
could not load font 14
-----------------------------------------------------------
  - turns out that this is because x11 version is not compiled
    with "-DMINI_XBLAST" (check x11_config.c). x11 version still
    requests vera font, however vera font is not in the font
    search path.
    BYW according to x11_config.c, x11 version requires some font
    from xorg-x11-fonts-ISO8859???? (note: I think that these
    font packages are not so usual for non-American/European users
    like me).

* gettext translation character corruption
  - well, x11 version does not use gettext, so the sentences
    are always English and have no problem.
  - some character corruption occurs on -sdl version (currently
    I created a link as /usr/share/xblast/locale -> ../locale)
    on both de_DE and fr_FR.

* Directory ownership:
> Actually by doing things that way, combined 
> with the circular deps this has chances are that 
> the directory will not be removed, because xblast-data
> could be removed after xblast-data at which moment it will not be 
> empty.
  - Okay. I catched what you mean. You want to remove
    all packages related to xblast by "yum remove xblast", right?
    Your opinition is reasonable and for this I respect your
    choice.

Oops... not BYW but BTW (by the way)

(In reply to comment #6)
> Well,
> 
> * x11 version problem
> ----------------------------------------------------------
> [tasaka1@localhost xblast]$ xblast-x11 
> could not load font 24
> could not load font 18
> could not load font 14
> -----------------------------------------------------------
>   - turns out that this is because x11 version is not compiled
>     with "-DMINI_XBLAST" (check x11_config.c). x11 version still
>     requests vera font, however vera font is not in the font
>     search path.
>     BYW according to x11_config.c, x11 version requires some font
>     from xorg-x11-fonts-ISO8859???? (note: I think that these
>     font packages are not so usual for non-American/European users
>     like me).
> 

Using -DMINI_XBLAST causes xblast to be compiled for low res screens and thus
draw  everything in a twice as low resolution, I don't think we want that.
The proper fix would be to add a Requires: xorg-x11-fonts-ISO8859-1-75dpi

> * gettext translation character corruption
>   - well, x11 version does not use gettext, so the sentences
>     are always English and have no problem.
>   - some character corruption occurs on -sdl version (currently
>     I created a link as /usr/share/xblast/locale -> ../locale)
>     on both de_DE and fr_FR.
> 

I've tried this, but I see no problems on my 64 bit rawhide machine, can you
give some examples?

Regards,

Hans

(In reply to comment #8)
> (In reply to comment #6)
> > Well,
> > 
> > * x11 version problem
> > ----------------------------------------------------------
> > [tasaka1@localhost xblast]$ xblast-x11 
> > could not load font 24
> > could not load font 18
> > could not load font 14
> > -----------------------------------------------------------
> >   - turns out that this is because x11 version is not compiled
> >     with "-DMINI_XBLAST" (check x11_config.c). x11 version still
> >     requests vera font, however vera font is not in the font
> >     search path.
> >     BYW according to x11_config.c, x11 version requires some font
> >     from xorg-x11-fonts-ISO8859???? (note: I think that these
> >     font packages are not so usual for non-American/European users
> >     like me).
> > 
> 
> Using -DMINI_XBLAST causes xblast to be compiled for low res screens and thus
> draw  everything in a twice as low resolution, I don't think we want that.
> The proper fix would be to add a Requires: xorg-x11-fonts-ISO8859-1-75dpi

I have already installed xorg-x11-fonts-ISO8859-1-75dpi.
Does xblast-x11 really work for you?
For me it seems this is due that -x11 version want to use
vera font, still vera font is not in the font search path.
Would you check x11_config.c?

> 
> > * gettext translation character corruption
> >   - well, x11 version does not use gettext, so the sentences
> >     are always English and have no problem.
> >   - some character corruption occurs on -sdl version (currently
> >     I created a link as /usr/share/xblast/locale -> ../locale)
> >     on both de_DE and fr_FR.
> > 
> 
> I've tried this, but I see no problems on my 64 bit rawhide machine, can you
> give some examples?
I will attach a screenshot.

Mamoru

Created attachment 148774 [details]
screenshot of xblast-sdl on LANG=fr_FR

screenshot on
LANG=fr_FR xblast-sdl

(In reply to comment #9)
> > The proper fix would be to add a Requires: xorg-x11-fonts-ISO8859-1-75dpi
> 
> I have already installed xorg-x11-fonts-ISO8859-1-75dpi.
> Does xblast-x11 really work for you?
> For me it seems this is due that -x11 version want to use
> vera font, still vera font is not in the font search path.
> Would you check x11_config.c?
> 

I've checked it and the relevant lines are:
        "-*-helvetica-bold-r-*-*-14-*-*-*-*-*-iso8859-*",       /* small */
        "-*-helvetica-bold-r-*-*-18-*-*-*-*-*-iso8859-*",       /* medium */
        "-*-helvetica-bold-r-*-*-24-*-*-*-*-*-iso8859-*",       /* large */

Note that that is convoluted X11 font naming, now if you look at:
/usr/share/X11/fonts/75dpi/fonts.dir

You will find in there:
helvB24-ISO8859-1.pcf.gz
-adobe-helvetica-bold-r-normal--24-240-75-75-p-138-iso8859-1

And rpm -qf /usr/share/X11/fonts/75dpi/helvB24-ISO8859-1.pcf.gz gives:
xorg-x11-fonts-ISO8859-1-75dpi-7.1-1.noarch

Unfortunately we cannot use strace here as this is going through X.

Maybe something is busted with your X-setup? :
* Does /etc/X11/fs/config contain:
  /usr/share/X11/fonts/75dpi:unscaled
  in the catalogue = ... lines?
* And does /etc/X11/xorg.conf contain:
  FontPath     "unix/:7100"
  In the Files section?

Can you try "entering" "-*-helvetica-bold-r-*-*-24-*-*-*-*-*-iso8859-*" into
xfontsel (from xorg-x11-utils package), maybe you've got more then one font
matching "-*-helvetica-bold-r-*-*-24-*-*-*-*-*-iso8859-*" and maybe that is the
problem?

> > I've tried this, but I see no problems on my 64 bit rawhide machine, can you
> > give some examples?
> I will attach a screenshot.
> 

I see your problem, and I don't have it I'll attach a screenshot of my version,
I'm using "export LANG=fr_FR.UTF-8" to test, are you also using UTF-8 ?

Created attachment 148785 [details]
Working French screenshot

* For garbage character, it disappeared when I type:
  "LANG=de_DE.UTF-8 xblast-sdl". Sorry for noises.

(In reply to comment #11)
> I've checked it and the relevant lines are:
>         "-*-helvetica-bold-r-*-*-14-*-*-*-*-*-iso8859-*",       /* small */
>         "-*-helvetica-bold-r-*-*-18-*-*-*-*-*-iso8859-*",       /* medium */
>         "-*-helvetica-bold-r-*-*-24-*-*-*-*-*-iso8859-*",       /* large */
> 
> You will find in there:
> helvB24-ISO8859-1.pcf.gz
> -adobe-helvetica-bold-r-normal--24-240-75-75-p-138-iso8859-1
>> Maybe something is busted with your X-setup? :
> * Does /etc/X11/fs/config contain:
>   /usr/share/X11/fonts/75dpi:unscaled
>   in the catalogue = ... lines?
> * And does /etc/X11/xorg.conf contain:
>   FontPath     "unix/:7100"
>   In the Files section?
Well, FontPath is okay, font search path are:
----------------------------------------------------
catalogue = /usr/share/X11/fonts/misc:unscaled,
        /usr/share/X11/fonts/75dpi:unscaled,
        /usr/share/X11/fonts/100dpi:unscaled,
        /usr/share/X11/fonts/Type1,
        /usr/share/fonts/default/Type1,
        /usr/share/fonts/default/ghostscript,
        ,
        /usr/share/fonts/korean/misc:unscaled,
        /usr/share/fonts/korean/misc,
        /usr/share/fonts/korean/TrueType,
        /usr/share/fonts/chinese/misc:unscaled,
        /usr/share/fonts/chinese/misc,
        /usr/share/fonts/chinese/TrueType,
        /usr/share/fonts/japanese/misc:unscaled,
        /usr/share/fonts/japanese/misc,
        /usr/share/fonts/japanese/TrueType/S2G,
        /usr/share/fonts/japanese/TrueType/mikachan,
        /usr/share/fonts/japanese/TrueType/neuropol,
        /usr/share/fonts/japanese/TrueType,
        /usr/share/fonts/japanese/efont-unicode-bdf,
        /usr/share/X11/fonts/75dpi:unscaled,
        /usr/share/X11/fonts/TTF,
        /usr/share/X11/fonts/75dpi
----------------------------------------------------

> 
> Can you try "entering" "-*-helvetica-bold-r-*-*-24-*-*-*-*-*-iso8859-*" into
> xfontsel (from xorg-x11-utils package), maybe you've got more then one font
> matching "-*-helvetica-bold-r-*-*-24-*-*-*-*-*-iso8859-*" and maybe that is the
> problem?

For me, with xfontsel
--------------------------------------------------------
"-*-helvetica-bold-r-*-*-14-*-*-*-*-*-iso8859-*",   12 names match
"-*-helvetica-bold-r-*-*-18-*-*-*-*-*-iso8859-*",    9 names match
"-*-helvetica-bold-r-*-*-24-*-*-*-*-*-iso8859-*",    9 names match
---------------------------------------------------------

Well I just tried installing all the xorg-x11-fonts-ISO8859* and xblast-x11
still works fine :|

So I'm afraid that there is something broken with your setup. I've tried it on
my work machine too (some days ago) and it ran fine there too. Both are rawhide
machines though.

I believe that adding a Requires: xorg-x11-fonts-ISO8859-1-75dpi should be
sufficient normally. Why it isn't working on your machine I don't understand.

Well, I tried on FC5 system and the result was ... no problem ...
Also for ISO8859 fonts:
------------------------------------------
[tasaka1@localhost ~]$ grep ISO8859 /var/log/rpmpkgs
xorg-x11-fonts-ISO8859-1-100dpi-7.0-3.noarch.rpm
xorg-x11-fonts-ISO8859-1-75dpi-7.0-3.noarch.rpm
------------------------------------------
Only two rpms are installed but it was okay...

So for now I assume that -x11 should work normally. So
would you update srpm/spec with the left issues fixed?

(In reply to comment #15)
> So for now I assume that -x11 should work normally. So
> would you update srpm/spec with the left issues fixed?

Done, new version here:

* Thu Mar  1 2007 Hans de Goede <j.w.r.degoede> 2.10.4-2
- Use sf.net sourceforge URL from the Guidelines (bz 229476)
- Keep timestamp while installing the wrapper (bz 229476)
- Sanitize and install the manpage (bz 229476)
- Add "Requires: xorg-x11-fonts-ISO8859-1-75dpi" to xblast-x11 (bz 229476)
- Make xblast look for translations under /usr/share/locale (bz 229476)

Spec URL: http://people.atrpms.net/~hdegoede/xblast.spec
SRPM URL: http://people.atrpms.net/~hdegoede/xblast-2.10.4-2.fc7.src.rpm

Well,

A. for xblast-2.10.4-2:
* Timestamps
---------------------------------------------
install -m 644 %{name}.man $RPM_BUILD_ROOT%{_mandir}/man6/%{name}.6
---------------------------------------------
  - Please use "install -p" (gzip keeps timestamp, so the timestamp
    of this file is finally kept).

Other things are okay.
------------------------------------------------------
  This package (xblast) is APPROVED by me.
------------------------------------------------------

(In reply to comment #17)
> Well,
> 
> A. for xblast-2.10.4-2:
> * Timestamps
> ---------------------------------------------
> install -m 644 %{name}.man $RPM_BUILD_ROOT%{_mandir}/man6/%{name}.6
> ---------------------------------------------
>   - Please use "install -p" (gzip keeps timestamp, so the timestamp
>     of this file is finally kept).


That file gets patched and thus the timestamp gets set to the build time, so
there is no use in using -p.

New Package CVS Request
=======================
Package Name:            xblast
Short Description:       Lay bombs and Blast the other players of the field
Owners:                  j.w.r.degoede
Branches:                FC-6 devel
InitialCC:               empty

(In reply to comment #18)
> (In reply to comment #17)
> > A. for xblast-2.10.4-2:
> > * Timestamps
> > ---------------------------------------------
> > install -m 644 %{name}.man $RPM_BUILD_ROOT%{_mandir}/man6/%{name}.6
> > ---------------------------------------------
> >   - Please use "install -p" (gzip keeps timestamp, so the timestamp
> >     of this file is finally kept).
> 
> 
> That file gets patched and thus the timestamp gets set to the build time, so
> there is no use in using -p.
Okay.

CVS done

Imported and build, as usual thanks for the review!

Closing.