Bug 850679 - Review Request: rubygem-pdf-reader - Ruby library to parse PDF files
Review Request: rubygem-pdf-reader - Ruby library to parse PDF files
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Bohuslav "Slavek" Kabrda
Fedora Extras Quality Assurance
:
Depends On: 850469
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-22 03:03 EDT by Miroslav Suchý
Modified: 2012-09-26 17:36 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-09-26 17:36:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bkabrda: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Miroslav Suchý 2012-08-22 03:03:53 EDT
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-3.fc17.src.rpm
Description: 
The PDF::Reader library implements a PDF parser conforming as much as possible
to the PDF specification from Adobe.

Fedora Account System Username: msuchy

$ rpmlint /tmp/tito-build/rubygem-pdf-reader-1.1.1-2.fc17.src.rpm /tmp/tito-build/noarch/rubygem-pdf-reader-1.1.1-2.fc17.noarch.rpm
rubygem-pdf-reader.noarch: W: no-manual-page-for-binary pdf_object
rubygem-pdf-reader.noarch: W: no-manual-page-for-binary pdf_text
rubygem-pdf-reader.noarch: W: no-manual-page-for-binary pdf_callbacks
rubygem-pdf-reader.noarch: W: no-manual-page-for-binary pdf_list_callbacks
2 packages and 0 specfiles checked; 0 errors, 4 warnings.

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4412558
Comment 1 Bohuslav "Slavek" Kabrda 2012-08-23 08:34:16 EDT
I'll take this for a review.
Comment 2 Bohuslav "Slavek" Kabrda 2012-08-23 08:58:17 EDT
- 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.
Comment 3 Bohuslav "Slavek" Kabrda 2012-08-23 09:00:43 EDT
Oh, one more forgotten comment - don't remove the %{gem_instdir}/bin directory. The files inside it are used by stubs in %{_bindir}.
Comment 4 Miroslav Suchý 2012-08-23 11:20:20 EDT
> 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
Comment 5 Bohuslav "Slavek" Kabrda 2012-08-24 01:46:13 EDT
(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
Comment 7 Bohuslav "Slavek" Kabrda 2012-09-05 03:30:14 EDT
- 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?
Comment 8 Miroslav Suchý 2012-09-05 06:03:14 EDT
> - 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.
Comment 9 Bohuslav "Slavek" Kabrda 2012-09-05 06:10:20 EDT
(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.
Comment 11 Miroslav Suchý 2012-09-05 06:29:21 EDT
>> 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 :)
Comment 12 Bohuslav "Slavek" Kabrda 2012-09-05 07:20:54 EDT
(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 :)
Comment 13 Miroslav Suchý 2012-09-05 07:48:26 EDT
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:
Comment 14 Gwyn Ciesla 2012-09-05 08:33:24 EDT
Git done (by process-git-requests).
Comment 15 Fedora Update System 2012-09-10 03:03:49 EDT
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
Comment 16 Fedora Update System 2012-09-10 12:35:22 EDT
rubygem-pdf-reader-1.1.1-6.el6 has been pushed to the Fedora EPEL 6 testing repository.
Comment 17 Fedora Update System 2012-09-26 17:36:18 EDT
rubygem-pdf-reader-1.1.1-6.el6 has been pushed to the Fedora EPEL 6 stable repository.

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