Bug 823351 - Review Request: rubygem-yajl-ruby - Ruby C bindings to YAJL - a JSON stream-based parser
Summary: Review Request: rubygem-yajl-ruby - Ruby C bindings to YAJL - a JSON stream-b...
Status: ASSIGNED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
(Show other bugs)
Version: rawhide
Hardware: Unspecified Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
: 1373005 (view as bug list)
Depends On:
Blocks: oVirt_on_Fedora
TreeView+ depends on / blocked
 
Reported: 2012-05-21 01:38 UTC by Jonas Courteau
Modified: 2018-01-09 16:52 UTC (History)
13 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
vondruch: fedora-review?


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
RDO 10681 None None None 2017-11-23 14:19 UTC
Red Hat Bugzilla 1426110 None None None Never

Description Jonas Courteau 2012-05-21 01:38:27 UTC
Spec URL: https://raw.github.com/jcourteau/rubygems-rpms/master/fc17/rubygem-yajl-ruby/rubygem-yajl-ruby.spec
SRPM URL: http://rpms.courteau.org/fedora/rubygem-yajl-ruby-1.1.0-1.fc17.src.rpm

Description: Ruby C bindings to the excellent Yajl JSON stream-based parser library

This is part of a set of dependencies for rubygem-chef.  I've got about 14 packages to add, all ruby gems, and am looking for a sponsor.  Several of the packages were previously in Fedora (F11 and F12), but were removed due to lack of a maintainer.

Comment 1 Vít Ondruch 2012-12-14 14:16:08 UTC
I'll take it for a review, since it is dependency of rubygem-chef. Not sure if Jonas is active though.

Comment 2 Vít Ondruch 2012-12-14 15:51:39 UTC
* Keep Gemfile and %{gem_name}.gemspec
  - Please consider to keep above mentioned files in -doc subpackage. Although
    they are not needed for runtime, they were originally shipped by upstream,
    so lets try to keep the modifications limited.

* Move files not needed for runtime into -doc subpackage
  - Please consider to move following files/directories into -doc subpackage,
    since they are not needed for runtime IMO:

      %doc %{gem_instdir}/CHANGELOG.md
      %doc %{gem_instdir}/README.md
      %{gem_instdir}/benchmark
      %{gem_instdir}/spec
      %{gem_instdir}/tasks

* Package does not own the binary extension directories
  - The "%{gem_extdir}/lib/yajl/yajl.so" means that only the yajl.so file is
    owned by the package. Please use "%{gem_extdir}" instead, to own the whole
    directory structure including the .so file.

* Exclude %{gem_cache}
  - Please exclude %{gem_cache}. This file is not needed on Fedora.

* Bundled library
  - This gem contains bundled YAJL library. Please work with upstream to allow
    to use the YAJL available on system.
  - I already submitted issue upstream: 
        https://github.com/brianmario/yajl-ruby/issues/111

Comment 3 Julian C. Dunn 2012-12-25 16:02:24 UTC
I have corrected all the issues identified and rebuilt the SRPM.

http://fedorapeople.org/~jdunn/rubygem-yajl-ruby/rubygem-yajl-ruby-1.1.0-2.fc19.src.rpm
http://fedorapeople.org/~jdunn/rubygem-yajl-ruby/rubygem-yajl-ruby.spec

I take it the bundled YAJL is a showstopper and that we can't proceed until upstream fixes this?

Comment 4 Michael Scherer 2012-12-30 17:21:06 UTC
Yep, the bundled is a showstopper. But we can proceed on the other review in the mean time.

Comment 5 Vít Ondruch 2013-01-02 13:25:45 UTC
Josef Stribny is trying to make yajl work with Fedora's yajl. Hopefully he will provide use with some update soon.

Comment 6 Josef Stribny 2013-01-03 10:47:58 UTC
As Vit has already mentioned, I am looking into yajl-ruby to find a way to make it work with system yajl (which is one version ahead, but with patches from the patched lib included with yajl-ruby).

Comment 7 Josef Stribny 2013-01-22 13:05:16 UTC
I made two patches for yajl-ruby; first is an update to Yajl 2 (to match the current system version on Fedora) and second is a switch to the system version of the library. I am discussing the progress with upstream on the GitHub project page [1].

[1] https://github.com/brianmario/yajl-ruby/issues/111#issuecomment-12542826

Comment 8 Vít Ondruch 2013-01-23 15:24:30 UTC
Just FYI, I just sent pull request [1] to upstream allowing yajl-ruby to build against system yajl library.


[1] https://github.com/brianmario/yajl-ruby/issues/113

Comment 9 Vaidas Jablonskis 2013-02-10 21:15:16 UTC
I think, once the upstream merges the changes to allow to use system's yajl libraries, the name of this package should be changed to 'rubygem-yajl' as per Fedora rubygem naming guidelines.

Comment 10 Vít Ondruch 2013-02-11 09:01:16 UTC
(In reply to comment #9)
> I think, once the upstream merges the changes to allow to use system's yajl
> libraries, the name of this package should be changed to 'rubygem-yajl' as
> per Fedora rubygem naming guidelines.

I disagree. This guidelines applies to "ruby" packages, not to rubygems. Moreover, we already met packages which were named foo and foo-ruby, while their sourcecode and upstreams were different.

Comment 11 Matthew Miller 2013-07-17 19:09:41 UTC
Can we go ahead with the "devendorized" patches locally in this package, until upstream responds?

Comment 12 Ken Dreyer 2013-11-02 06:19:37 UTC
Adding myself to the CC, since this blocks Gitorious in Fedora as well. (yajl-ruby -> pygments.rb -> makeup -> libdolt -> Gitorious)

Comment 13 Ken Dreyer 2013-12-27 05:55:00 UTC
Hi Julian, I've made some adjustments to the spec file and pushed the changes to a Git repo, here:
http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-yajl-ruby.git/log/

I've broken each change into a separate Git commit so you can review each one. I'll also try to explain each suggested change below.

1. (60bdf07) remove gem2rpm autogenerated comment. The gem2rpm tool inserts this comment into the packages, but it is just cruft so we can remove it.

2. (233f9b0) use HTTPS in URLs. This commit changes the URL from using plaintext HTTP to secured HTTPS.

3. (f054c1c) remove trailing whitespace. There were two lines with trailing whitespace in the spec file, so this commit fixes that.

4. (27bddfb) simplify el6/rubyabi conditionals. The last Fedora version that supported ruby(abi) is Fedora 18, and this is going end-of-life in a few days. We only need to support ruby(abi) on EL6. This commit simplifies the RPM dist conditionals to reflect this.

5. (a9e8c12) build doc subpackage as noarch. The -doc subpackage should be set to "noarch" since there's no architecture-specific files within it.

6. (c6cb71d) move build process into %build. The Ruby guidelines indicate that the gem should be unpacked in %prep and built in %build. This commit runs "gem unpack" and "gem spec" in %prep, and "gem build" in %build.

7. (2abbc85) use %gem_install macro. On platforms that are not EL6, the %gem_install macro is available and preferable to manually compiling the gem.

8. (2b748f8) use %{ruby_sitearch} and %{gem_extdir_mri}. The %{gem_extdir} macro is deprecated. Use %{ruby_sitearch} on EL6, and use %{gem_extdir_mri} elsewhere.

9. (a2de3a8) BuildRequire ruby(rubygems). I needed to add this to get the package to build on EL6.

10. (031261f) mark README.md and CHANGELOG.md as %doc. These files are text, not code, so I marked them as such in this commit.

11. (8f5c851) move README.md to main package. I thought it would be a good idea to ship this in the main package because so that it's more accessible to users.

12. (1e3db58) delete unnecessary gemspec during %install. It's not necessary to ship two gemspecs for this package. This commit deletes the duplicate during %install.

13. (128ca61) remove developer-only files during %prep. Some files were deleted during %install, some files were %exclude'd, and some were shipped in -doc. This commit just deletes them all during %prep (and removes them from the gemspec) for cleanliness and consistency.

I've tested this version on EL6 and F19, and simple JSON tests work on both platforms.

Comment 14 Ken Dreyer 2014-02-15 20:12:47 UTC
Hi Julian, just checking to see if you've had a chance to review those patches and merge them into your yajl-ruby package? It would be great to get yajl-ruby into Fedora.

I've updated for the latest yajl-ruby release (1.2.0):

http://fedorapeople.org/cgit/ktdreyer/public_git/rubygem-yajl-ruby.git/commit/?id=40e3e04fa5fb421ffda3b2112eb0570257413fb5

F21 scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6533809

Comment 15 Julian C. Dunn 2014-02-17 00:18:36 UTC
Hi Ken, I'm ok with the build patches, but we still need to solve the big issue which is whether to integrate the de-vendorization patch into Fedora yajl. That would potentially change the behavior of yajl from upstream, since the patch is not 100% finished pending input from (unresponsive) upstream we would need to make a call as to what to do.

Specifically: the de-vendorization patch (https://github.com/brianmario/yajl-ruby/pull/113) causes some tests to fail, and I'm unsure whether this is a problem.

Comment 16 Vít Ondruch 2014-03-12 15:23:43 UTC
Just for the record, it seems tha yajl-ruby might be renamed to yajl: https://github.com/brianmario/yajl-ruby/pull/128

Comment 17 Julian C. Dunn 2014-06-27 21:30:01 UTC
Since Brian Lopez seems to not want the devendorization patches against yajl-ruby, we (Chef Software, Inc.) have rewritten our own FFI wrapper around libyajl2 to get around this vendoring issue. Future versions of Chef will use ffi-yajl instead of both yajl-ruby and the JSON gem, so I have submitted bz#1114146 as a precursor to getting Chef into Fedora.

Comment 18 Vít Ondruch 2016-09-08 12:05:22 UTC
*** Bug 1373005 has been marked as a duplicate of this bug. ***

Comment 20 Sandro Bonazzola 2017-11-23 14:19:59 UTC
Matthias can you please review?
Also pushed to https://review.rdoproject.org/r/#/c/10681/

Comment 21 Matthias Runge 2017-12-04 12:46:13 UTC
Sandro, you -2'ed the RDO review. I guess, this is not necessary anymore?

Comment 22 Sandro Bonazzola 2018-01-09 16:52:55 UTC
(In reply to Matthias Runge from comment #21)
> Sandro, you -2'ed the RDO review. I guess, this is not necessary anymore?

It's still needed but the RDO review patch is not working:
Fails functional testing on fedora. I get: Ignoring yajl-ruby-1.3.1 because its extensions are not built.  Try: gem pristine yajl-ruby --version 1.3.1


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