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@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flag|fedora-review? |fedora-review+ --- Comment #3 from James Laska <jlaska@xxxxxxxxxx> 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review