Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: pidgin-gfire - Xfire plugin for pidgin
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Summary: Review Request: pidgin-gfire - Xfire plugin for pidgin Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: low Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: th0br0@mkdir.name QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
Spec URL: http://fedora.mkdir.name/packages/pidgin-gfire-0.7.1/pidgin-gfire.spec
SRPM URL: http://fedora.mkdir.name/packages/pidgin-gfire-0.7.1/pidgin-gfire-0.7.1-1.fc...
Description: Gfire is an open source plugin for the Pidgin IM client which allows you to connect the Xfire network.
Features: * Friend’s list * Add & Remove friends * View friend’s current game (games list updated weekly) * View friend’s profiles * Join friend’s server * Detects games and logs game time * Set your status as “Away from Keyboard (AFK)” * Set custom status
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Andreas Osowski th0br0@mkdir.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841(FE-NEEDSPONSOR)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jussi.lehtola@iki.fi
--- Comment #1 from Jussi Lehtola jussi.lehtola@iki.fi 2009-03-28 05:34:08 EDT --- This doesn't seem to me to be a pidgin plugin, more like a purple plugin. Thus the name should be purple-gfire and BuildRequires: libpurple-devel.
The existence of the pixmap does call for the Requires: pidgin.
Fix mixed use of tabs and spaces: pidgin-gfire.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 15)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Andreas Osowski th0br0@mkdir.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: |Review Request: |pidgin-gfire - Xfire plugin |purple-gfire - Xfire plugin |for pidgin |for libpurple
--- Comment #2 from Andreas Osowski th0br0@mkdir.name 2009-03-28 06:09:41 EDT --- Spec URL: http://fedora.mkdir.name/packages/purple-gfire-0.7.1/purple-gfire.spec
SRPM URL: http://fedora.mkdir.name/packages/purple-gfire-0.7.1/purple-gfire-0.7.1-2.fc...
* Sat Mar 28 2009 Andreas Osowski th0br0@mkdir.name - 0.7.1-2 - Renamed to purple-gfire - Fixed indentation - Changed description
rpmlint now remains quiet.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Andreas Osowski th0br0@mkdir.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841(FE-NEEDSPONSOR) |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
--- Comment #3 from Andreas Osowski th0br0@mkdir.name 2009-05-20 12:46:31 EDT --- Spec URL: http://fedora.mkdir.name/packages/purple-gfire-0.8.1/purple-gfire.spec
SRPM URL: http://fedora.mkdir.name/packages/purple-gfire-0.8.1/purple-gfire-0.8.1-1.fc...
* Wed May 20 2009 Andreas Osowski th0br0@mkdir.name - 0.8.1-1 - New upstream version
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |jussi.lehtola@iki.fi Flag| |fedora-review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
--- Comment #4 from Jussi Lehtola jussi.lehtola@iki.fi 2009-05-20 14:23:32 EDT --- (In reply to comment #1)
This doesn't seem to me to be a pidgin plugin, more like a purple plugin. Thus the name should be purple-gfire and BuildRequires: libpurple-devel.
Hmm, I no longer agree with myself :D This is after all a pidgin plugin, at least the home page clearly says so. So the name should be after all pidgin-xfire. Sorry for the hassle.
Now:
- You should Requires: libpurple explicitly for dir ownership, even though this should be automatically picked up by rpm as a lib dependency. Also, please make a comment about the dir ownership section of Requires, so that you/other people will not remove it without knowing why it's there.
- Please don't use macros when not necessary (%{__chmod} is just chmod).
- Change %{_datadir}/purple/%{srcname}/* %{_datadir}/purple/%{srcname}/scripts/server_detection/* to %{_datadir}/purple/%{srcname}/ as this will own the directory (necessary) and everything in it. Now the package doesn't own the directory /usr/share/purple/gfire and you are listing stuff twice:
warning: File listed twice: /usr/share/purple/gfire/scripts/server_detection/4097.py warning: File listed twice: /usr/share/purple/gfire/scripts/server_detection/4097.pyc warning: File listed twice: /usr/share/purple/gfire/scripts/server_detection/4097.pyo warning: File listed twice: /usr/share/purple/gfire/scripts/server_detection/tools
- You can fit the make install -stuff into one line.
- Description uses some odd characters. ’ should be ' and ” should be ".
- License is GPLv3+ not GPLv2+.
- BR: libpurple-devel is redundant, it's already pulled in by pidgin-devel. So is glib2-devel, which is pulled in by pidgin-devel -> gtk2-devel.
- What is the line sed -i -e '/#!/d' %{buildroot}/%{_datadir}/purple/%{srcname}/scripts/server_detection/4097.py for? I assume it removes a shebang in the python file. Please, add a comment!
***
rpmlint output is clean.
MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK - Don't use %{__chmod} for chmod.
MUST: The package must be named according to the Package Naming Guidelines. NEEDSWORK - Should be named pidgin-xfire (yes, my mistake) :)
MUST: The spec file name must match the base package %{name}. OK - Remember to rename this back too.
MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. NEEDSFIX - License is GPLv3+ not GPLv2+.
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK MUST: The package MUST successfully compile and build into binary rpms. OK MUST: The spec file MUST handle locales properly. N/A MUST: Optflags are used and time stamps preserved. OK MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. NEEDSWORK - Should Requires: libpurple explicitly.
MUST: Files only listed once in %files listings. NEEDSWORK - Use %{_datadir}/purple/%{srcname}/ to own everything you should.
MUST: Debuginfo package is complete. OK MUST: Permissions on files must be set properly. OK MUST: Clean section exists. OK MUST: Large documentation files must go in a -doc subpackage. OK MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK MUST: Header files must be in a -devel package. N/A MUST: Static libraries must be in a -static package. N/A MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A MUST: Packages does not contain any .la libtool archives. N/A MUST: Desktop files are installed properly. N/A MUST: No file conflicts with other packages and no general names. OK MUST: Buildroot cleaned before install. OK SHOULD: %{?dist} tag is used in release. OK SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK SHOULD: The package builds in mock. OK
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Andreas Osowski th0br0@mkdir.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: |Review Request: |purple-gfire - Xfire plugin |pidgin-gfire - Xfire plugin |for libpurple |for Pidgin
--- Comment #5 from Andreas Osowski th0br0@mkdir.name 2009-05-20 15:44:18 EDT --- I hope everything is fixed now, I didn't notice upstream's license change. When adding Requires: libpurple, rpmlint says E: explicit-lib-dependency libpurple so I dropped that tag again... RPM is recognizing it by itself. Besides, Requires: pidgin automatically pulls in libpurple.
Spec URL: http://fedora.mkdir.name/packages/pidgin-gfire-0.8.1/pidgin-gfire.spec
SRPM URL: http://fedora.mkdir.name/packages/pidgin-gfire-0.8.1/pidgin-gfire-0.8.1-2.fc...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Jussi Lehtola jussi.lehtola@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #6 from Jussi Lehtola jussi.lehtola@iki.fi 2009-05-20 17:26:06 EDT --- (In reply to comment #5)
I hope everything is fixed now, I didn't notice upstream's license change. When adding Requires: libpurple, rpmlint says E: explicit-lib-dependency libpurple so I dropped that tag again... RPM is recognizing it by itself. Besides, Requires: pidgin automatically pulls in libpurple.
Oh, sometimes you don't have to care about rpmlint's warnings since it does produce some false positives; this is one very common type of them. Due to the dir ownership issue it IS wanted that libpurple is explicitly required; the rpmlint warning can be ignored.
I don't have an xfire account, so I can't test this. From a packaging point of view the package looks good, though.
APPROVED
PS. A final nitpick: I'd use %{_datadir}/pixmaps/pidgin/protocols/*/%{srcname}.png instead of %{_datadir}/pixmaps/pidgin/protocols/*/%{srcname}* since thus one sees straight away what kind of stuff in is in those directories.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
--- Comment #7 from Andreas Osowski th0br0@mkdir.name 2009-05-21 03:36:00 EDT --- All right, I fixed that stuff now.
Spec URL: http://fedora.mkdir.name/packages/pidgin-gfire-0.8.1/pidgin-gfire.spec
SRPM URL: http://fedora.mkdir.name/packages/pidgin-gfire-0.8.1/pidgin-gfire-0.8.1-3.fc...
* Wed May 21 2009 Andreas Osowski th0br0@mkdir.name - 0.8.1-3 - Added Requires: libpurple - Changed the files entry for the pixmaps
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Andreas Osowski th0br0@mkdir.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #8 from Andreas Osowski th0br0@mkdir.name 2009-05-21 03:38:14 EDT --- New Package CVS Request ======================= Package Name: pidgin-gfire Short Description: Xfire plugin for Pidgin Owners: th0br0 Branches: F-10 F-11
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Kevin Fenzi kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
--- Comment #9 from Kevin Fenzi kevin@tummy.com 2009-05-21 19:45:35 EDT --- cvs done.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
--- Comment #10 from Fedora Update System updates@fedoraproject.org 2009-05-22 02:31:59 EDT --- pidgin-gfire-0.8.1-3.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/pidgin-gfire-0.8.1-3.fc10
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
--- Comment #11 from Fedora Update System updates@fedoraproject.org 2009-05-22 02:33:45 EDT --- pidgin-gfire-0.8.1-3.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/pidgin-gfire-0.8.1-3.fc11
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
Andreas Osowski th0br0@mkdir.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
--- Comment #12 from Fedora Update System updates@fedoraproject.org 2009-06-12 17:18:05 EDT --- pidgin-gfire-0.8.2-1.fc10 has been submitted as an update for Fedora 10. http://admin.fedoraproject.org/updates/pidgin-gfire-0.8.2-1.fc10
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=492583
--- Comment #13 from Fedora Update System updates@fedoraproject.org 2009-06-12 17:55:35 EDT --- pidgin-gfire-0.8.2-1.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/pidgin-gfire-0.8.2-1.fc11
package-review@lists.fedoraproject.org