[Bug 492583] Review Request: purple-gfire - Xfire plugin for libpurple

[bugzilla at redhat.com]

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 at 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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.