Bug 250538

I'd be happy to review this package. Look for a full review in a bit. 

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
See below - License
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
f4ca8bc228069616d4c8fa9288af6bb5  pidgin-otr-3.1.0.tar.gz
f4ca8bc228069616d4c8fa9288af6bb5  pidgin-otr-3.1.0.tar.gz.1
See below - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install
OK - .la files are removed.
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have sane scriptlets.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Per the new License guidelines I think the License: tag here should be
GPLv2 (GPL version 2 only).

2. Your Provides and Obsoletes for gaim-otr don't look right to me.
See:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-3cfc1ea19d28975faad9d56f70a6ae55661d3c3d

I think you should have:

Provides: gaim-otr = 3.0.1-0.7.20060712cvs.fc7
Obsoletes: gaim-otr < 3.0.1-0.6.20060712cvs.fc7

3. Doesn't build here under mock. I get:
checking for XML::Parser... configure: error: XML::Parser perl module is
required for intltool
error: Bad exit status from /var/tmp/rpm-tmp.1879 (%build)

If I add a "BuildRequires: perl(XML::Parser)" it gets further and then blows up
with:

make[2]: Entering directory `/builddir/build/BUILD/pidgin-otr-3.1.0/po'
file=`echo es | sed 's,.*/,,'`.gmo \
          && rm -f $file &&  -o $file es.po
file=`echo fr | sed 's,.*/,,'`.gmo \
          && rm -f $file &&  -o $file fr.po
/bin/sh: line 1: -o: command not found
make[2]: *** [es.gmo] Error 127
make[2]: *** Waiting for unfinished jobs....
/bin/sh: line 1: -o: command not found
make[2]: *** Waiting for unfinished jobs....
/bin/sh: line 1: -o: command not found
make[2]: *** [fr.gmo] Error 127
make[2]: Leaving directory `/builddir/build/BUILD/pidgin-otr-3.1.0/po'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/builddir/build/BUILD/pidgin-otr-3.1.0'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.47112 (%build)

Needs to also have "BuildRequires: gettext"

That gets it building.

4. rpmlint says:

W: pidgin-otr invalid-license GPL
W: pidgin-otr invalid-license GPL
W: pidgin-otr unversioned-explicit-obsoletes gaim-otr
W: pidgin-otr-debuginfo invalid-license GPL

Would be fixed by the issue 1 and issue 2 being fixed.

Thanks for the review!

I have fixed the license, the unversioned obsolete, and the buildrequires

How did you build pidgin-otr in mock, with libotr not being available? Or did
you run mock on a "devel" system? I don't have a devel system, so I couldn't run
it easilly with mock, and I'm waiting for pidgin-otr to be available before
bumping both libotr and pidgin-otr into f7 and fc6.

Spec URL: ftp://ftp.xelerance.com/pidgin-otr/pidgin-otr.spec
SRPM URL: ftp://ftp.xelerance.com/pidgin-otr/pidgin-otr-3.1.0-2.fc7.src.rpm

ok, confirmed it with a koji scratch build.

Yes, I build in mock for devel by default. I also often build f7 and fc6. 
In this case I just looked at the devel output. 

Your Provides is still using the version of this package instead of just
slightly larger than the last gaim-otr that it's replacing. Thats not a big deal
in this 
case, but the guidelines have that rule so that if the package you are replacing
ever did come back you could just add it back and it would be ok, instead of 
needing to be a higher version than this package. Since gaim-otr is unlikely
ever to return, that can be ignored in this case.  

I see no further blockers, so this package is APPROVED. 
Don't forget to close this once it's been imported and built. 

Also, do consider doing some reviews of other waiting packages to help
spread out the review load. 

Actually, the latest release of gaim-otr is 3.0.1-0.6.20060712cvs
So as per
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-3cfc1ea19d28975faad9d56f70a6ae55661d3c3d
I bumped the release one to 3.0.1-0.7.20060712cvs for the Obsoletes:

Thanks for the review. I will pick up a few other reviews in return.

New Package CVS Request
=======================
Package Name: pidgin-otr
Short Description: OTR messaging for pidgin (was gaim-otr)
Owners: paul
Branches: F-7

(since pidgin is not in <= FC-6)

Actually there is pidgin for FC-6, so I'm asking for a branche there too.

New Package CVS Request
=======================
Package Name: pidgin-otr
Short Description: OTR messaging for pidgin (was gaim-otr)
Owners: paul
Branches: FC-6, F-7

cvs done.

This package appears to have been imported and built. 
Closing this review request now. 

Package Change Request
======================
Package Name: pidgin-otr
New Branches: epel7
Owners: pwouters

Git done (by process-git-requests).