https://bugzilla.redhat.com/show_bug.cgi?id=823351
Bug ID: 823351 QA Contact: extras-qa@fedoraproject.org Severity: unspecified Version: rawhide Priority: unspecified CC: notting@redhat.com, package-review@lists.fedoraproject.org Assignee: nobody@fedoraproject.org Summary: Review Request: rubygem-yajl-ruby Regression: --- Story Points: --- Classification: Fedora OS: Unspecified Reporter: rpms@courteau.org Type: Bug Documentation: --- Hardware: Unspecified Mount Type: --- Status: NEW Component: Package Review Product: Fedora
Spec URL: https://raw.github.com/jcourteau/rubygems-rpms/master/fc17/rubygem-yajl-ruby... 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.
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Michael Schwendt mschwendt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |823352
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Michael Schwendt mschwendt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Jonas Courteau rpms@courteau.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: |Review Request: |rubygem-yajl-ruby |rubygem-yajl-ruby - Ruby C | |bindings to YAJL - a JSON | |stream-based parser
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Jonas Courteau rpms@courteau.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841 |823344
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
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 it for a review, since it is dependency of rubygem-chef. Not sure if Jonas is active though.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
--- Comment #2 from Vít Ondruch vondruch@redhat.com --- * 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
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
--- Comment #3 from Julian C. Dunn jdunn@aquezada.com --- 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.f... 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?
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
Michael Scherer misc@zarb.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |misc@zarb.org
--- Comment #4 from Michael Scherer misc@zarb.org --- Yep, the bundled is a showstopper. But we can proceed on the other review in the mean time.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(jstribny@redhat.c | |om)
--- Comment #5 from Vít Ondruch vondruch@redhat.com --- Josef Stribny is trying to make yajl work with Fedora's yajl. Hopefully he will provide use with some update soon.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
Josef Stribny jstribny@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jstribny@redhat.com Flags|needinfo?(jstribny@redhat.c | |om) |
--- Comment #6 from Josef Stribny jstribny@redhat.com --- 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).
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
--- Comment #7 from Josef Stribny jstribny@redhat.com --- 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
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
--- Comment #8 from Vít Ondruch vondruch@redhat.com --- 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
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
Vaidas Jablonskis jablonskis@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jablonskis@gmail.com
--- Comment #9 from Vaidas Jablonskis jablonskis@gmail.com --- 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.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
--- Comment #10 from Vít Ondruch vondruch@redhat.com --- (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.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=823351
Allen Hewes allen@decisiv.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC|package-review@lists.fedora | |project.org | CC| |allen@decisiv.net
https://bugzilla.redhat.com/show_bug.cgi?id=823351
--- Comment #13 from Ken Dreyer ktdreyer@ktdreyer.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Ken Dreyer ktdreyer@ktdreyer.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(jdunn@aquezada.co | |m)
--- Comment #14 from Ken Dreyer ktdreyer@ktdreyer.com --- 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/commi...
F21 scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6533809
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Julian C. Dunn jdunn@aquezada.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(jdunn@aquezada.co | |m) |
--- Comment #15 from Julian C. Dunn jdunn@aquezada.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=823351
--- Comment #16 from Vít Ondruch vondruch@redhat.com --- Just for the record, it seems tha yajl-ruby might be renamed to yajl: https://github.com/brianmario/yajl-ruby/pull/128
https://bugzilla.redhat.com/show_bug.cgi?id=823351
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=823351
--- Comment #17 from Julian C. Dunn jdunn@aquezada.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=823351
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=823351
Vít Ondruch vondruch@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |yguenane@redhat.com
--- Comment #18 from Vít Ondruch vondruch@redhat.com --- *** Bug 1373005 has been marked as a duplicate of this bug. ***
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Sandro Bonazzola sbonazzo@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugzilla.redhat.com | |/show_bug.cgi?id=1426110
https://bugzilla.redhat.com/show_bug.cgi?id=823351
--- Comment #19 from Sandro Bonazzola sbonazzo@redhat.com --- Bumped to 1.3.1. Spec URL: http://copr-dist-git.fedorainfracloud.org/cgit/sbonazzo/ovirt-fluentd/rubyge... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/sbonazzo/ovirt-fluentd/fedor...
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Sandro Bonazzola sbonazzo@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- External Bug ID| |RDO 10681 Flags| |needinfo?(mrunge@redhat.com | |)
--- Comment #20 from Sandro Bonazzola sbonazzo@redhat.com --- Matthias can you please review? Also pushed to https://review.rdoproject.org/r/#/c/10681/
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Sandro Bonazzola sbonazzo@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1460625 (oVirt_on_Fedora)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1460625 [Bug 1460625] [Tracker] Fedora support
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Matthias Runge mrunge@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(mrunge@redhat.com |needinfo?(sbonazzo@redhat.c |) |om)
--- Comment #21 from Matthias Runge mrunge@redhat.com --- Sandro, you -2'ed the RDO review. I guess, this is not necessary anymore?
https://bugzilla.redhat.com/show_bug.cgi?id=823351
Sandro Bonazzola sbonazzo@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(sbonazzo@redhat.c | |om) |
--- Comment #22 from Sandro Bonazzola sbonazzo@redhat.com --- (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
package-review@lists.fedoraproject.org