Bug 504424 - Review Request: rubygem-json - A JSON implementation in Ruby
Review Request: rubygem-json - A JSON implementation in Ruby
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-06 17:05 EDT by Xavier Lamien
Modified: 2009-08-14 15:36 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-08-14 15:36:48 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Xavier Lamien 2009-06-06 17:05:21 EDT
Spec URL: http://laxathom.fedorapeople.org/RPMS/ruby-json/ruby-json.spec
SRPM URL: http://laxathom.fedorapeople.org/RPMS/ruby-json/ruby-json-1.1.6-1.fc10.src.rpm

Description:
This is a implementation of the JSON specification according to RFC 4627 in Ruby.
You can think of it as a low fat alternative to XML, if you want to store
data to disk or transmit it over a network rather than use a verbose
markup language.
Comment 1 Susi Lehtola 2009-06-10 16:28:42 EDT
Whoops, copy paste error.
Comment 2 Mamoru TASAKA 2009-06-20 12:35:42 EDT
Will review shortly. Instead I will appreciate it if you would
review either of my review requests (bug 504707, bug 504710,
bug 505406 or bug 506168)
Comment 3 Mamoru TASAKA 2009-06-20 13:15:09 EDT
Some notes:

* native ruby module vs rubygem
  - As json rubygem is available, would you consider to use
    json rubygem to package rpm instead of using native tarball?
    ref:
    https://fedoraproject.org/wiki/Packaging/Ruby#Packaging_for_Gem_and_non-Gem_use


* License
  - Should be "Ruby or GPLv2"

* BR
  - You should use "BR: rubygem(rake)" instead of "BR: rubygem-rake"
    like perl. Ref:
    https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides

! Requires
  ! json/editor.rb requires ruby(gtk2). Should "ruby(gtk2)" be added as
    the Requires of this package or should this script be separated into
    a subpackage?

* Stripping binaries
  * json/ext/*.so should not be stripped to create debuginfo rpm
    correctly
Comment 4 Mamoru TASAKA 2009-07-11 12:29:37 EDT
ping?
Comment 5 Xavier Lamien 2009-07-12 07:28:28 EDT
Pong.
I'm going to rebuild it to rubygem-json.
Comment 6 Mamoru TASAKA 2009-07-27 11:12:40 EDT
ping?
Comment 8 Mamoru TASAKA 2009-08-04 12:26:20 EDT
rubygem-json-1.1.7-1.fc10 does not build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1580525

Some notes:
* SourceURL
  - For gems, you can use:
    http://gems.rubyforge.org/gems/<gemname>-<version>.gem
    and this URL is easier.

* Gem installation process
  - The main reason the build fails like:
---------------------------------------------------
Binary file /builddir/build/BUILDROOT/rubygem-json-1.1.7-1.fc12.x86_64/usr/lib/ruby/gems/1.8/gems/json-1.1.7/ext/json/ext/generator/unicode.o matches
---------------------------------------------------
    is because you installs gem file under %buildroot instead
    of creating directory under %_builddir and installing gem
    file under there [1]
    This is also mentioned in current ruby guidelines [2]
    For example, please refer to rubygem-nokogiri spec file [3]
    
[1] https://www.redhat.com/archives/fedora-devel-list/2009-July/msg00929.html
[2] https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Gem_with_extension_libraries_written_in_C
[3] http://cvs.fedoraproject.org/viewvc/rpms/rubygem-nokogiri/devel/rubygem-nokogiri.spec?revision=1.11

* Enable tests
  - As this gem file installs some tests under %geminstdir/tests/,
    please enable tests (also see the example [3])

* Documents directories
  - Documents installed under %geminstdir should just be marked as %doc
    and should not be moved to under %_defaultdocdir because
    $ gem contents <gemname> expects that those files should be under
    %geminstdir.
Comment 9 Xavier Lamien 2009-08-05 10:54:50 EDT
gaahh, good point !

Ok, here's an update.
I disabled test for now as failling on parser.rl (I need a closer look on that on)

http://laxathom.fedorapeople.org/RPMS/ruby-json/rubygem-json.spec
http://laxathom.fedorapeople.org/RPMS/ruby-json/rubygem-json-1.1.7-2.fc10.src.rpm
Comment 10 Mamoru TASAKA 2009-08-05 14:06:33 EDT
For 1.1.7-2:

* URL
  - Would you explain why you prefer to use http://rubyforge.org/frs/download.php/...
    instead of the URL I wrote in my previous comment (note that the repoid
    "59617" changes randomly every time the version is upgraded)

? Non-gem support
  - It is okay to add non-gem support, however would you explain
    why you want to add this for json (while ruby-json is not in
    Fedora yet)?

    ( And perhaps 

* Installation directory under %_builddir
-----------------------------------------------------------
pushd ./%{gemdir}
gem install --local --install-dir ./%{gemdir} -V --force %{SOURCE0}
-----------------------------------------------------------
  - So gem file is installed under 
    %_builddir/%{name}-%{version}/%{gemdir}/%{gemdir}, which should
    be %_builddir/%{name}-%{version}/%{gemdir}
    ( The first line of "pushd" is not needed )

* gem script files under bin/
-----------------------------------------------------------
mv $RPM_BUILD_ROOT/%{gemdir}/bin/* $RPM_BUILD_ROOT/%{geminstdir}/bin
-----------------------------------------------------------
  - Well, actually the scripts under %gemdir/bin and under %geminstdir/bin
    are different and both scripts are needed. The scripts under %{gemdir}/bin
    load the file under %{geminstdir}/bin.

    While we usually move the scripts under %{gemdir}/bin to %{_bindir}
    like:
-----------------------------------------------------------
mkdir %{buildroot}%{_bindir}
mv %{buildroot}%{gemdir}/bin/* %{buildroot}%{_bindir}
rmdir %{buildroot}%{gemdir}
-----------------------------------------------------------
    the scripts under %{geminstdir}/bin should not be modified (except
    for shebang issues or so)

* %check
  - The reason %check fails for you is that you are trying to do
    "rake --test" under %{buildroot}%{geminstdir}, where some
    files needed for tests (like ext/) are already cleaned up.

    The correct way is to call "rake --test" under 
    %_builddir/%name-%version/%{geminstdir}.
Comment 11 Xavier Lamien 2009-08-12 17:28:06 EDT
* URL
I've no preference, i just omitted to fix that url.

* Non-gem support
We have some codes which not use Ruby gem.
for remind, my first approach was to add ruby-json to fpc and
currently have no objection to add and maintain both.

* gem script fixed

* %check fixed and passed.

New updates:
http://laxathom.fedorapeople.org/RPMS/ruby-json/rubygem-json.spec
http://laxathom.fedorapeople.org/RPMS/ruby-json/rubygem-json-1.1.7-3.fc10.src.rpm
Comment 12 Mamoru TASAKA 2009-08-13 04:35:45 EDT
Okay.

--------------------------------------------------------
  This package (rubygem-json) is APPROVED by mtasaka
--------------------------------------------------------
Comment 13 Xavier Lamien 2009-08-13 07:45:45 EDT
Thanks Mamoru.


New Package CVS Request
=======================
Package Name:       rubygem-json
Short Description:  A JSON implementation in Ruby
Owners:             laxathom
Branches:           F-10 F-11 EL-5
InitialCC:          (nobody)
Comment 14 Jason Tibbitts 2009-08-14 12:14:45 EDT
CVS done.
Comment 15 Xavier Lamien 2009-08-14 15:36:48 EDT
Imported and built.

Thx guys.

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