Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1114146 - Review Request: rubygem-ffi-yajl - Ruby FFI wrapper around YAJL 2.x
Summary: Review Request: rubygem-ffi-yajl - Ruby FFI wrapper around YAJL 2.x
Keywords:
Status: ASSIGNED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1137007
Blocks: 823344 823352 1133213
TreeView+ depends on / blocked
 
Reported: 2014-06-27 21:25 UTC by Julian C. Dunn
Modified: 2018-02-05 12:32 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed:
vondruch: fedora-review?


Attachments (Terms of Use)

Description Julian C. Dunn 2014-06-27 21:25:18 UTC
Spec URL: http://people.fedoraproject.org/~jdunn/rubygem-ffi-yajl/rubygem-ffi-yajl.spec
SRPM URL: http://people.fedoraproject.org/~jdunn/rubygem-ffi-yajl/rubygem-ffi-yajl-0.2.0-1.fc21.src.rpm
Description: Ruby FFI wrapper around YAJL 2.x
Fedora Account System Username: jdunn

rpmlint output on Rawhide:

rubygem-ffi-yajl.x86_64: E: useless-provides rubygem(ffi-yajl)
rubygem-ffi-yajl.x86_64: W: no-documentation
rubygem-ffi-yajl.x86_64: E: zero-length /usr/lib64/gems/ruby/ffi-yajl-0.2.0/gem.build_complete

I want to use the same spec for building for EL6 and 7 so I am ignoring useless-provides. Also I am unsure whether gem.build_complete should be %excluded or not, since the Ruby guidelines say nothing about that.

rpmlint output on F20:

rubygem-ffi-yajl.x86_64: W: no-documentation

Comment 1 Vít Ondruch 2014-07-08 10:04:27 UTC
I'll take this for a review.

Comment 2 Vít Ondruch 2014-07-08 10:56:19 UTC
* Patches are missing comments
  - Your .spec file contains 4 patches. It would be nice to comment them what
    they are good for, why they are not upstream. For example, Patch3 seems to
    be fixing compatibility with RSpec 2.x, while upstream is using RSpec 3.x
    already.

* libyaml2 gem dependency
  - I have to say, I am disappointed with this way of bundling, although not
    sure if you can do something about it, since this is upstream issue, but I
    must point this out.

    I'd call your patches substantial. The biggest change is that you completely
    drop the dependency on libyajl2, that means if somebody is sharing
    Gemfile.lock (and we can put aside if this is good idea or not), their
    dependencies will differ for Fedoras version of ffi-libyaml in comparison to
    original gems.

    This very much reminds me the current situation with therubyracer.
    therubyracer is currently more or less unupdatable, since it depends on v8
    gem, where the v8 gem version corresponds with the version of shipped v8
    library. Unfortunately, our system v8 has different version. So there is not
    possible to unbundle the v8 without breaking what the version is
    specifying. This is not yet case of the libyaml2, but I won't be
    surprised if somebody will realize soon, that it could have the same
    version as the shipped lirary.

    If the bundling is necessary, I'd rather see the approach Psych is using for
    example, i.e. it bundles the psych library directly inside the gem, if
    system one is not available. This way of bundling is quite easy to remove.

So to say, I'll release this review to somebody else, since I cannot sign myself under this.

With regards to gem.build_complete, yes, you have to keep it. It is in current guidelines [1] (last line of example), although the guidelines could get updated in the meantime. This is RubyGems requirement, otherwise they tries to recompile the binary extension. Packaging guidelines just follows.



[1] https://fedoraproject.org/wiki/Packaging:Ruby#Building_gems

Comment 3 Julian C. Dunn 2014-07-08 13:24:03 UTC
(In reply to Vít Ondruch from comment #2)

> * Patches are missing comments
>   - Your .spec file contains 4 patches. It would be nice to comment them what
>     they are good for, why they are not upstream. For example, Patch3 seems
> to
>     be fixing compatibility with RSpec 2.x, while upstream is using RSpec 3.x
>     already.
> 
> * libyaml2 gem dependency
>   - I have to say, I am disappointed with this way of bundling, although not
>     sure if you can do something about it, since this is upstream issue, but
> I
>     must point this out.
> 
>     I'd call your patches substantial. The biggest change is that you
> completely
>     drop the dependency on libyajl2, that means if somebody is sharing
>     Gemfile.lock (and we can put aside if this is good idea or not), their
>     dependencies will differ for Fedoras version of ffi-libyaml in
> comparison to
>     original gems.

So I have already had this discussion with upstream. The vendoring (or not) of the C library is all within the separate libyajl2 gem, to abstract that away. I can separately package that gem as rubygem-libyajl2 with the vendoring turned off (it's supported by upstream) instead of doing it the way I have done; would that be acceptable, to maintain the existing dependency tree?

The only reason I did it this way is because at this point the rubygem-libyajl2 package becomes a complete no-op and is only used to satisfy gem deps.

Comment 4 Vít Ondruch 2014-07-09 08:26:35 UTC
(In reply to Julian C. Dunn from comment #3)
> So I have already had this discussion with upstream. The vendoring (or not)
> of the C library is all within the separate libyajl2 gem, to abstract that
> away.

Yes, and that is the point. This is one level of abstraction too much.

If the vendored library would be part of the ffi-yajl, as that was actually case for the yajl-ruby, that would be OK.

If that would be no hard dependency, that would be OK as well. You can take a look at bson and bson_ext for example, or at multi_json. Every of this library has dependencies. If they are not fulfilled, warning or error is issued.

> I can separately package that gem as rubygem-libyajl2 with the
> vendoring turned off (it's supported by upstream) instead of doing it the
> way I have done; would that be acceptable, to maintain the existing
> dependency tree?

With sad hearth. Since I am afraid it will end up as therubyracer with its v8 dependency :/

> The only reason I did it this way is because at this point the
> rubygem-libyajl2 package becomes a complete no-op and is only used to
> satisfy gem deps.

Yes, I understand your reasons.

Comment 5 Julian C. Dunn 2014-09-03 20:17:16 UTC
(In reply to Vít Ondruch from comment #4)
> (In reply to Julian C. Dunn from comment #3)
> > So I have already had this discussion with upstream. The vendoring (or not)
> > of the C library is all within the separate libyajl2 gem, to abstract that
> > away.
> 
> Yes, and that is the point. This is one level of abstraction too much.

Well, upstream has reasons for doing it that way (wanted to divorce building native extension for vendored yajls from the FFI wrapper, because the native extension needs to be built on many different esoteric platforms). But I already had conversations with them & they are not willing to change it, so here we are.

> > I can separately package that gem as rubygem-libyajl2 with the
> > vendoring turned off (it's supported by upstream) instead of doing it the
> > way I have done; would that be acceptable, to maintain the existing
> > dependency tree?
> 
> With sad hearth. Since I am afraid it will end up as therubyracer with its
> v8 dependency :/

I have addressed the concerns by packaging rubygem-libyajl2 separately and removed the invasive patches. I also took the opportunity to upgrade to the latest version of ffi-yajl.

SRPM: https://jdunn.fedorapeople.org/rubygem-ffi-yajl/rubygem-ffi-yajl-1.1.0-1.fc20.src.rpm
SPEC: https://jdunn.fedorapeople.org/rubygem-ffi-yajl/rubygem-ffi-yajl.spec

Comment 6 Michel Alexandre Salim 2017-01-13 22:14:02 UTC
Vit, are you interested in continuing the review, and Julian, are you still interested in packaging this?

I need to experiment with Chef Zero at work and so would like to have the related Chef Zero upgrade unblocked. If there's still interest in getting this done I'll help do a review.

Comment 7 Julian C. Dunn 2017-01-15 20:19:07 UTC
I can continue to work on it with your review as long as we are ok on the approach of packaging rubygem-libyajl2 separately.

Comment 8 Vít Ondruch 2017-01-16 16:03:42 UTC
(In reply to Julian C. Dunn from comment #7)
> I can continue to work on it with your review as long as we are ok on the
> approach of packaging rubygem-libyajl2 separately.

I still think that packaging independent rubygem-libyajl2 is bad idea. But I am quite sure that with some effort, ffi-yajl can be made to use the system library.

Comment 9 Michel Alexandre Salim 2017-01-26 22:38:46 UTC
Yeah, that sounds best. Julian, interested in doing that?

Comment 10 Vít Ondruch 2017-05-30 12:01:21 UTC
Hm, this does not appear to lead anywhere, so here is my shot:

Spec URL: https://fedorapeople.org/cgit/vondruch/public_git/rubygem-ffi-yajl.git/plain/rubygem-ffi-yajl.spec?id=a8f49d1a38159d96de202804f20f9ce79e2b35fc
SRPM URL: http://people.redhat.com/vondruch/rubygem-ffi-yajl-2.3.0-1.fc27.src.rpm
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=19781253

And now a bit of explanation:

1) This drops the dependency on libyajl2 gem. Really, there is no reason to package this.
2) I replaced the package with stub file, which provides the "no functionality" which would be provided by the libyajl2 gem.
3) The ffi-yajl must be adjusted to load the correct system library. This is due to lame (no offense) libyajl2 packaging practices.

And lastly, the FFI version of the package does not work. I don't think this is big deal for Fedora. If it is, there is way to fix it, but the test suite is not passing due to some assumptions and I did not bothered to fix it, but there is upstream ticket.

Comment 11 Julian C. Dunn 2018-01-21 06:30:28 UTC
This all seems reasonable. I updated it to rubygem-ffi-yajl 2.3.1 and rebuilt it; seems fine:

SRPM: https://fedorapeople.org/~jdunn/rubygem-ffi-yajl/rubygem-ffi-yajl-2.3.1-1.fc27.src.rpm
SPEC: https://fedorapeople.org/~jdunn/rubygem-ffi-yajl/rubygem-ffi-yajl.spec
Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=24336747

Are we good to go on this now?

Comment 12 Vít Ondruch 2018-02-05 12:32:43 UTC
(In reply to Julian C. Dunn from comment #11)
> This all seems reasonable. I updated it to rubygem-ffi-yajl 2.3.1 and
> rebuilt it

There is missing changelog entry, but this is just minor nit.

> Are we good to go on this now?

Well, I am, but since I proposed this, I don't think I am eligible to approve this.

BTW it would be probably better to have the runtime dependency directly on the libyajl library instead of the package, i.e.:

~~~
Requires: libyajl.so.2()%{_isa}
~~~


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