Bug 462818 - Review Request: perl-Net-SMTP-SSL - SSL support for Net::SMTP
Summary: Review Request: perl-Net-SMTP-SSL - SSL support for Net::SMTP
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paul Howarth
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 467748 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-09-19 03:51 UTC by Dan Nicholson
Modified: 2010-06-26 16:49 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-11-07 02:52:26 UTC
Type: ---
Embargoed:
paul: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Dan Nicholson 2008-09-19 03:51:47 UTC
Spec URL: http://people.freedesktop.org/~dbn/rpm/perl-Net-SMTP-SSL.spec
SRPM URL: http://people.freedesktop.org/~dbn/rpm/perl-Net-SMTP-SSL-1.01-1.fc9.src.rpm
Description: This is a simple perl module (20 LOC) to add SSL support to Net::SMTP. It just substitutes IO::Socket::SSL for the network operations.

This is used by git-send-email for SSL/TLS support when using an SMTP server. It's very nice if you don't have a local mail server and depend on an external mail service like gmail. See here for example:

http://kerneltrap.org/mailarchive/git/2008/5/12/1793204

This is my first package for Fedora, so I need a sponsor. However, I'm very familiar with Linux packaging and rpm in particular. See the misguided time spent when I was rolling my own distro:

http://gitweb.dwcab.com/?p=pound.git

Thanks.

Comment 1 Paul Howarth 2008-10-13 22:59:40 UTC
Looks good. From a first glance, the only thing that sticks out as needing changing is that your %files list needs to have:

%{perl_vendorlib}/Net/

rather than:

%{perl_vendorlib}/Net/SMTP/SSL.pm

so that directories %{perl_vendorlib}/Net/ and %{perl_vendorlib}/Net/SMTP/ are owned by the package.

Formal review to follow.

Comment 2 Dan Nicholson 2008-10-13 23:19:49 UTC
Thanks for the review.

As far as the comment, %{perl_vendorlib}/Net/ is owned by perl-libwww-perl, but /Net/SMTP/ is not owned (thought it was owned by perl-Net-SMTP, but Net::SMTP is part of perl). Would this cause any conflicts with perl-libwww-perl (I forget how rpm-4.4 handles directory ownership)?

Comment 3 Paul Howarth 2008-10-13 23:28:49 UTC
perl-libwww-perl isn't a dependency of this package and so we can't assume that it's present. It's very common for perl module directories to have multiple ownership as many unrelated modules share directory paths, e.g.:

$ rpm -qf /usr/lib/perl5/vendor_perl/5.10.0/Mail/
perl-MailTools-2.03-1.fc9.noarch
spamassassin-3.2.5-1.fc9.i386
perl-Mail-DKIM-0.32-3.fc9.noarch
perl-Mail-SPF-2.005-2.fc9.noarch

Comment 4 Paul Howarth 2008-10-13 23:31:02 UTC
Review:

- rpmlint clean
- package meets naming guidelines
- spec file name matches package name
- package meets guidelines
- license is same as perl
- license tag is correct
- no license file in upstream tarball to include (though included README states terms)
- spec file written in English and is legible
- source matches upstream (size, timestamp, md5sum ba039288ebf7a343feecacd374da8c1a)
- latest upstream version packaged
- no worrying-looking bugs on CPAN
- package builds on on Rawhide x86_64 in mock
- buildreqs OK
- no locales, shared or static libraries, or devel files to worry about
- package is not designed to be relocatable
- no duplicate files
- permissions sane
- %clean section present and correct in spec file
- macro usage is consistent
- code, not content
- no large docs, in fact no large anything
- docs don't affect runtime
- no subpackages
- not a GUI app, no desktop file needed
- %install section starts by clearing buildroot
- all filenames are valid UTF-8
- no scriptlets


NEEDSWORK:

- directories %{perl_vendorlib}/Net/ and %{perl_vendorlib}/Net/SMTP/ need to
  be owned by the package

Fix that and I'll approve this and sponsor you.

Comment 5 Dan Nicholson 2008-10-13 23:56:54 UTC
Alright, I decided to just be explicit with the directories instead of globbing in any unwanted files. Have another look:

http://people.freedesktop.org/~dbn/rpm/perl-Net-SMTP-SSL.spec
http://people.freedesktop.org/~dbn/rpm/perl-Net-SMTP-SSL-1.01-1.fc9.src.rpm

Thanks again for the review.

Comment 6 Paul Howarth 2008-10-14 09:25:04 UTC
OK, that looks fine.

A couple of minor comments:

 1. Please bump the release number of the package every time you make a change that anyone else (e.g. a reviewer) will see; this removes a possible source of confusion about which version of a package any particular comment applies to, and once you're building packages in Fedora, will ensure that packages you build and subsequently release are consistent with any that people may build locally from CVS.

 2. I find it aids readability to append a "/" to directories included in %files lists, just to emphasize that they are directories (which in many cases will include further files, though that's not the case here), e.g.
dir %{perl_vendorlib}/Net/

APPROVED.

Let me know your account name in the Fedora Account System and I'll sponsor you for membership of the Fedora Packager CVS Commit Group.

Comment 7 Dan Nicholson 2008-10-14 16:21:37 UTC
(In reply to comment #6)
> OK, that looks fine.
> 
> A couple of minor comments:
> 
>  1. Please bump the release number of the package every time you make a change
> that anyone else (e.g. a reviewer) will see; this removes a possible source of
> confusion about which version of a package any particular comment applies to,
> and once you're building packages in Fedora, will ensure that packages you
> build and subsequently release are consistent with any that people may build
> locally from CVS.

Certainly.

>  2. I find it aids readability to append a "/" to directories included in
> %files lists, just to emphasize that they are directories (which in many cases
> will include further files, though that's not the case here), e.g.
> dir %{perl_vendorlib}/Net/

Fair enough - one character for better readability can't hurt.

> APPROVED.
> 
> Let me know your account name in the Fedora Account System and I'll sponsor you
> for membership of the Fedora Packager CVS Commit Group.

Account name is "dbn". Hopefully I have all that stuff set up appropriately. Please let me know if there are any hiccups with the account.

Thanks.

Comment 8 Paul Howarth 2008-10-14 18:56:51 UTC
I think you'll need to apply for membership of the packager group in the account system and then I can approve it.

Comment 9 Dan Nicholson 2008-10-14 19:15:56 UTC
I knew I'd forgotten something. Request should be there now.

Comment 10 Paul Howarth 2008-10-14 19:30:18 UTC
OK you're sponsored now. When you make the CVS Admin request for adding this package, it's conventional for perl modules to add "perl-sig" as InitialCC: - this will mean that any bug reports get copied to fedora-perl-devel-list, where list members may be able to assist with fixes.

Comment 11 Dan Nicholson 2008-10-15 03:35:58 UTC
New Package CVS Request
=======================
Package Name: perl-Net-SMTP-SSL
Short Description: SSL support for Net::SMTP
Owners: dbn
Branches: F-9 F-10
InitialCC: perl-sig

Comment 12 Paul Howarth 2008-10-15 06:39:31 UTC
You don't actually need to request an F-10 branch here; if you omit it you'll still get the devel branch for Rawhide that will eventually become F-10. There will be a mass branching of all packages shortly before F-10 goes gold to create an F-10 branch from what's currently in devel. The availability of early F-10 branching (which is what you've requested here) is targeted at packages for which major changes will be happening in the F-11 development period, so that those changes can get underway now in CVS without affecting what will be going into F-10.

Having said that, it's not going to do any harm to have the F-10 branch earlier, and this package is unlikely to change before the mass branching happens. Just so you know...

Comment 13 Huzaifa S. Sidhpurwala 2008-10-15 10:09:31 UTC
cvs done

Comment 14 Fedora Update System 2008-10-15 20:18:09 UTC
perl-Net-SMTP-SSL-1.01-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/perl-Net-SMTP-SSL-1.01-1.fc9

Comment 15 Dan Nicholson 2008-10-15 20:20:31 UTC
Ah, too bad about the F-10 branch. I thought the branching had already happened
with the F-10 beta, but I should have looked more closely. Anyway, I've
submitted builds for F-9, F-10 and F-11.

https://koji.fedoraproject.org/koji/buildinfo?buildID=66466
https://koji.fedoraproject.org/koji/buildinfo?buildID=66463
https://koji.fedoraproject.org/koji/buildinfo?buildID=66462

Comment 16 Chris Weyl 2008-10-21 01:57:42 UTC
*** Bug 467748 has been marked as a duplicate of this bug. ***

Comment 17 Fedora Update System 2008-11-07 02:52:23 UTC
perl-Net-SMTP-SSL-1.01-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 18 Jeroen van Meeuwen 2010-06-25 04:34:56 UTC
Package Change Request
======================
Package Name: perl-Net-SMTP-SSL
New Branches: EL-5 EL-6
Owners: kanarip

Comment 19 Jason Tibbitts 2010-06-26 06:48:20 UTC
Can we get an ack from the current Fedora maintainer?

Comment 20 Dan Nicholson 2010-06-26 07:33:08 UTC
I don't have access to EL, but this is a completely benign module. Seems fine to me as long as the dependencies are available.

Comment 21 Jason Tibbitts 2010-06-26 16:16:43 UTC
CVS done (by process-cvs-requests.py).

Comment 22 Jason Tibbitts 2010-06-26 16:49:42 UTC
That went slightly awry; sorry.  I've fixed it up.


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