What's the reason for including perl and intltool as Requires ? rpmbuild picks
libgtk-x11 as dependency so (without testing the program itself) I'd say that
completely removing the Requires line is a good idea.

Oh, and the translation (the pl/*mo) should be handled by the %find_lang macro

> What's the reason for including perl and intltool as Requires?
I thougt Requires is a mandatory field

new version:
SPEC URL: http://packages.cassmodiah.de/fedora/griv/bug-454166/05.07-2343/griv.spec
SRPM URL:
http://packages.cassmodiah.de/fedora/griv/bug-454166/05.07-2343/griv-0.1.9-1.fc9.src.rpm

two minor nitpicks:
- please bump the release each time you modify the spec. this makes life easier
for whoever wants to track the differences
- you have some duplicate BuildRequires: libX11-devel (by gtk2-devel),
xorg-x11-proto-devel (by libX11-devel)

Now, the serious problems
- the mandatory compiling flags are not obeyed to. See
https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags for details
- packaging a desktop file is not enough to make is useful. You should also
follow https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files in
order to a) make sure it is correct and b) install it

Sorry for my late response. I hope the desktopfile and the compilerflags are now
correct.


SPEC URL:
http://packages.cassmodiah.de/fedora/griv/bug-454166/griv-0.1.9-2/griv.spec
SRPM URL:
http://packages.cassmodiah.de/fedora/griv/bug-454166/griv-0.1.9-2/griv-0.1.9-2.fc9.src.rpm

Changelog:
- Remove duplicate BuildRequires: libX11-devel (by gtk2-devel)
- Remove duplicate BuildRequires: xorg-x11-proto-devel (by libX11-devel)
- Add compiler flags
- Add installer for .desktop file
- Add icon-install
- Add script to update gtk-iccon-cache
- Create and add manpage

Manuel, are you willing to do a formal review?

Created attachment 312268 [details]
Patch to honor Fedora cflags correctly

For 0.1.9-2:

* License
  - License tag should be "GPLv3".

* BuildRequires
  - Please don't write "perl-devel" as BuildRequies
    Instead please use module name virtual Provides the needed rpms Provide:
    https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides

* CFLAGS
  - Fedora specific compilation flags are not correctly passed yet:
    http://koji.fedoraproject.org/koji/getfile?taskID=729334&name=build.log
    (Parent task:
     http://koji.fedoraproject.org/koji/taskinfo?taskID=729331 )
    Makefiles in the tarball is wrong.
    Please apply the patch attached and remove extra lines
-------------------------------------------------------------
CFLAGS="${CFLAGS:-$RPM_OPT_FLAGS}"
export CFLAGS
-------------------------------------------------------------
    ! Note
      These 2 lines should not be needed anyway. You can check what
      %configure actually does by
      $ rpm --eval %configure

* icons
  - Please verify if installing a icon into
    %{_datadir}/icons/hicolor/22x22/apps is really needed.
    The same icon is already installed under %{_datadir}/pixmaps/.

* Documents
  - Please add the following files to %doc.
-------------------------------------------------------------
AUTHORS
Changelog
-------------------------------------------------------------

There are a couple of problems. The major one is that , according to the build
log, the RPMOPT flags are not used:
gcc -DHAVE_CONFIG_H -I. -I..     -I/usr/include/gtk-2.0
-I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/
usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
-I/usr/include/freetype2   -MT conf.o -MD -MP -MF .deps/
conf.Tpo -c -o conf.o conf.c

while, a few lines above (before %configure) we have:
+ CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic'
+ export CFLAGS

I've just come back from vacation and I do not have the time for a deeper check,
but I suspect that the makefile needs a bit of love.

Minor nitpicks
- there is no need to delete the original desktop file from the tree
- please use either RPMBUILDROOT or rpmbuildroot, but not both
- the Icon tag in the desktop file should either use the full path to the icon
or the icon name without extension ( see Packaging/Guidelines#desktop ) 
- Your idea to create a man page is excellent and once you settle on a final
format of the file, I suggest to send it upstream for inclusion in their next
release. However the current wording needs a bit of improvement. I am not a
native English speaker either, so take the next lines with a grain of salt. I
have included below a slightly modified text for the Description paragraph of
the man page. Feel free to use it (or not):

DESCRIPTION
  griv is a serverless lan chat program, with the protocol based on RivChat by
Arkadiusz Kolacz (Wielebny K.)
  The specification is available at .........

(In reply to comment #8)
> There are a couple of problems. The major one is that , according to the build
> log, the RPMOPT flags are not used:

Would you check my comment?

> Minor nitpicks
> - there is no need to delete the original desktop file from the tree

Why? Unless using "--delete-original", this is needed (if --vendor fedora is
specified)

> - please use either RPMBUILDROOT or rpmbuildroot, but not both

Where is it used?

>Would you check my comment?
I apologize, I have started the review quite early today but I have been
interrupted by $RegularWork. Before I had the chance to come back to the review
in order to end it, you have replied and assigned the bug to yourself.


>Why? Unless using "--delete-original", this is needed (if --vendor fedora is
specified)
Because either --delete-original in desktop-file-install or an exclude line in
%files can be used


>> - please use either RPMBUILDROOT or rpmbuildroot, but not both
>Where is it used?
desktop-file-install --vendor="fedora" \
        --dir=$RPM_BUILD_ROOT%{_datadir}/applications \
        %{buildroot}/%{_datadir}/applications/%{name}.desktop 

(obviously, in #8 I meant %buildroot not rpmbuildroot )

(In reply to comment #10)
> >Why? Unless using "--delete-original", this is needed (if --vendor fedora is
> specified)
> Because either --delete-original in desktop-file-install or an exclude line in
> %files can be used

Well, slightly unrelated to this, however I usually recommend _not_ to
use %exclude but to remove files explicitly unless avoided such cases like
- Some complicated %files entry description like a situation in which some files
  in a same directories are packaged in different subpackages
- .py{o,c} under %_bindir
... especially when including binaries (for this package, desktop file is a text
file
and --delete-original can be used here). For binaries, using %exclude makes
debuginfo rpm
larger unneededly (because splitted out part of binaries are included into
debuginfo rpm
even if they are %exclude'd). To avoid further issues I don't know, 
I recommend not to use %exclude.

> >> - please use either RPMBUILDROOT or rpmbuildroot, but not both
> >Where is it used?
> desktop-file-install --vendor="fedora" \
>         --dir=$RPM_BUILD_ROOT%{_datadir}/applications \
>         %{buildroot}/%{_datadir}/applications/%{name}.desktop 
> 
> (obviously, in #8 I meant %buildroot not rpmbuildroot )

Ah, okay, sorry for not noticing this.

sorry for my late response.
i was very busy

one more try:

SPEC: http://packages.cassmodiah.de/fedora/griv/bug-454166/griv-0.1.9-3/griv.spec
SRPM:
http://packages.cassmodiah.de/fedora/griv/bug-454166/griv-0.1.9-3/griv-0.1.9-3.fc9.src.rpm

there is a small typo in the changelog, a missing space between "sed" and
"command"...
please try to keep in mind to fix it before uploading to CVS (or for the next
release of the package, should it be needed)

Removing NEEDSPONSOR as I am sponsoring the submitter (bug 454208)
Review will follow later.

Created attachment 313000 [details]
Patch to suppress implicit function declaration warning

For -3:

* BR: perl-devel
  - Writing "BuildRequires: perl-devel" is not recommended on Fedora:
    https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides

    For this package "BuildRequires: perl(XML::Parser)" is sufficient
    (and even this is redundant because perl(XML::Parser) is required by
     intltool)

* sed -i "s|%{name}.png|%{name}|"
  - Fixing Icon entry on the desktop file after executing desktop-file-install
    leaves a warning like:
---------------------------------------------------------------
   524 
/builddir/rpmbuild/BUILDROOT/griv-0.1.9-3.fc10.i386/usr/share/applications/fedora-griv.desktop:
warning: val
ue "griv.png" for key "Icon" in group "Desktop Entry" is an icon name with an
extension, but there should be no exte
nsion as described in the Icon Theme Specification if the value is not an
absolute path
---------------------------------------------------------------
    Fix griv.desktop(.in) before desktop-file-install is called (generally
    modifying griv.desktop.in in %prep is preferred)

! Misc issue
  - Please consider to apply the patch attached to suppress implicit function
    declaration function warning.

Created attachment 313014 [details]
desktopfile patch

ah, okay, okay

i thought i removed perl-devel. i wrote it in changelog, because i found the
xmlparser in intltool, too. :-)
sorry for my mistake

should i replace the sed command with the diff-patch above? eventually it would
be easier, than handle it with sed.(In reply to comment #16)

(In reply to comment #17)
> should i replace the sed command with the diff-patch above? eventually it would
> be easier, than handle it with sed.(In reply to comment #16)

Either will be okay.

SPEC:http://packages.cassmodiah.de/fedora/griv/bug-454166/griv-0.1.9-4/griv.spec
SRPM:http://packages.cassmodiah.de/fedora/griv/bug-454166/griv-0.1.9-4/griv-0.1.9-4.fc9.src.rpm

Okay, clean.

-------------------------------------------------------------
    This package (griv) is APPROVED by me
-------------------------------------------------------------

New Package CVS Request
=======================
Package Name: griv
Short Description: a gtk rivchat
Owners: cassmodiah
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes

cvs done.

Please close this bug as NEXTRELEASE when you rebuilt this package on
devel/F-9/F-8 and request on bodhi is done.

Closing as rebuild is done and request on bodhi is done.