Bug 850679

I'll take this for a review.

- Please run the upstream specs in %check section.
- Did you try building the package on RHEL? It seems to me that you have no BuildRequires for RHEL, therefore it will not build (on RHEL, you should BR: rubygems, at least).
- You should probably make the scripts in examples executable (some already are, but some are not).
- You should fix the Ascii85 gem dependency. The gemspec says

s.add_runtime_dependency(%q<Ascii85>, ["~> 1.0.0"])

So you should also use Requires: rubygem(Ascii85) < 1.1

Otherwise the package looks good. Please post the updated spec and srpm, before I can approve it.

Oh, one more forgotten comment - don't remove the %{gem_instdir}/bin directory. The files inside it are used by stubs in %{_bindir}.

> Requires: rubygem(Ascii85) < 1.1
fixed

> Please run the upstream specs in %check section.
Which one? I see non in that gem file.

> make the scripts in examples executable
This is not intended for runnning (will even SELinux allow it?) and it is in gem_docdir. I assume the intention is that developer will just read it as sample tutotiral. Do you see this as blocker?

> Did you try building the package on RHEL?
No, just Fedora :)
Fixed
BR: rubygems was enough http://koji.fedoraproject.org/koji/taskinfo?taskID=4417470

>%{gem_instdir}/bin
fixed

Spec URL: http://miroslav.suchy.cz/fedora/rubygem-pdf-reader/rubygem-pdf-reader.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-pdf-reader/rubygem-pdf-reader-1.1.1-4.fc17.src.rpm

(In reply to comment #4)
> > Please run the upstream specs in %check section.
> Which one? I see non in that gem file.
> 

Yes, upstream sometimes doesn't include test suite in the .gem file, but has them [1]. There is a quick howto for such cases in Ruby packaging guidelines [2]. I consider this a blocker, running tests/specs is very important.

> > make the scripts in examples executable
> This is not intended for runnning (will even SELinux allow it?) and it is in
> gem_docdir. I assume the intention is that developer will just read it as
> sample tutotiral. Do you see this as blocker?
> 

Not a blocker, but your current way is not consistent. Either make _all_ of the examples executable or _all_ non-executable. I'd still prefer making them executable, as I don't see any possible selinux problems there.


[1] https://github.com/yob/pdf-reader/tree/master/spec
[2] https://fedoraproject.org/wiki/Packaging:Ruby#Test_suites_not_included_in_the_package

Addressed.

Spec URL: http://miroslav.suchy.cz/fedora/rubygem-pdf-reader/rubygem-pdf-reader.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-pdf-reader/rubygem-pdf-reader-1.1.1-5.fc17.src.rpm

- Building in mock results in an error:

DEBUG: + rspec spec
DEBUG: /usr/share/rubygems/rubygems/custom_require.rb:36:in `require': cannot load such file -- minitest/unit (LoadError)

You need to BR: rubygem(minitest) to fix this. Please do mock or koji-scratch builds before posting the spec and srpm. They can find errors like this and will help you to go faster through the review.

- It is good to include package version in the name of the archive with specs. This assures that when you update to newest version, rpmbuild will shout that the archive was not found. This will remind you that you need to package newer specs as well.

- Why exactly is the spec execution conditionalized? And why for Fedora 16 and higher? Fedora 15 is EOLed, I see no point in that. Could you please explain this?

> - Why exactly is the spec execution conditionalized? And why for Fedora 16 and > higher? Fedora 15 is EOLed, I see no point in that. Could you please explain this?

To be precise it is Fedora 17 and higher:
  %if 0%{?fedora} > 16

It is becase rubygem-rspec is only in 
F15, F17, F18 and rawhide.

It is missing in EPEL and in F16.

(In reply to comment #8)
> > - Why exactly is the spec execution conditionalized? And why for Fedora 16 and > higher? Fedora 15 is EOLed, I see no point in that. Could you please explain this?
> 
> To be precise it is Fedora 17 and higher:
>   %if 0%{?fedora} > 16
> 

Ah, yes, sorry about that.

> It is becase rubygem-rspec is only in 
> F15, F17, F18 and rawhide.
> 
> It is missing in EPEL and in F16.

It should be named rubygem-rspec-core in F16 and EPEL - you can try that out.

Spec URL: http://miroslav.suchy.cz/fedora/rubygem-pdf-reader/rubygem-pdf-reader.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-pdf-reader/rubygem-pdf-reader-1.1.1-6.fc17.src.rpm

>> It is missing in EPEL and in F16.
>
>It should be named rubygem-rspec-core in F16 and EPEL

I investigated it little bit more:

rubygem-rspec exist in EPEL6, but there is no /usr/bin/rspec and there exist only /usr/bin/spec

You are correct with rubygem-rspec-core in F16 and it even contains /usr/bin/rspec.

But I'm not sure if it is worth the work. I definitelly do not want to spend the time on F16 as it is close to EOL.
And if you will insist on running tests on EL6, I can give it try, but I'm lazy and I hesitate to do that :)

(In reply to comment #11)
> >> It is missing in EPEL and in F16.
> >
> >It should be named rubygem-rspec-core in F16 and EPEL
> 
> I investigated it little bit more:
> 
> rubygem-rspec exist in EPEL6, but there is no /usr/bin/rspec and there exist
> only /usr/bin/spec
> 
> You are correct with rubygem-rspec-core in F16 and it even contains
> /usr/bin/rspec.
> 
> But I'm not sure if it is worth the work. I definitelly do not want to spend
> the time on F16 as it is close to EOL.
> And if you will insist on running tests on EL6, I can give it try, but I'm
> lazy and I hesitate to do that :)

Not a blocker. The tests run in Fedora and that is the important thing for this review.
The package now looks fine now it is APPROVED :)

New Package SCM Request
=======================
Package Name: rubygem-pdf-reader
Short Description: Ruby library to parse PDF files
Owners: msuchy
Branches: F-18, F-17, F-16, EL-6
InitialCC:

Git done (by process-git-requests).

rubygem-pdf-reader-1.1.1-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/rubygem-pdf-reader-1.1.1-6.el6

rubygem-pdf-reader-1.1.1-6.el6 has been pushed to the Fedora EPEL 6 testing repository.

rubygem-pdf-reader-1.1.1-6.el6 has been pushed to the Fedora EPEL 6 stable repository.