Bug 250538 - Review Request: pidgin-otr - OTR messaging for pidgin (was gaim-otr)
Review Request: pidgin-otr - OTR messaging for pidgin (was gaim-otr)
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-02 01:07 EDT by Paul Wouters
Modified: 2015-02-01 10:41 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-28 00:03:55 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paul Wouters 2007-08-02 01:07:07 EDT
Spec URL: ftp://ftp.xelerance.com/pidgin-otr/pidgin-otr.spec
SRPM URL: ftp://ftp.xelerance.com/pidgin-otr/pidgin-otr-3.1.0-1.fc7.src.rpm
Description: This is a Pidgin plugin which implements Off-the-Record (OTR) Messaging. It is known to work (at least) under the Linux and Windows versions of
Pidgin. 

This package used to be called gaim-otr, also packaged by me. The intention is to obsolete gaim-otr.

pidgin-otr requires libotr-3.1.0, which was only just pushed into devel/
Comment 1 Kevin Fenzi 2007-08-05 16:04:12 EDT
I'd be happy to review this package. Look for a full review in a bit. 
Comment 2 Kevin Fenzi 2007-08-05 17:50:51 EDT
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.
Comment 3 Paul Wouters 2007-08-06 11:08:03 EDT
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
Comment 4 Paul Wouters 2007-08-06 13:22:57 EDT
ok, confirmed it with a koji scratch build.
Comment 5 Kevin Fenzi 2007-08-06 22:29:52 EDT
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. 
Comment 6 Paul Wouters 2007-08-06 22:49:51 EDT
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@xelerance.com
Branches: F-7

(since pidgin is not in <= FC-6)
Comment 7 Paul Wouters 2007-08-06 22:59:08 EDT
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@xelerance.com
Branches: FC-6, F-7
Comment 8 Kevin Fenzi 2007-08-06 23:26:16 EDT
cvs done.
Comment 9 Kevin Fenzi 2007-08-28 00:03:55 EDT
This package appears to have been imported and built. 
Closing this review request now. 
Comment 10 Paul Wouters 2015-01-31 12:31:08 EST
Package Change Request
======================
Package Name: pidgin-otr
New Branches: epel7
Owners: pwouters
Comment 11 Jon Ciesla 2015-02-01 10:41:39 EST
Git done (by process-git-requests).

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