Bug 1870208

Summary:	Review Request: rubygem-ruby-vips - Ruby extension for the vips image processing library
Product:	[Fedora] Fedora	Reporter:	Pavel Valena <pvalena>
Component:	Package Review	Assignee:	Vít Ondruch <vondruch>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	package-review, vondruch
Target Milestone:	---	Flags:	vondruch: fedora-review+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:	rubygem-ruby-vips-2.0.17-1.fc34	Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2020-10-30 00:27:49 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Pavel Valena 2020-08-19 14:28:44 UTC

Spec URL: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01616639-rubygem-ruby-vips/rubygem-ruby-vips.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01616639-rubygem-ruby-vips/rubygem-ruby-vips-2.0.17-1.fc34.src.rpm
Description: ruby-vips is a binding for the vips image processing library. It is fast and it can process large images without loading the whole image in memory.

Fedora Account System Username: pvalena

Builds & Tests run: https://gist.github.com/pvalena/75824094c5d97a347c036680bb2185ed

Comment 1 Vít Ondruch 2020-08-19 14:59:56 UTC

I'm taking this for a review.

Comment 2 Vít Ondruch 2020-08-19 16:07:28 UTC

* `BuildRequires: vips-devel`
  - What is the reason to include vips-devel instead of just vips?

* Requires
  - I think there should be something aka `Requires: vips`
  - Possibly, there could be required all libraries loaded via `ffi_lib` call.

BTW, I'd rather see requires such as 'libvips.so.42`, but they are not properly required [1].

* Wrong shebangs:

~~~
rubygem-ruby-vips-doc.noarch: E: env-script-interpreter /usr/share/gems/gems/ruby-vips-2.0.17/example/example1.rb /usr/bin/env ruby
rubygem-ruby-vips-doc.noarch: E: env-script-interpreter /usr/share/gems/gems/ruby-vips-2.0.17/example/thumb.rb /usr/bin/env ruby
~~~




[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1870275

Comment 3 Pavel Valena 2020-08-20 21:32:40 UTC

Thank you for your reviews!

>   - What is the reason to include vips-devel instead of just vips?

You're right, I thought I needed headers, but I don't.

>   - I think there should be something aka `Requires: vips`

Yes, you're right. I've tried using it in the test suite of image_processing gem (which succeeds).

> * Wrong shebangs:

Hmm. I've already fixed that (I've linked probably an earlier iteration of spec file by mistake).


Refreshed SPEC and SRPM with the changes (the release bump is just for COPR).

Spec: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01619049-rubygem-ruby-vips/rubygem-ruby-vips.spec
SRPM: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01619049-rubygem-ruby-vips/rubygem-ruby-vips-2.0.17-2.fc34.src.rpm


Builds & Test log: https://gist.github.com/pvalena/7dc8d06603fc58c7f207cd854a1d4b8d

Comment 4 Vít Ondruch 2020-08-26 14:02:52 UTC

(In reply to Pavel Valena from comment #3)
> > * Wrong shebangs:
> 
> Hmm. I've already fixed that (I've linked probably an earlier iteration of
> spec file by mistake).

I admire your upstream convincing skills in this regard :)

Otherwise LGTM => APPROVED

Comment 5 Vít Ondruch 2020-09-14 09:00:31 UTC

(In reply to Vít Ondruch from comment #2)
> BTW, I'd rather see requires such as 'libvips.so.42`, but they are not
> properly required [1].

And it turns out, there is way to do it according to RPM upstream [1]:

~~~
Requires: (libvips.so.42()(64bit) if libc.so.6()(64bit))
Requires: (libvips.so.42 if libc.so.6)
~~~

Because this is Ruby package, we could possible use libruby.so instead.


[1]: https://github.com/rpm-software-management/rpm/issues/1344#issuecomment-681916527

Comment 6 Vít Ondruch 2020-09-14 15:51:20 UTC

(In reply to Vít Ondruch from comment #5)
> Because this is Ruby package, we could possible use libruby.so instead.

or libffi.so.6

Comment 7 Pavel Valena 2020-09-23 08:47:25 UTC

(In reply to Vít Ondruch from comment #4)
> (In reply to Pavel Valena from comment #3)
> > > * Wrong shebangs:
> > 
> > Hmm. I've already fixed that (I've linked probably an earlier iteration of
> > spec file by mistake).
> 
> I admire your upstream convincing skills in this regard :)
> 
> Otherwise LGTM => APPROVED

I think it was luck.

(In reply to Vít Ondruch from comment #5)
> (In reply to Vít Ondruch from comment #2)
> > BTW, I'd rather see requires such as 'libvips.so.42`, but they are not
> > properly required [1].
> 
> And it turns out, there is way to do it according to RPM upstream [1]:
> 
> ~~~
> Requires: (libvips.so.42()(64bit) if libc.so.6()(64bit))
> Requires: (libvips.so.42 if libc.so.6)
> ~~~
> 
> Because this is Ruby package, we could possible use libruby.so instead.
> 
> 
> [1]:
> https://github.com/rpm-software-management/rpm/issues/1344#issuecomment-
> 681916527

Thanks of figuring this out! `libffi.so.6` seems like a good choice indeed.

Comment 8 Gwyn Ciesla 2020-09-23 13:31:40 UTC

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-ruby-vips