I'll take this for a review.

* 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

(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.

(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.

(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

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.

I can continue to work on it with your review as long as we are ok on the approach of packaging rubygem-libyajl2 separately.

(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.

Yeah, that sounds best. Julian, interested in doing that?

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.

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?

(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}
~~~

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

I'd like to get this packaged as well, so I've updated it to 2.4.0 and resubmitted it in #1955985. Happy to add you as co-maintainer if you'd like.

*** This bug has been marked as a duplicate of bug 1955985 ***