Bug 541491 - Review Request: rubygem-ruby_parser - A ruby parser written in pure ruby
Review Request: rubygem-ruby_parser - A ruby parser written in pure ruby
Status: CLOSED ERRATA
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: 541185
Blocks: 541512
  Show dependency treegraph
 
Reported: 2009-11-25 22:33 EST by Matthew Kent
Modified: 2010-09-11 15:46 EDT (History)
3 users (show)

See Also:
Fixed In Version: 2.0.4-3.fc12
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-04 10:30:43 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Matthew Kent 2009-11-25 22:33:22 EST
Spec URL: http://magoazul.com/wip/SPECS/rubygem-ruby_parser.spec
SRPM URL: http://magoazul.com/wip/SRPMS/rubygem-ruby_parser-2.0.4-1.fc13.src.rpm
Description: ruby_parser (RP) is a ruby parser written in pure ruby (utilizing racc - which does by default use a C extension). RP's output is the same as ParseTree's output: s-expressions using ruby's arrays and base types.

mkent@fedora-devel-chef:~/rpmbuild/SPECS$ rpmlint rubygem-ruby_parser.spec /var/tmp/results/rubygem-ruby_parser-*
rubygem-ruby_parser-doc.noarch: W: no-documentation
rubygem-ruby_parser-doc.noarch: W: misspelled-macro /usr/lib/ruby/gems/1.8/doc/ruby_parser-2.0.4/ri/RubyParser/Environment/dynamic%3f-i.yaml %3f
rubygem-ruby_parser-doc.noarch: W: misspelled-macro /usr/lib/ruby/gems/1.8/doc/ruby_parser-2.0.4/ri/RubyParser/Environment/used%3f-i.yaml %3f
rubygem-ruby_parser-doc.noarch: W: misspelled-macro /usr/lib/ruby/gems/1.8/doc/ruby_parser-2.0.4/ri/RubyParser/Environment/%5b%5d%3d-i.yaml %5b
rubygem-ruby_parser-doc.noarch: W: misspelled-macro /usr/lib/ruby/gems/1.8/doc/ruby_parser-2.0.4/ri/RubyLexer/src%3d-i.yaml %3d
rubygem-ruby_parser-doc.noarch: W: misspelled-macro /usr/lib/ruby/gems/1.8/doc/ruby_parser-2.0.4/ri/RubyLexer/lex_state%3d-i.yaml %3d
rubygem-ruby_parser-doc.noarch: W: misspelled-macro /usr/lib/ruby/gems/1.8/doc/ruby_parser-2.0.4/ri/RubyParserGauntlet/should_skip%3f-i.yaml %3f
rubygem-ruby_parser-doc.noarch: W: misspelled-macro /usr/lib/ruby/gems/1.8/doc/ruby_parser-2.0.4/ri/RubyParser/Environment/%5b%5d-i.yaml %5b
rubygem-ruby_parser-doc.noarch: W: misspelled-macro /usr/lib/ruby/gems/1.8/doc/ruby_parser-2.0.4/ri/RPStringScanner/begin_of_line%3f-i.yaml %3f
rubygem-ruby_parser-doc.noarch: W: hidden-file-or-dir /usr/lib/ruby/gems/1.8/gems/ruby_parser-2.0.4/.autotest
3 packages and 1 specfiles checked; 0 errors, 10 warnings.
Comment 1 Mamoru TASAKA 2009-11-28 10:58:51 EST
Some notes:

* Explicit version dependency
  - ">= 3.0" on Requires: rubygem(sexp_processor) is redundant
    because all rubygem-sexp_processor shipped on Fedora satisfies
    this version dependency:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires

* %check
----------------------------------------------------------------
    16  # These test cases are carried in the ParseTree gem in test/. Carry them here
    17  # rather than attempting to install ParseTree-doc in check and introducing a circular
    18  # dependency
    19  Source1: pt_testcase.rb
    79  %check
    80  pushd .%{geminstdir}
    81  cp %{SOURCE1} test/
    82  rake test
----------------------------------------------------------------
  - IMO if this script is really needed for "rake test" (and actually
    it seems so), this script should also be included in the rebuilt
    binary rpm (i.e. better to move the lines 80-81 to %build).

? Dependency loop
  - lib/gauntlet_rubyparser.rb contains:
----------------------------------------------------------------
     8  require 'rubygems'
     9  require 'ruby2ruby'
    10  require 'ruby_parser'
    11  
    12  require 'gauntlet'
----------------------------------------------------------------
    i.e. this script needs two other gems: "ruby2ruby" "gauntlet"
    - The formar one causes dependency loop
    - The latter one is not found on Fedora (even on review request)
    Can this dependency (rather, this script) be ignored?
Comment 2 Matthew Kent 2009-11-30 01:51:49 EST
Thank you for the review.

(In reply to comment #1)
> Some notes:
> 
> * Explicit version dependency
>   - ">= 3.0" on Requires: rubygem(sexp_processor) is redundant
>     because all rubygem-sexp_processor shipped on Fedora satisfies
>     this version dependency:
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires
> 

Noted, thanks.

> * %check
> ----------------------------------------------------------------
>     16  # These test cases are carried in the ParseTree gem in test/. Carry
> them here
>     17  # rather than attempting to install ParseTree-doc in check and
> introducing a circular
>     18  # dependency
>     19  Source1: pt_testcase.rb
>     79  %check
>     80  pushd .%{geminstdir}
>     81  cp %{SOURCE1} test/
>     82  rake test
> ----------------------------------------------------------------
>   - IMO if this script is really needed for "rake test" (and actually
>     it seems so), this script should also be included in the rebuilt
>     binary rpm (i.e. better to move the lines 80-81 to %build).
> 

Good idea.

> ? Dependency loop
>   - lib/gauntlet_rubyparser.rb contains:
> ----------------------------------------------------------------
>      8  require 'rubygems'
>      9  require 'ruby2ruby'
>     10  require 'ruby_parser'
>     11  
>     12  require 'gauntlet'
> ----------------------------------------------------------------
>     i.e. this script needs two other gems: "ruby2ruby" "gauntlet"
>     - The formar one causes dependency loop
>     - The latter one is not found on Fedora (even on review request)
>     Can this dependency (rather, this script) be ignored?  

Ah you noticed this as well, I should have added some notes inline.

I chose to ignore it's dependencies as the developer didn't add it as either a primary or development dependency in the Rakefile. It's also not a library as far as I can tell, more of a script used in testing, and not included/invoked in any of the unit testing.

Plus looking at the gauntlet gem itself it's clearly geared toward library development rather than providing functionality to a library.

What's the correct approach here, %exclude the script? Add a note about it and leave dependencies as is?

Will go on the assumption you'd rather it be excluded.
Comment 3 Matthew Kent 2009-11-30 01:53:08 EST
Spec URL: http://magoazul.com/wip/SPECS/rubygem-ruby_parser.spec
SRPM URL: http://magoazul.com/wip/SRPMS/rubygem-ruby_parser-2.0.4-2.fc13.src.rpm

* Sun Nov 29 2009 Matthew Kent <mkent@magoazul.com> - 2.0.4-2
- Move pt_testcase.rb to the build stage so it's included in the rpm (#541491).
- Drop version requirements for sexp_processor as it is a new package
  (#541491).
- Exclude gauntlet_rubyparser.rb as it introduces a circular dependency
  (#541491).
Comment 4 Mamoru TASAKA 2009-11-30 02:08:57 EST
(In reply to comment #2)
> What's the correct approach here, %exclude the script? Add a note about it and
> leave dependencies as is?
> 
> Will go on the assumption you'd rather it be excluded.  

- If you are sure that no one would need this script, then simply
  exclude the script. However if there may be some people who wants
  this script, rather leave this script as it is (and also leave
  the dependency as it is). Those who want to use this script can
  install the needed dependency by him/herself
  ( note that I don't know how this script is to be used, so I would
    keep this script as it is )
Comment 5 Matthew Kent 2009-11-30 04:33:29 EST
Spec URL: http://magoazul.com/wip/SPECS/rubygem-ruby_parser.spec
SRPM URL:
http://magoazul.com/wip/SRPMS/rubygem-ruby_parser-2.0.4-2.fc13.src.rpm
(In reply to comment #4)
> (In reply to comment #2)
> > What's the correct approach here, %exclude the script? Add a note about it and
> > leave dependencies as is?
> > 
> > Will go on the assumption you'd rather it be excluded.  
> 
> - If you are sure that no one would need this script, then simply
>   exclude the script. However if there may be some people who wants
>   this script, rather leave this script as it is (and also leave
>   the dependency as it is). Those who want to use this script can
>   install the needed dependency by him/herself
>   ( note that I don't know how this script is to be used, so I would
>     keep this script as it is )  

Yeah I suppose I can't be certain no one would use it. I'll leave it in.
Comment 6 Matthew Kent 2009-11-30 04:34:02 EST
Spec URL: http://magoazul.com/wip/SPECS/rubygem-ruby_parser.spec
SRPM URL: http://magoazul.com/wip/SRPMS/rubygem-ruby_parser-2.0.4-3.fc13.src.rpm

* Mon Nov 30 2009 Matthew Kent <mkent@magoazul.com> - 2.0.4-3
- Remove exclude for gauntlet_rubyparser.rb, let user deal with dependencies if
  they need it.
Comment 7 Mamoru TASAKA 2009-11-30 12:42:21 EST
One comment:
-----------------------------------------------------
+# Script creates a circular dependency and is primarily for development
+# Included, but it's dependencies aren't met.
+%{geminstdir}/lib/gauntlet_rubyparser.rb
-----------------------------------------------------
- This %files entry causes duplicate %files entry:
-----------------------------------------------------
   120  warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/ruby_parser-2.0.4/lib/gauntlet_rubyparser.rb
-----------------------------------------------------
  as this file is already included in
-----------------------------------------------------
%{geminstdir}/lib
-----------------------------------------------------
  Just leave the line as a comment and avoid duplicate %files
  entry.

--------------------------------------------------------------
  This package (rubygem-ruby_parser) is APPROVED by mtasaka
--------------------------------------------------------------
Comment 8 Mamoru TASAKA 2009-11-30 14:23:12 EST
One more thing
* Timestamp
  - Use "cp -p" instead of just "cp" to keep timestamps on installed
    files:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

Still approved.
Comment 9 Matthew Kent 2009-12-01 00:22:44 EST
New Package CVS Request
=======================
Package Name: rubygem-ruby_parser
Short Description: A ruby parser written in pure ruby
Owners: mkent
Branches: F-11 F-12
InitialCC:
Comment 10 Kevin Fenzi 2009-12-03 01:46:54 EST
cvs done.
Comment 11 Fedora Update System 2009-12-04 01:08:58 EST
rubygem-ruby_parser-2.0.4-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/rubygem-ruby_parser-2.0.4-3.fc11
Comment 12 Fedora Update System 2009-12-04 01:09:43 EST
rubygem-ruby_parser-2.0.4-3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/rubygem-ruby_parser-2.0.4-3.fc12
Comment 13 Mamoru TASAKA 2009-12-04 10:30:43 EST
Closing
Comment 14 Fedora Update System 2009-12-04 18:40:24 EST
rubygem-ruby_parser-2.0.4-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 15 Fedora Update System 2009-12-04 18:59:21 EST
rubygem-ruby_parser-2.0.4-3.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 16 Michael Stahnke 2010-09-10 16:35:36 EDT
Package Change Request
======================
Package Name: rubygem-ruby_parser
New Branches: el5 el6
Owners: stahnma

mkent is aware of request.
Comment 17 Kevin Fenzi 2010-09-11 15:46:45 EDT
Git done (by process-git-requests).

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