Bug 771233

This package is need for updating Sinatra to 1.3.2.

I'm taking this one.

- The package FTBTS [1]. Please do a Koji build before every review to see if it passes.
-- You need to BR: rubygem(rspec-core) instead of rubygem(rspec).
-- You also need to BR: rubygem(rack-test) for running the specs.
- Could you specify what is the advantage of running the specs with "ruby -S rspec spec", when "rspec spec" suffices? This is not a blocker, but why not make things as simple as possible?
- Do you intend to place the package into EPEL, too? If not, please remove the unnecessary BuildRoot tag (see [2] for more info).
- Please be consistent in usage of macros for shell commands and the commands themselves. For example, you use both "%{__mkdir_p}" macro and "mkdir -p" command. So decide whether you want to use macros or commands and don't mix the two.
- Consider excluding the cached gem, as it is not an unnecessary payload, not needed for RPM package.
- Mark %{geminstdir}/License with %doc.
- Move %{geminstdir}/README.md to the -doc subpackage, if it is not needed for runtime (which I believe it isn't) and mark it with %doc.
- Also, mark %{gemdir}/doc/%{gemname}-%{version} with %doc.


[1] http://koji.fedoraproject.org/koji/taskinfo?taskID=3615389
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

(In reply to comment #3)

Hi Bohuslav

First thanks for reviewing this package!

> - The package FTBTS [1]. Please do a Koji build before every review to see if
> it passes.
> -- You need to BR: rubygem(rspec-core) instead of rubygem(rspec).
> -- You also need to BR: rubygem(rack-test) for running the specs.

All done.

> - Could you specify what is the advantage of running the specs with "ruby -S
> rspec spec", when "rspec spec" suffices? This is not a blocker, but why not
> make things as simple as possible?

We used it in thin package. However this form sounds better and shorter for me.
Fixed in -2.

> - Do you intend to place the package into EPEL, too? If not, please remove the
> unnecessary BuildRoot tag (see [2] for more info).

This package will be imported to EPEL. I'll exclude BuildRoot from Fedora packages
before import.

> - Please be consistent in usage of macros for shell commands and the commands
> themselves. For example, you use both "%{__mkdir_p}" macro and "mkdir -p"
> command. So decide whether you want to use macros or commands and don't mix the
> two.

Done.

> - Consider excluding the cached gem, as it is not an unnecessary payload, not
> needed for RPM package.

Done.

> - Mark %{geminstdir}/License with %doc.

Done,

> - Move %{geminstdir}/README.md to the -doc subpackage, if it is not needed for
> runtime (which I believe it isn't) and mark it with %doc.
> - Also, mark %{gemdir}/doc/%{gemname}-%{version} with %doc.

Actually I don't think marking -doc subpackage files with %doc is necessary.
Could you point me to a guideline where this is required?

=====================================================

koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3615827

rpmlint:
rubygem-rack-protection.noarch: I: enchant-dictionary-not-found en_US
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

* Mon Jan 03 2012 Michal Fojtik <mfojtik> - 1.2.0-2
- Fixed BR
- Marked documentation file with doc tag
- Changed the way how to run rspec tests

Revision -2:

Spec: http://omicron.mifo.sk/rubygem-rack-protection.spec
SRPM: http://omicron.mifo.sk/rubygem-rack-protection-1.2.0-1.fc14.src.rpm

> > - Move %{geminstdir}/README.md to the -doc subpackage, if it is not needed for
> > runtime (which I believe it isn't) and mark it with %doc.
> > - Also, mark %{gemdir}/doc/%{gemname}-%{version} with %doc.
> 
> Actually I don't think marking -doc subpackage files with %doc is necessary.
> Could you point me to a guideline where this is required?

I believe that there is no specific guideline for this. But if you take a look at it from the logical point of view, you have two types of files in  your -doc subpackage:
- The Rakefile and spec/ directory, which are not needed for runtime, so you moved them to -doc subpackage (which is completely ok, I think), but they are not documentation.
- The real documentation (README.md and the doc directory).
So, to me, it makes sense to distinct these two.

Additional comments:
- I think it is clearer not to remove the files by "rm", but use %exclude in %files. But this is just my opinion, so not a blocker.
- As for the macros vs. commands thing: There are also macros for commands like "rm", so it may be good to use them, once you decide to use macros for some commands. But at this stage, it doesn't have the feeling of inconsistency, so not a blocker. (BTW, I think that using macros for things like "mkdir -p" is not necessary, but again, just my opinion.)
- The link in comment #5 points to the first release srpm, so when importing to fedpkg, please make sure to import the second one :)

Since none of my additional comments are blockers, this package is APPROVED.

(In reply to comment #6)
> > > - Move %{geminstdir}/README.md to the -doc subpackage, if it is not needed for
> > > runtime (which I believe it isn't) and mark it with %doc.
> > > - Also, mark %{gemdir}/doc/%{gemname}-%{version} with %doc.
> > 
> > Actually I don't think marking -doc subpackage files with %doc is necessary.
> > Could you point me to a guideline where this is required?
> 
> I believe that there is no specific guideline for this. But if you take a look
> at it from the logical point of view, you have two types of files in  your -doc
> subpackage:
> - The Rakefile and spec/ directory, which are not needed for runtime, so you
> moved them to -doc subpackage (which is completely ok, I think), but they are
> not documentation.
> - The real documentation (README.md and the doc directory).
> So, to me, it makes sense to distinct these two.

Good point. My general through was that once the subpackage is marked as '-doc'
the files are already recognized as %doc and thus don't need additional marking.

Anyway, I'll mark them, not a blocker for me of course ;-)
 
> Additional comments:
> - I think it is clearer not to remove the files by "rm", but use %exclude in
> %files. But this is just my opinion, so not a blocker.
> - As for the macros vs. commands thing: There are also macros for commands like
> "rm", so it may be good to use them, once you decide to use macros for some
> commands. But at this stage, it doesn't have the feeling of inconsistency, so
> not a blocker. (BTW, I think that using macros for things like "mkdir -p" is
> not necessary, but again, just my opinion.)
> - The link in comment #5 points to the first release srpm, so when importing to
> fedpkg, please make sure to import the second one :)

Sure, I'll try to fix that before importing to Fedora (EPEL). Thanks for review Bohuslav!

New Package SCM Request
=======================
Package Name: rubygem-rack-protection
Short Description: Ruby gem that protects against typical web attacks
Owners: mfojtik
Branches: f16 el6

Git done (by process-git-requests).

rubygem-rack-protection-1.2.0-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rubygem-rack-protection-1.2.0-2.fc16

rubygem-rack-protection-1.2.0-2.fc16 has been pushed to the Fedora 16 testing repository.

rubygem-rack-protection-1.2.0-2.fc16 has been pushed to the Fedora 16 stable repository.

rubygem-rack-protection-1.2.0-2.fc16 has been pushed to the Fedora 16 stable repository.