Bug 454166 - Review Request: griv - a gtk rivchat
Review Request: griv - a gtk rivchat
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-05 16:48 EDT by Simon
Modified: 2008-08-03 09:13 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-03 09:13:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patch to honor Fedora cflags correctly (701 bytes, patch)
2008-07-21 11:00 EDT, Mamoru TASAKA
no flags Details | Diff
Patch to suppress implicit function declaration warning (982 bytes, patch)
2008-07-30 10:08 EDT, Mamoru TASAKA
no flags Details | Diff
desktopfile patch (293 bytes, patch)
2008-07-30 13:19 EDT, Simon
no flags Details | Diff

  None (edit)
Description Simon 2008-07-05 16:48:05 EDT
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 17:08:18 EDT
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 17:08:55 EDT
Oh, and the translation (the pl/*mo) should be handled by the %find_lang macro
Comment 3 Simon 2008-07-05 17:47:09 EDT
> 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 16:39:13 EDT
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 15:00:35 EDT
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 13:15:30 EDT
Manuel, are you willing to do a formal review?
Comment 7 Mamoru TASAKA 2008-07-21 11:00:51 EDT
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 11:13:53 EDT
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 11:30:59 EDT
(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 11:55:44 EDT
>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 12:13:02 EDT
(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 16:20:03 EDT
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 03:46:32 EDT
Removing NEEDSPONSOR as I am sponsoring the submitter (bug 454208)
Review will follow later.
Comment 15 Mamoru TASAKA 2008-07-30 10:08:41 EDT
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 13:19:04 EDT
Created attachment 313014 [details]
desktopfile patch
Comment 17 Simon 2008-07-30 13:27:55 EDT
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 13:38:59 EDT
(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 03:08:14 EDT
Okay, clean.

-------------------------------------------------------------
    This package (griv) is APPROVED by me
-------------------------------------------------------------
Comment 21 Simon Wesp 2008-07-31 13:51:47 EDT
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 11:51:11 EDT
cvs done.
Comment 23 Mamoru TASAKA 2008-08-03 08:57:18 EDT
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 09:13:23 EDT
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.