Bug 601160 - Review Request: rubygem-rdiscount - Fast Implementation of Gruber's Markdown in C
Summary: Review Request: rubygem-rdiscount - Fast Implementation of Gruber's Markdown ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL: http://github.com/rtomayko/rdiscount
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-06-07 11:28 UTC by Gerd Pokorra
Modified: 2010-06-21 21:43 UTC (History)
3 users (show)

Fixed In Version: rubygem-rdiscount-1.6.3.2-4.fc13
Clone Of:
Environment:
Last Closed: 2010-06-21 21:28:20 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Gerd Pokorra 2010-06-07 11:28:46 UTC
Spec URL: ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount.spec

SRPM URL: 
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount-1.6.3.2-1.fc12.src.rpm

Description: Discount is an implementation of John Gruber's Markdown markup language in C. It implements all of the language described in the markdown syntax document and passes the Markdown 1.0 test suite.

Successful scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2235208

Comment 1 Mamoru TASAKA 2010-06-07 19:04:29 UTC
Some initial notes:

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

* debuginfo rpm
  - Please don't set "%define debug_package %{nil}" and create
    debuginfo rpm correctly

* License
  - As far as I checked the source codes, the license tag should
    be "ASL 1.1".

* ruby(abi) dependency
  - For ruby module packages, writing "R: ruby(abi) = 1.8" is
    mandatory on Fedora:
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines

* BuildRoot no longer needed
  - For Fedora (not for EPEL), BuildRoot tag is no longer needed:
    https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

* redundant "exit 0"
  - You needed not redundant "exit 0" at the end of
    %prep, %build, %install (not: %prep, %build, %install stage execute
    shell script with "/bin/sh -e")

* Some generic packaging issue for rubygems containing C extension modules
  - Arch-dependent files (like rdiscount.so) must be moved to under
    %ruby_sitearch:
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_packages_with_binary_content.2Fshared_libraries

  - Files under ext/ directory (and ext/ directory itself) are needed
    to generate C extension module file and should not be needed on
    runtime.
    i.e. ext/ directory should not be in generated binary rpm.

* Marking files as %doc
  - Please mark document files as %doc properly
    - (COPYING and) README.markdown Rakefile should be marked as %doc
      ( note that Rakefile is something like makefiles in autotool based
        packages )
    - man/ test/ directories should also be marked as %doc
    - Also I usually suggest to create -doc subpackage and move
      * Rakefile
      * man/ test/ directories
      * %{gemdir}/doc/%{gemname}-%{version}
      to -doc subpackage
    - By the way
        * I think rdiscount.1 man file should be moved to %_mandir/man1
        * I am not sure if markfile.7 should also be moved to %_mandir/man7
          or not.

* Duplicate %files entry
  - Please make it sure that every file/directory/etc is listed only once
    in %files entry. Currently build.log shows:
-------------------------------------------------------------------------
   164  Processing files: rubygem-rdiscount-1.6.3.2-1.fc14.i686
   165  warning: File listed twice: /usr/lib/ruby/gems/1.8/gems/rdiscount-1.6.3.2/COPYING
-------------------------------------------------------------------------

* Enabling test
  - As this gem contains test/ directory, please add %check section
    and execute some test program (like $ rake test) there.

Comment 2 Gerd Pokorra 2010-06-08 13:30:40 UTC
Thank you for the first look. Would you please make a second look?
I did not increment the revision (release) number.
I only overwrite the files at Spec URL: 

ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount.spec

and SRPM URL:
 
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount-1.6.3.2-1.fc12.src.rpm

The URL from the scratch build of the current files is:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2238352

I do the best I can for "Enabling tests". May be you know a better solution for that.

Comment 3 Mamoru TASAKA 2010-06-08 13:49:35 UTC
fedora-review flag must be set by reviewer (assignee), once
resetting. Also would you bump release number to avoid confusion?
(and bumping release number is helpful when using diff)

Comment 4 Gerd Pokorra 2010-06-08 17:11:12 UTC
I am sorry for the mistake. I was in a hurry because I had an appointment and my last review request is some time ago.

I increment the release number in the spec file, it is still stored under the URL:
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount.spec

The new SRPM is at:
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount-1.6.3.2-2.fc12.src.rpm

Scratch build URL:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2238903

Comment 5 Mamoru TASAKA 2010-06-10 18:08:34 UTC
Well,

* ruby(abi) dependency
  - must be strict equality (i.e. R: ruby(abi) = 1.8)

* rubygem module related dependency
  - For rubygem module related dependency, please use 
    "(Build)Requires: rubygem(foo)" style.

    ref: (althogh this for perl guideline:)
    https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides

* Dependency for -doc subpackage
  - -doc subpackage should depend on main package for directory
    ownership issue (also see below)

* Directory for non-arch-dependent ruby scripts
  - As I said, arch-dependent files (like C extentsion modules) must
    be moved to %ruby_sitearch.

    On the other hand, non-arch-dependent ruby scripts must not
    be moved from %geminstdir.

* Movement of files under %_builddir
  - Currently build.log says in %check section:
--------------------------------------------------------------
   199  + rake test
   200  rm -f 
   201  touch ext/ruby-54f732744bed984d9d0d133997bf4128
   202  cd ext
   203  (in /builddir/build/BUILD/rubygem-rdiscount-1.6.3.2/usr/lib/ruby/gems/1.8/gems/rdiscount-1.6.3.2)
   204  /usr/bin/ruby extconf.rb
   205  checking for random()... yes
   206  checking for srandom()... yes
....
....
   211  cd ext && make clean && make
   212  gcc -I. -I. -I/usr/lib/ruby/1.8/i386-linux -I. -DHAVE_RANDOM -DHAVE_SRANDOM -DHAVE_RAND -DHAVE_SRAND  -D_FILE_OFFSET_BITS=64 -fPIC -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -fno-strict-aliasing  -fPIC  -c Csio.c
....
   264  cp -p ext/rdiscount.so lib/
   265  /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:31:in `gem_original_require': no such file to load -- markdown (LoadError)
--------------------------------------------------------------
    So currently "rake test"
    - tries to compile rdiscont.so again which was actually built
      in %build
    - and finally fails because lib/markdown.rb is not found (under
      %_builddir/%gemname-%version/%geminstdir).

    This is because rdiscount.so is moved to %ruby_sitearch before
    the tree is copied into %buildroot. You must
    - first copy the whole tree to %buildroot
    - then move files under %buildroot if needed.
    ! Note that as said above lib/markdown should not be moved to
      %ruby_sitearch.

* "rake test"
  - For this gem "rake test" fails because the needed test/MarkdownTest_1.0.3
    is missing. Please replace this with "rake test:unit"

* Issues in %files
  - As %geminstdir is explicitly defined, please use the macro also
    in %files

  - The directory %geminstdir itself is not owned by any packages:
    https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes

* %doc tag in -doc subpackage
  - Usually I think %doc attribute in -doc subpackage is just redundant
    because the name of the rpm already says that this is for documentation.

* %exclude'ing ext/ directory
  - Leaves ext/rdiscount.so.debug files in -debuginfo rpm, which is not needed.
    Please "remove" ext/ directory from %buildroot completely (not from
    %_builddir) instead of using %exclude to avoid this.

Comment 6 Gerd Pokorra 2010-06-11 08:33:51 UTC
Thank you again for the competent comments and good help.

The old SPEC file is:
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount.spec.2

The URL of the current SPEC file is:
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount.spec

new SRPM-URL:
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount-1.6.3.2-3.fc12.src.rpm

Scratch build URL:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2244251

Comment 7 Mamoru TASAKA 2010-06-11 17:53:44 UTC
For -3:

* Unneeded macro
  - %ruby_sitelib macro is used nowhere.

* Dependency for -doc subpackage
  - As I said in the previous comment, for directory (%geminstdir)
    ownership issue, -doc subpackage should have the dependency
    for main package
    ( i.e. -doc subpackage should have "R: %{name} = %{version}-%{release} )

* .gemspec
  - Move %{geminstdir}/rdiscount.gemspec to -doc subpackage.
    This one is needed only for tests.
    ( %{gemdir}/specifications/%{gemname}-%{version}.gemspec should
      stay in main package ).

Comment 8 Gerd Pokorra 2010-06-12 09:18:43 UTC
I done the three items from the last list. Only the dependency
in the doc subpackage to the main package I do not like so much. I would prefer the possibility to install the doc package und be able to read the documentation without the need to have the main package installed. But I looked to the spec file of an other rubygem package. That has the same dependency included.

The URL for the old SPEC file is:
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount.spec.3

The URL of the current SPEC file is:
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount.spec

new SRPM-URL:
ftp://ftp.uni-siegen.de/pub/review/rubygem-rdiscount-1.6.3.2-4.fc12.src.rpm

new scratch build URL:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2246202

Comment 9 Gerd Pokorra 2010-06-12 10:02:10 UTC
Apologize again, after a deeper look in the documentation I understand that setting the dependency is the only way to resolve the ownership of the directory
"%geminstdir".

Comment 10 Mamoru TASAKA 2010-06-12 18:01:45 UTC
For -4:

* Directory ownership issue
  - Oops... I missed one thing. Currently the directory
    %{geminstdir}/bin itself is not owned by any package. Please
    fix this.
    (i.e. change to %{geminstdir}/bin/ in %files or explicitly
     add the directory by %dir)

! %doc in -doc
  - As I said in the above, I think %doc in -doc subpackage is redundant,
    however not a blocker.

Please fix the above issue(s) before importing this package into
Fedora.
----------------------------------------------------------------
    This package (rubygem-rdiscount) is APPROVED by mtasaka
-----------------------------------------------------------------

Comment 11 Gerd Pokorra 2010-06-14 05:09:35 UTC
New Package CVS Request
=======================
Package Name: rubygem-rdiscount
Short Description: Fast Implementation of Gruber's Markdown in C
Owners: gerd
Branches: F-12 F-13
InitialCC: gerd

Comment 12 Mamoru TASAKA 2010-06-14 12:38:46 UTC
Please set fedora-cvs flag to ?

Comment 13 Gerd Pokorra 2010-06-14 13:30:35 UTC
New Package CVS Request
=======================
Package Name: rubygem-rdiscount
Short Description: Fast Implementation of Gruber's Markdown in C
Owners: gerd
Branches: F-12 F-13
InitialCC: gerd

Comment 14 Kevin Fenzi 2010-06-21 02:15:43 UTC
CVS done (by process-cvs-requests.py).

Comment 15 Fedora Update System 2010-06-21 07:41:42 UTC
rubygem-rdiscount-1.6.3.2-4.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/rubygem-rdiscount-1.6.3.2-4.fc13

Comment 16 Fedora Update System 2010-06-21 07:55:56 UTC
rubygem-rdiscount-1.6.3.2-4.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/rubygem-rdiscount-1.6.3.2-4.fc12

Comment 17 Fedora Update System 2010-06-21 21:28:16 UTC
rubygem-rdiscount-1.6.3.2-4.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 18 Fedora Update System 2010-06-21 21:43:36 UTC
rubygem-rdiscount-1.6.3.2-4.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.


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