https://bugzilla.redhat.com/show_bug.cgi?id=1114146
Bug ID: 1114146 Summary: Review Request: rubygem-ffi-yajl - Ruby FFI wrapper around YAJL 2.x Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: jdunn@aquezada.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://people.fedoraproject.org/~jdunn/rubygem-ffi-yajl/rubygem-ffi-yajl.spe... SRPM URL: http://people.fedoraproject.org/~jdunn/rubygem-ffi-yajl/rubygem-ffi-yajl-0.2... 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
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
Julian C. Dunn jdunn@aquezada.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |823344
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=823344 [Bug 823344] Review Request: rubygem-ohai - detects data about your system, exports as JSON for use with Chef
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
Julian C. Dunn jdunn@aquezada.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |823352
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=823352 [Bug 823352] Review Request: rubygem-chef - a client for the Chef config management system
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |vondruch@redhat.com Assignee|nobody@fedoraproject.org |vondruch@redhat.com Flags| |fedora-review?
--- Comment #1 from Vít Ondruch vondruch@redhat.com --- I'll take this for a review.
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #2 from Vít Ondruch vondruch@redhat.com --- * 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
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #3 from Julian C. Dunn jdunn@aquezada.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #4 from Vít Ondruch vondruch@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
Julian C. Dunn jdunn@aquezada.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |1137007
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1137007 [Bug 1137007] Review Request: rubygem-libyajl2 - Installs a vendored copy of libyajl2 for distributions which lack it
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #5 from Julian C. Dunn jdunn@aquezada.com --- (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.fc2... SPEC: https://jdunn.fedorapeople.org/rubygem-ffi-yajl/rubygem-ffi-yajl.spec
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
Julian C. Dunn jdunn@aquezada.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1133213
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1133213 [Bug 1133213] rubygem-chef-zero-3.2.1 is available
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
Michel Alexandre Salim michel@michel-slm.name changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |michel@michel-slm.name Flags| |needinfo?
--- Comment #6 from Michel Alexandre Salim michel@michel-slm.name --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
Julian C. Dunn jdunn@aquezada.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo? |
--- Comment #7 from Julian C. Dunn jdunn@aquezada.com --- I can continue to work on it with your review as long as we are ok on the approach of packaging rubygem-libyajl2 separately.
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #8 from Vít Ondruch vondruch@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #9 from Michel Alexandre Salim michel@michel-slm.name --- Yeah, that sounds best. Julian, interested in doing that?
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #10 from Vít Ondruch vondruch@redhat.com --- 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... 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #11 from Julian C. Dunn jdunn@aquezada.com --- 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.fc... 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?
https://bugzilla.redhat.com/show_bug.cgi?id=1114146
--- Comment #12 from Vít Ondruch vondruch@redhat.com --- (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} ~~~
package-review@lists.fedoraproject.org