Bug 504424
| Summary: | Review Request: rubygem-json - A JSON implementation in Ruby | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Xavier Lamien <lxtnow> |
| Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | fedora-package-review, notting |
| Target Milestone: | --- | Flags: | mtasaka:
fedora-review+
j: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2009-08-14 19:36:48 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Xavier Lamien
2009-06-06 21:05:21 UTC
Whoops, copy paste error. 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) 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
ping? Pong. I'm going to rebuild it to rubygem-json. ping? here the new package: Spec URL: http://laxathom.fedorapeople.org/RPMS/ruby-json/rubygem-json.spec SRPM URL: http://laxathom.fedorapeople.org/RPMS/ruby-json/rubygem-json-1.1.7-1.fc10.src.rpm 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. 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 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}. * 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 Okay. -------------------------------------------------------- This package (rubygem-json) is APPROVED by mtasaka -------------------------------------------------------- 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) CVS done. Imported and built. Thx guys. |