Bug 492583 - Review Request: pidgin-gfire - Xfire plugin for Pidgin
Summary: Review Request: pidgin-gfire - Xfire plugin for Pidgin
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-03-27 14:56 UTC by Andreas Osowski
Modified: 2009-06-12 21:55 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-05-22 06:35:07 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Andreas Osowski 2009-03-27 14:56:51 UTC
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.fc10.src.rpm

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

Comment 1 Susi Lehtola 2009-03-28 09:34:08 UTC
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)

Comment 2 Andreas Osowski 2009-03-28 10:09:41 UTC
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.fc10.src.rpm

* Sat Mar 28 2009 Andreas Osowski <th0br0> - 0.7.1-2
- Renamed to purple-gfire
- Fixed indentation
- Changed description

rpmlint now remains quiet.

Comment 3 Andreas Osowski 2009-05-20 16:46:31 UTC
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.fc10.src.rpm

* Wed May 20 2009 Andreas Osowski <th0br0> - 0.8.1-1
- New upstream version

Comment 4 Susi Lehtola 2009-05-20 18:23:32 UTC
(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

Comment 5 Andreas Osowski 2009-05-20 19:44:18 UTC
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.fc10.src.rpm

Comment 6 Susi Lehtola 2009-05-20 21:26:06 UTC
(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.

Comment 7 Andreas Osowski 2009-05-21 07:36:00 UTC
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.fc10.src.rpm

* Wed May 21 2009 Andreas Osowski <th0br0> - 0.8.1-3
- Added Requires: libpurple
- Changed the files entry for the pixmaps

Comment 8 Andreas Osowski 2009-05-21 07:38:14 UTC
New Package CVS Request
=======================
Package Name: pidgin-gfire
Short Description: Xfire plugin for Pidgin
Owners: th0br0
Branches: F-10 F-11

Comment 9 Kevin Fenzi 2009-05-21 23:45:35 UTC
cvs done.

Comment 10 Fedora Update System 2009-05-22 06:31:59 UTC
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

Comment 11 Fedora Update System 2009-05-22 06:33:45 UTC
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

Comment 12 Fedora Update System 2009-06-12 21:18:05 UTC
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

Comment 13 Fedora Update System 2009-06-12 21:55:35 UTC
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


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