Bug 632912 - Review Request: rubygem-robots - Simple robots.txt parser
Summary: Review Request: rubygem-robots - Simple robots.txt parser
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 553898 624290 632917
Blocks: 632919
TreeView+ depends on / blocked
 
Reported: 2010-09-11 20:50 UTC by Michal Ambroz
Modified: 2012-04-17 02:13 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-04-17 02:13:08 UTC
Type: ---
Embargoed:
mtasaka: fedora-review?


Attachments (Terms of Use)

Description Michal Ambroz 2010-09-11 20:50:18 UTC
SPEC URL: http://rebus.fedorapeople.org/SPECS/rubygem-robots.spec
SRPM URL: http://rebus.fedorapeople.org/SRPMS/rubygem-robots-0.10.0-1.fc13.src.rpm

Description:
A simple Ruby library to parse robots.txt.

Hello,
Please could you review the package whatweb?

Output from rpmlint:
$ rpmlint rubygem-robots-0.10.0-1.fc13.src.rpm rubygem-robots-0.10.0-1.fc13.noarch.rpm
#txt is in the reference to the name of the robots.txt file
rubygem-robots.src: W: spelling-error Summary(en_US) txt -> text, ext, tit
rubygem-robots.src: W: spelling-error %description -l en_US txt -> text, ext, tit
rubygem-robots.noarch: W: spelling-error Summary(en_US) txt -> text, ext, tit
rubygem-robots.noarch: W: spelling-error %description -l en_US txt -> text, ext, tit

#% is part urlencoded name of documentation file 
rubygem-robots.noarch: W: unexpanded-macro /usr/lib/ruby/gems/1.8/doc/robots-0.10.0/ri/Robots/ParsedRobots/allowed%3f-i.yaml %3f
rubygem-robots.noarch: W: unexpanded-macro /usr/lib/ruby/gems/1.8/doc/robots-0.10.0/ri/Robots/timeout%3d-c.yaml %3d
rubygem-robots.noarch: W: unexpanded-macro /usr/lib/ruby/gems/1.8/doc/robots-0.10.0/ri/Robots/allowed%3f-i.yaml %3f

#% test scripts are commonly set with only 644 permission
rubygem-robots.noarch: E: non-executable-script /usr/lib/ruby/gems/1.8/gems/robots-0.10.0/test/test_robots.rb 0644L /usr/bin/env

Koji F13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2462027

Comment 1 Mamoru TASAKA 2010-09-12 18:15:47 UTC
Some notes:

* %define -> %global
  - Now we prefer to use %global instead of %define:
    https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

* BuildRoot
  - BuildRoot tag is no longer used on Fedora and EPEL6:
    https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

* ruby(abi) Requires
  - Writing "R: ruby(abi) = 1.8" is a must
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines

* Requires
  - R: rubygem(shoulda) is not needed
    - thoughtbot-shoulda (not shoulda) dependency appeas in installed gemspec,
      however this is for development dependency and is not needed on runtime.

* Duplicate %files entry
----------------------------------------------------------------
    58  warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/robots-0.10.0/CHANGELOG
    59  warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/robots-0.10.0/README
----------------------------------------------------------------
  - Please make it sure that all files are listed only once in %files
    entry.
  ! Note
    The %files entry
----------------------------------------------------------------
%files
/some_dir/
----------------------------------------------------------------
    contains the directory /some_dir/ itself and all files/directories/etc
    under /some_dir/ , while
----------------------------------------------------------------
%files
%dir /some_dir/
----------------------------------------------------------------
    contains the directory /some_dir/ only.

* rpmlint issue
----------------------------------------------------------------
rubygem-robots.noarch: E: non-executable-script /usr/lib/ruby/gems/1.8/gems/robots-0.10.0/test/test_robots.rb 0644L /usr/bin/env
----------------------------------------------------------------
  - This script need not have shebang and the shebang should be removed.

* %check
  - As this gem contains test/ directory, please add %check section and execute
    some test program there
    ! Note
      To make "$ rake test" succeed, some modification against Rakefile is
      needed.

Comment 2 Mamoru TASAKA 2010-09-12 18:17:43 UTC
One more comment
* Macros
  - Please use the defined %{geminstdir} macro consistently also
    in %files

Comment 3 Mamoru TASAKA 2010-09-22 17:19:23 UTC
ping?

Comment 4 Michal Ambroz 2010-09-25 19:49:53 UTC
Hello Mamoru,
thank you for review, here is the updated package:
SPEC URL: http://rebus.fedorapeople.org/SPECS/rubygem-robots.spec
SRPM URL:
http://rebus.fedorapeople.org/SRPMS/rubygem-robots-0.10.0-2.fc13.src.rpm

I have tried to implement changes based on your comments. Unfortunately I was not able to implement all the changes you have been asking - mainly:

* BuildRoot
- having this tag is not an error - I would like to keep the package maintained for EPEL5 as well and I would prefer to have 1 spec if possible.

* %check
- some packages needed for test are not available in Fedora yet (shoulda, jeweler), I have included BuildRequire(check) dependencies and check section, but it is now commented out. I will uncomment as soon as the dependencies are available.

Best regards
Michal Ambroz

Comment 5 Mamoru TASAKA 2010-10-03 19:22:36 UTC
Sorry for late response.
Well,

* %check issue
  - rubygem(jeweler) is not strictly needed for $ rake test
  - also rubygem(shoulda) is only for gem.add_development_dependency and
    not needed for $ rake test
  - However Rakefile itself is broken for $ rake test.
    Please consider to apply the following patch and enable $ rake test on %check.
-----------------------------------------------------------------
$ diff -u Rakefile.debug Rakefile
--- Rakefile.debug	1970-01-01 09:00:00.000000000 +0900
+++ Rakefile	2010-09-13 02:32:52.000000000 +0900
@@ -21,7 +21,7 @@
 require 'rake/testtask'
 Rake::TestTask.new(:test) do |test|
   test.libs << 'lib' << 'test'
-  test.pattern = 'test/**/*_test.rb'
+  test.pattern = 'test/**/test_*.rb'
   test.verbose = true
 end
 
@@ -38,7 +38,7 @@
   end
 end
 
-task :test => :check_dependencies
+#task :test => :check_dependencies
 
 task :default => :test
 
-----------------------------------------------------------------

* Creating -doc subpackage
  - For rubygem based rpms, We usually suggest to create -doc subpackages,
    make -doc subpackage have "Requires: %{name} = %{version}-%{release}"
    and move the following files / directories to -doc subpackage.
----------------------------------------------------------------
%{gemdir}/doc/%{gemname}-%{version}
%{geminstdir}/Rakefile
%{geminstdir}/test
----------------------------------------------------------------

Comment 6 Mamoru TASAKA 2010-10-14 18:09:27 UTC
ping?

Comment 7 Mamoru TASAKA 2010-10-29 17:38:10 UTC
ping again?

Comment 8 Mamoru TASAKA 2010-11-05 17:31:44 UTC
Again ping?

Comment 9 Mamoru TASAKA 2010-11-15 16:46:26 UTC
ping?

Comment 10 Michal Ambroz 2011-04-11 15:18:07 UTC
Hello Mamoru,
I am sorry - I forgot abuot the package. 
I will incorporate your suggestions.

Comment 11 Michal Ambroz 2011-04-11 23:42:25 UTC
Updated package:

SPEC: http://rebus.fedorapeople.org/SPECS/rubygem-robots.spec
SRPM: http://rebus.fedorapeople.org/SRPMS/rubygem-robots-0.10.0-3.fc14.src.rpm

- added suggested patch
- split doc package
- enabled %check

Best regards
Michal Ambroz

Comment 12 Michal Ambroz 2012-04-17 02:13:08 UTC
Resigning in getting the anemone/robots to Fedora.


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