[Bug 742189] Review Request: rubygem-webmock - Library for stubbing HTTP requests in Ruby

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=742189

James Laska <jlaska at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
               Flag|fedora-review?              |fedora-review+

--- Comment #3 from James Laska <jlaska at redhat.com> 2011-10-03 10:16:23 EDT ---
(In reply to comment #2)
> New Spec / SPRM:
> 
> 
> Spec URL: http://mo.morsi.org/files/rpms/rubygem-webmock.spec
> SRPM URL:
> http://mo.morsi.org/files/rpms/rubygem-webmock-1.7.6-2.fc15.src.rpm

$ rpmlint rubygem-webmock.spec rubygem-webmock-1.7.6-2.fc15.src.rpm
rubygem-webmock-1.7.6-2.fc15.noarch.rpm
rubygem-webmock-doc-1.7.6-2.fc15.noarch.rpm | grep -v "unexpanded-macro"
3 packages and 1 specfiles checked; 0 errors, 60 warnings.

rpmlint looks good, excluding the 'unexpanded-macro' warning.

> > rubygem-webmock.noarch: W: doc-file-dependency
> > /usr/lib/ruby/gems/1.8/gems/webmock-1.7.6/Rakefile /usr/bin/env
> > 
> > From rpmlint source ... '''An included file marked as %doc creates a possible
> > additional dependency in the package.  Usually, this is not wanted and may be
> > caused by eg. example scripts with executable bits set included in the
> > package's documentation.'''
> 
> Fixed, unmarked Rakefile as doc. Also changed /usr/bin/env rake to
> /usr/bin/rake


Fix confirmed in latest spec/packages.

> > rubygem-webmock-doc.noarch: W: unexpanded-macro
> > /usr/lib/ruby/gems/1.8/doc/webmock-1.7.6/ri/WebMock/RequestSignature/eql%3f-i.yaml
> > %3f
> > <snip>
> > 
> > From rpmlint source ... '''This package contains a file whose path contains
> > something that looks like an unexpanded macro; this is often the sign of a
> > misspelling. Please check your specfile.'''
> > 
> 
> These can be ignored, they occur in any rubygem that ships ri documentation

Agreed, just a warning and can be ignored.

> > > [ FAIL ] MUST: The License field in the package spec file must match the 
> > >          actual license
> > 
> > The upstream LICENSE file seems to indicate MIT, does this need to be updated?
> >
> 
> Fixed

Fix confirmed in latest spec/packages.

> > Note, there are too many files listed as %doc.  For example, the Rakefile
> > probably shouldn't be a %doc.  Maybe the same with other source code?
> > 
> > %exclude %{geminstdir}/Rakefile
> >
> 
> Unmarked Rakefile as doc, the others are appropriately marked as doc (include
> tests and such)

Gotcha, thanks for explaining.

> > > [ FAIL ] MUST: Permissions on files must be set properly. Executables should 
> > >          be set with executable permissions, for example. Every %files section
> > >          must include a %defattr(...) line.
> 
> This is no longer needed
> 
> http://fedoraproject.org/wiki/Packaging/Guidelines#File_Permissions

Thanks, I need to update my checklist.

> > > [ FAIL ] MUST: At the beginning of %install, each package MUST run rm -rf
> > >          %{buildroot} (or $RPM_BUILD_ROOT).
> > 
> > Fixed, see http://fpaste.org/GNSM/
> >
> 
> This is no longer needed / should not be present
> 
> http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
> 
> 
> 
> > > [ WARN ] - The Ruby library files in a pure Ruby package must be placed into Config::CONFIG["sitelibdir"] . The specfile must get that path using %{!?ruby_sitelib: %global ruby_sitelib %(ruby -rrbconfig -e 'puts Config::CONFIG["sitelibdir"] ')}
> > 
> > The specfile is using a different method for locating /usr/lib/ruby/*.  Should
> > it be using %{ruby_sitelib} instead?  Or does this not apply since this is
> > providing a rubygem?
> > 
> 
> Yes according to the Fedora gem packaging guidelines, we define gemdir and
> geminstdir correctly
> 
> 
> http://fedoraproject.org/wiki/Packaging:Ruby#Ruby_Gems

Gotcha

> > > [ WARN ] - The %prep and %build sections of the specfile should be empty.
> > 
> > %build is empty, %prep is not ... I've adjusted per the ruby guidelines
> > slightly.  However this is a *should* requirement, not a *must*.  
> > 
> 
> I would prefer to leave it as it is. The reason being if we ever have to patch
> the gem, the gem install needs to occur in the %prep section before we can run
> the %patch commands there as well.

No objections, this shows up as a "should" requirement, and is entirely up to
the maintainer in my opinion.

> > > [ FAIL ] - The install should be performed with the command 'gem install --local --install-dir %{buildroot}%{gemdir} --force %{SOURCE0}'
> > 
> > This command is currently used in the %prep.  I've adjusted slighty to
> > accommodate the *should* requirement.  Feel free to use if desired.
> > 
> > http://fpaste.org/GNSM/
> >
> Again would prefer to leave as is unless this is a major blocker.

Understood.  Makes sense given your comments about applying patches.

> Thank you greatly for the review!

Anytime.  

>From what I can tell, everything else looks good with the packages posted in
comment#2.  I approve this review request.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.