Bug 166713 - Review Request: perl-GnuPG-Interface
Review Request: perl-GnuPG-Interface
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul Howarth
David Lawrence
http://domsch.com/linux/fedora/extras...
:
Depends On:
Blocks: FE-ACCEPT 168070
  Show dependency treegraph
 
Reported: 2005-08-24 16:30 EDT by Matt Domsch
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-10-05 13:44:06 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Matt Domsch 2005-08-24 16:30:12 EDT
Spec Name or Url: http://domsch.com/linux/fedora/extas/perl-GnuPG-Interface/perl-GnuPG-Interface.spec
SRPM Name or Url: http://domsch.com/linux/fedora/extas/perl-GnuPG-Interface/perl-GnuPG-Interface-0.33-0.fc4.src.rpm
Description: Perl interface to GnuPG

GnuPG::Interface is needed by the Debian 'caff - CA Fire and Forget' program, part of debian pgp-tools or signing-party packages.  I'm looking to package caff for FE, and this is a dependency.

I had to create and apply one patch against the list_secret_keys test #2 to match gpg output of the secret key.  The previous test hadn't been touched since 2002 and the results didn't match anymore since gpg changed its output format.
Comment 1 Paul Howarth 2005-08-25 13:30:58 EDT
Review:

- rpmlint clean
- package and spec naming OK
- package meets guidelines
- license is same as perl, matches spec
- spec written in English and is legible
- sources match upstream
- builds OK on FC4 and in mock for rawhide (i386)
- buildreqs OK
- no locales, libraries, subpackages or pkgconfigs to worry about
- not relocatable
- no directory ownership or permissions issues
- no duplicate files
- %clean section present and correct
- macro usage consistent
- code, not content
- no large docs
- docs don't affect runtime
- no scriptlets

Nitpick:

- %check output can be cleaned somewhat by adding "chmod 700 test" before "make
test"
- any idea what's happening here?

+ make test
PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0,
'blib/lib', 'blib/arch')" t/*.t
t/clearsign................ok
t/decrypt..................ok
t/detach_sign..............ok
t/encrypt..................ok 1/3unknown record type tru at
blib/lib/GnuPG/Interface.pm (autosplit into
blib/lib/auto/GnuPG/Interface/get_keys.al) line 548, <GEN27> line 1.
t/encrypt..................ok
t/encrypt_symmetrically....ok
t/export_keys..............ok
t/Fingerprint..............ok
t/get_public_keys..........unknown record type tru at
blib/lib/GnuPG/Interface.pm (autosplit into
blib/lib/auto/GnuPG/Interface/get_keys.al) line 548, <GEN14> line 1.
t/get_public_keys..........ok
t/get_secret_keys..........ok
t/import_keys..............ok
t/Interface................ok
t/list_public_keys.........ok
t/list_secret_keys.........ok
t/list_sigs................ok
t/passphrase_handling......ok
t/sign.....................ok
t/sign_and_encrypt.........ok
t/UserId...................ok
t/verify...................ok
t/wrap_call................ok

- for an FC3 build you don't need the patch, so you could maintain a different
  spec in the FC3 branch, or conditionally apply the patch:

%if 0%{?fedora} > 3
%patch0 -p1
%endif

- license text not included in package; suggest adding to %setup:
perldoc perlgpl > GPL
perldoc perlartistic > Artistic
  and to %files:
%doc GPL Artistic

- although "BuildReq: gpg" works, might "gnupg" be better?

Comment 2 Matt Domsch 2005-08-26 00:48:38 EDT
Fixed everything noted by Paul.  Found a patch from Mail::GPG to handle the
'tru' (trust?) field now reported by gnupg-1.4.  Apply the test-fix patch only
for gnupg > 1.2.

Spec Name or Url:
http://domsch.com/linux/fedora/extras/perl-GnuPG-Interface/perl-GnuPG-Interface.spec
SRPM Name or Url:
http://domsch.com/linux/fedora/extras/perl-GnuPG-Interface/perl-GnuPG-Interface-0.33-1.src.rpm

Thanks for your insight, please review again.
Comment 3 Ralf Corsepius 2005-08-26 01:39:49 EDT
I dislike the magic being used to apply the patch conditionally.
As I don't have FC3 anymore, I can't easily test, so let me ask: Does Patch1
cause any harm/failures/malfunctions on FC3?

- If no, then I'd propose you to apply the patch unconditionally.

- If yes, then I'd propose to replace the gpgversion detection magic by using 2
different versions of the spec file (one for FC3 and one for FC4).

Missing:
Requires: gpg
Comment 4 Paul Howarth 2005-08-26 02:15:19 EDT
(In reply to comment #3)
> I dislike the magic being used to apply the patch conditionally.
> As I don't have FC3 anymore, I can't easily test, so let me ask: Does Patch1
> cause any harm/failures/malfunctions on FC3?
> 
> - If no, then I'd propose you to apply the patch unconditionally.
> 
> - If yes, then I'd propose to replace the gpgversion detection magic by using 2
> different versions of the spec file (one for FC3 and one for FC4).

"make test" fails on FC3 if the patch is applied because the output format from
gpg is not recognised.

Regarding version detection magic, I'm personally in favour of it (at least if
it's fairly clear what's going on - you might want to add a comment about it in
the spec) because I can maintain a single spec file for all branches (currently
devel, FC-4, and Fc-3), which makes future changes easier to do. Others, like
Ralf, clearly prefer to maintain separate spec files when necessary in each
branch, which will be clearer to read and less error-prone if there's ever an
issue with the "magic". But I think it's the packager's choice, not a policy issue.

> Missing:
> Requires: gpg

Yes, this is needed (or Requires: gnupg).
Comment 5 Ralf Corsepius 2005-08-26 02:38:15 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > I dislike the magic being used to apply the patch conditionally.

> Regarding version detection magic, I'm personally in favour of it ( Others, like
> Ralf, clearly prefer to maintain separate spec files when necessary in each
> branch, which will be clearer to read and less error-prone if there's ever an
> issue with the "magic".

The point is: This magic is only necessary to cater obsolete versions of gpg and
versions of Fedora to be EOL'ed soon. It thereby unnecessarily polutes upstream
Fedora rpm.specs.
Comment 6 Matt Domsch 2005-08-29 00:18:56 EDT
I think such selective patch magic should be allowed at the packager's
preference.  However, given there would be a need for exactly 2 spec files at
this point (FC4 and higher, and FC3 and below), I've removed the magic for the
package to go into the devel trunk, and the patch can be removed in the FC3
branch if/when one is made.

Also, tested with gnupg2, and it works (provided you either symlink gpg to gpg2,
or set GnuPG::Interface::call => gpg2.  As both gnupg and gnupg2 Provide: gpg,
I've changed the BR and Requires lines back to requiring gpg.

Spec Name or Url:
http://domsch.com/linux/fedora/extras/perl-GnuPG-Interface/perl-GnuPG-Interface.spec
SRPM Name or Url:
http://domsch.com/linux/fedora/extras/perl-GnuPG-Interface/perl-GnuPG-Interface-0.33-2.src.rpm
Comment 7 Paul Howarth 2005-08-29 07:10:33 EDT
Please use "perldoc -t" rather than plain "perldoc" - I forgot that in Comment #1.

You might also want to add "%{?_smp_mflags}" as an option to make in %build.

You can address these in CVS.

APPROVED.
Comment 8 Ralf Corsepius 2005-09-12 13:29:52 EDT
Ping? It's 2 weeks since this package had been approved.
Comment 9 Matt Domsch 2005-09-12 13:44:11 EDT
I was hoping to have legal approval to become a contributor by now.  I'll ask 
spot to commit it to CVS on my behalf.

spot, I fixed the last nits Paul noticed, and put the srpm here, if you would 
be so kind:

http://domsch.com/linux/fedora/extras/perl-GnuPG-Interface/perl-GnuPG-
Interface-0.33-3.fc4.src.rpm

Comment 10 Tom "spot" Callaway 2005-10-05 13:44:06 EDT
This should be built ok now.

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