Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 454166 - Review Request: griv - a gtk rivchat
Summary: Review Request: griv - a gtk rivchat
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-07-05 20:48 UTC by Simon
Modified: 2008-08-03 13:13 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-03 13:13:23 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch to honor Fedora cflags correctly (deleted)
2008-07-21 15:00 UTC, Mamoru TASAKA
no flags Details | Diff
Patch to suppress implicit function declaration warning (deleted)
2008-07-30 14:08 UTC, Mamoru TASAKA
no flags Details | Diff
desktopfile patch (deleted)
2008-07-30 17:19 UTC, Simon
no flags Details | Diff

Description Simon 2008-07-05 20:48:05 UTC
Spec URL: http://packages.cassmodiah.de/fedora/griv/griv.spec
SRPM URL: http://packages.cassmodiah.de/fedora/griv/griv-0.1.9-1.fc9.src.rpm
Description: griv is a gtk chat using a modified riv-protocol for a serverless communication in a network. In this version the autoaway will beactive.

There is just one Problem, the standard-port 16127 is closed by default and has to be opened manually. 

This is my first package and I need a sponsor

Comment 1 manuel wolfshant 2008-07-05 21:08:18 UTC
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.

Comment 2 manuel wolfshant 2008-07-05 21:08:55 UTC
Oh, and the translation (the pl/*mo) should be handled by the %find_lang macro

Comment 3 Simon 2008-07-05 21:47:09 UTC
> 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

Comment 4 manuel wolfshant 2008-07-06 20:39:13 UTC
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



Comment 5 Simon 2008-07-08 19:00:35 UTC
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



Comment 6 Stefan Posdzich 2008-07-09 17:15:30 UTC
Manuel, are you willing to do a formal review?

Comment 7 Mamoru TASAKA 2008-07-21 15:00:51 UTC
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
-------------------------------------------------------------

Comment 8 manuel wolfshant 2008-07-21 15:13:53 UTC
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 .........



Comment 9 Mamoru TASAKA 2008-07-21 15:30:59 UTC
(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?


Comment 10 manuel wolfshant 2008-07-21 15:55:44 UTC
>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 )

Comment 11 Mamoru TASAKA 2008-07-21 16:13:02 UTC
(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.



Comment 13 manuel wolfshant 2008-07-29 20:20:03 UTC
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)

Comment 14 Mamoru TASAKA 2008-07-30 07:46:32 UTC
Removing NEEDSPONSOR as I am sponsoring the submitter (bug 454208)
Review will follow later.

Comment 15 Mamoru TASAKA 2008-07-30 14:08:41 UTC
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.

Comment 16 Simon 2008-07-30 17:19:04 UTC
Created attachment 313014 [details]
desktopfile patch

Comment 17 Simon 2008-07-30 17:27:55 UTC
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)



Comment 18 Mamoru TASAKA 2008-07-30 17:38:59 UTC
(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.


Comment 20 Mamoru TASAKA 2008-07-31 07:08:14 UTC
Okay, clean.

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

Comment 21 Simon Wesp 2008-07-31 17:51:47 UTC
New Package CVS Request
=======================
Package Name: griv
Short Description: a gtk rivchat
Owners: cassmodiah
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes

Comment 22 Kevin Fenzi 2008-08-01 15:51:11 UTC
cvs done.

Comment 23 Mamoru TASAKA 2008-08-03 12:57:18 UTC
Please close this bug as NEXTRELEASE when you rebuilt this package on
devel/F-9/F-8 and request on bodhi is done.

Comment 24 Mamoru TASAKA 2008-08-03 13:13:23 UTC
Closing as rebuild is done and request on bodhi is done.


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