Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=454166
Summary: Review Request: griv - a gtk rivchat Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: low Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: cassmodiah@fedoraproject.org QAContact: extras-qa@fedoraproject.org CC: fedora-package-review@redhat.com,notting@redhat.com
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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
wolfy@nobugconsulting.ro changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO| |177841 nThis| |
------- Additional Comments From wolfy@nobugconsulting.ro 2008-07-05 17:08 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From wolfy@nobugconsulting.ro 2008-07-05 17:08 EST ------- Oh, and the translation (the pl/*mo) should be handled by the %find_lang macro
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From cassmodiah@fedoraproject.org 2008-07-05 17:47 EST -------
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...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From wolfy@nobugconsulting.ro 2008-07-06 16:39 EST ------- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From cassmodiah@fedoraproject.org 2008-07-08 15:00 EST ------- 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...
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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
cheekyboinc@foresightlinux.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |cheekyboinc@foresightlinux.o | |rg
------- Additional Comments From cheekyboinc@foresightlinux.org 2008-07-09 13:15 EST ------- Manuel, are you willing to do a formal review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
mtasaka@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |mtasaka@ioa.s.u-tokyo.ac.jp Status|NEW |ASSIGNED Flag| |fedora-review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2008-07-21 11:00 EST ------- Created an attachment (id=312268) --> (https://bugzilla.redhat.com/attachment.cgi?id=312268&action=view) 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 -------------------------------------------------------------
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From wolfy@nobugconsulting.ro 2008-07-21 11:13 EST ------- 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 .........
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2008-07-21 11:30 EST ------- (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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From wolfy@nobugconsulting.ro 2008-07-21 11:55 EST -------
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 )
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2008-07-21 12:13 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From cassmodiah@fedoraproject.org 2008-07-29 14:34 EST ------- 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...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From wolfy@nobugconsulting.ro 2008-07-29 16:20 EST ------- 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)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
mtasaka@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|177841 | nThis| |
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2008-07-30 03:46 EST ------- Removing NEEDSPONSOR as I am sponsoring the submitter (bug 454208) Review will follow later.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2008-07-30 10:08 EST ------- Created an attachment (id=313000) --> (https://bugzilla.redhat.com/attachment.cgi?id=313000&action=view) 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From cassmodiah@fedoraproject.org 2008-07-30 13:19 EST ------- Created an attachment (id=313014) --> (https://bugzilla.redhat.com/attachment.cgi?id=313014&action=view) desktopfile patch
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
cassmodiah@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #313014|application/octet-stream |text/plain mime type| | Attachment #313014|0 |1 is patch| |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From cassmodiah@fedoraproject.org 2008-07-30 13:27 EST ------- 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)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2008-07-30 13:38 EST ------- (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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
------- Additional Comments From cassmodiah@fedoraproject.org 2008-07-30 16:20 EST ------- 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...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
mtasaka@ioa.s.u-tokyo.ac.jp changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
------- Additional Comments From mtasaka@ioa.s.u-tokyo.ac.jp 2008-07-31 03:08 EST ------- Okay, clean.
------------------------------------------------------------- This package (griv) is APPROVED by me -------------------------------------------------------------
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
simon@w3sp.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
------- Additional Comments From simon@w3sp.de 2008-07-31 13:51 EST ------- New Package CVS Request ======================= Package Name: griv Short Description: a gtk rivchat Owners: cassmodiah Branches: F-8 F-9 InitialCC: Cvsextras Commits: yes
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: griv - a gtk rivchat
https://bugzilla.redhat.com/show_bug.cgi?id=454166
kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
------- Additional Comments From kevin@tummy.com 2008-08-01 11:51 EST ------- cvs done.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla