Bug 476360 - Review Request: rubygem-tlsmail - This library enables pop or smtp via ssl/tls
Summary: Review Request: rubygem-tlsmail - This library enables pop or smtp via ssl/tls
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-12-13 14:49 UTC by Darryl L. Pierce
Modified: 2015-06-22 00:06 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-01-25 07:57:10 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Darryl L. Pierce 2008-12-13 14:49:05 UTC
Spec URL: http://mcpierce.fedorapeople.org/rubygem-tlsmail.spec
SRPM URL: http://mcpierce.fedorapeople.org/rubygem-tlsmail-0.0.1-1.fc10.src.rpm
Description: This library enables pop or smtp via ssl/tls by dynamically replacing these classes to these in ruby 1.9.

Comment 1 Mamoru TASAKA 2008-12-26 07:56:22 UTC
I will review this. Instead I would appreciate it if you
would review my review request for rubygem-nokogiri (bug 477883)

Comment 2 Mamoru TASAKA 2008-12-26 14:55:11 UTC
For 0.0.1-1:

* Documents
  - Would you check if Rakefile or Manifest.txt really needed?
  - README.txt should be marked as %doc.

* Possibly duplicate/conflicting files with system ones
  - This rpm contains net/pop.rb, net/smtp.rb. These are
    already included in ruby-libs rpm (however the files
    in ruby-libs seems older than those in this rpm).
    Can these files safely installed in parallel?

* CRLF line terminators
  - Some ruby scripts in this rpm have CRLF line terminators
    (although my rpmlint could not detect them...)

Comment 3 Mamoru TASAKA 2008-12-26 15:02:13 UTC
Oops..

* ruby abi requirement
  - Also ruby(abi) requirement is needed.
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines

Comment 4 Darryl L. Pierce 2009-01-05 16:00:37 UTC
(In reply to comment #2)
> * Possibly duplicate/conflicting files with system ones
>   - This rpm contains net/pop.rb, net/smtp.rb. These are
>     already included in ruby-libs rpm (however the files
>     in ruby-libs seems older than those in this rpm).
>     Can these files safely installed in parallel?

That is the point of this gem - it's providing alternatives to those classes in Ruby. But it only overrides them if you explicitly require the tlsmail gem in your code.

Comment 5 Darryl L. Pierce 2009-01-05 16:21:45 UTC
Updated with requested changes:
Spec URL: http://mcpierce.fedorapeople.org/rpms/rubygem-tlsmail.spec
SRPM URL: http://mcpierce.fedorapeople.org/rpms/rubygem-tlsmail-0.0.1-2.fc10.src.rpm

Comment 6 Mamoru TASAKA 2009-01-06 16:33:39 UTC
Well, there are still many files in binary rpm which have
CRLF line terminators as

$ rpm -ql rubygem-tlsmail | xargs file | grep CRLF

shows (although rpmlint does not seem to be able to detect these)

Comment 7 Darryl L. Pierce 2009-01-15 15:05:46 UTC
(In reply to comment #6)
> Well, there are still many files in binary rpm which have
> CRLF line terminators as
> 
> $ rpm -ql rubygem-tlsmail | xargs file | grep CRLF
> 
> shows (although rpmlint does not seem to be able to detect these)

Here's an update with those CRLF issues fixed:

Spec URL: http://mcpierce.fedorapeople.org/rpms/rubygem-tlsmail.spec
SRPM URL: http://mcpierce.fedorapeople.org/rpms/rubygem-tlsmail-0.0.1-3.fc10.src.rpm

Comment 8 Mamoru TASAKA 2009-01-15 17:33:10 UTC
Well, sorry for not pointing out before, however:

- now you defined %gemdocsdir, it is better that you use this macro
  also on %files

- Also you use %{gemdir}/gems/%{gemname}-%{version}/ in %files, while
  %{geminstdir} is also used in %files.

- Now build.log warns about:
-----------------------------------------------------------
warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/tlsmail-0.0.1/README.txt
-----------------------------------------------------------

- In %changelog, please use %% instead of single % (i.e. %%doc, for
  example) to aviod macros from being expanded.

Comment 9 Darryl L. Pierce 2009-01-15 20:09:41 UTC
(In reply to comment #8)
> Well, sorry for not pointing out before, however:
> 
> - now you defined %gemdocsdir, it is better that you use this macro
>   also on %files
> 
> - Also you use %{gemdir}/gems/%{gemname}-%{version}/ in %files, while
>   %{geminstdir} is also used in %files.
> 
> - Now build.log warns about:
> -----------------------------------------------------------
> warning: File listed twice:
> /usr/lib/ruby/gems/1.8/gems/tlsmail-0.0.1/README.txt
> -----------------------------------------------------------
> 
> - In %changelog, please use %% instead of single % (i.e. %%doc, for
>   example) to aviod macros from being expanded.

The above points have been fixed:

SPEC URL: http://mcpierce.fedorapeople.org/rpms/rubygem-tlsmail.spec
SRPM URL: http://mcpierce.fedorapeople.org/rpms/rubygem-tlsmail-0.0.1-4.fc10.src.rpm

Comment 10 Mamoru TASAKA 2009-01-16 17:01:33 UTC
It seems you reversed the following?

(In reply to comment #2)
> For 0.0.1-1:
> 
> * Documents
>   - README.txt should be marked as %doc.

What I mean is that %files should be
------------------------------------
%dir %{geminstdir}
%doc %{geminstdir}/[A-Z]*
%{geminstdir}/*/
------------------------------------
or so.

Other things are okay.
-------------------------------------------------------------
   This package (rubygem-tlsmail) is APPROVED by mtasaka
-------------------------------------------------------------

Comment 11 Darryl L. Pierce 2009-01-16 18:04:12 UTC
(In reply to comment #10)
> It seems you reversed the following?
> 
> (In reply to comment #2)
> > For 0.0.1-1:
> > 
> > * Documents
> >   - README.txt should be marked as %doc.
> 
> What I mean is that %files should be
> ------------------------------------
> %dir %{geminstdir}
> %doc %{geminstdir}/[A-Z]*
> %{geminstdir}/*/
> ------------------------------------
> or so.
> 
> Other things are okay.
> -------------------------------------------------------------
>    This package (rubygem-tlsmail) is APPROVED by mtasaka
> -------------------------------------------------------------

Thanks. I've changed the RPM per your final suggestions. Thank you, and I'll begin your review tomorro.

Comment 12 Darryl L. Pierce 2009-01-16 18:07:33 UTC
New Package CVS Request
=======================
Package Name: rubygem-tlsmail
Short Description: This library enables pop or smtp via ssl/tls
Owners: mcpierce
Branches: F-9 F-10
InitialCC:

Comment 13 Kevin Fenzi 2009-01-17 03:18:34 UTC
cvs done.

Comment 14 Mamoru TASAKA 2009-01-25 07:57:10 UTC
Closing.


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