Bug 603269 - Review Request: rubygem-plist - All-purpose Property List manipulation library
Summary: Review Request: rubygem-plist - All-purpose Property List manipulation library
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://plist.rubyforge.org
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-06-12 06:11 UTC by Gerd Pokorra
Modified: 2010-06-21 21:33 UTC (History)
3 users (show)

Fixed In Version: rubygem-plist-3.1.0-5.fc12
Clone Of:
Environment:
Last Closed: 2010-06-21 21:30:09 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
patch to make rake test succeed (574 bytes, patch)
2010-06-14 16:21 UTC, Mamoru TASAKA
no flags Details | Diff

Description Gerd Pokorra 2010-06-12 06:11:31 UTC
Spec URL: ftp://ftp.uni-siegen.de/pub/review/rubygem-plist.spec
SRPM URL: ftp://ftp.uni-siegen.de/pub/review/rubygem-plist-3.1.0-1.fc12.src.rpm
Description:
Plist is a library to manipulate Property List files, also known as plists. 
It can parse plist files into native Ruby data structures as well as
generating new plist files from your Ruby objects.


I do not know how to enable tests correct !!!
I hope someone who know more about ruby can help.


It seems to be a simple package and I hope it is not to much work to review it.


%prep- and %build-sections are no used so I removed it.


Scratch-build-URL of successful build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=2245099

Comment 1 Mamoru TASAKA 2010-06-14 16:21:10 UTC
Created attachment 423896 [details]
patch to make rake test succeed

Some notes:

* ruby(abi) dependency
  - ruby module packages must have "R: ruby(abi) = 1.8".
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines

* %prep, %build section:
  - From rpmlint
----------------------------------------------------------------
$ rpmlint -I 'no-%prep-section'
no-%prep-section:
The spec file does not contain a %prep section.  Even if some packages don't
directly need it, section markers may be overridden in rpm's configuration to
provide additional "under the hood" functionality.  Add the section, even if
empty.
----------------------------------------------------------------
    i.e. (in the future) rpmbuild may do some additional needed procedure
    before the contents written in %prep or %build begins.

    So even if currently %prep or %build can be empty, please add them.

* %check
  - Doing "rake test" needs the attached patch.
    ( I guess that (although I have not checked it in detail)
      rdoc/task is in ruby 1.9. On Fedora rdoctask is in rake gem anyway )

* %doc in -doc subpackage
  - I think %doc attribute in -doc subpackage is redundant because
    rpm's name already shows that this is for documentation.

! macros in comments
------------------------------------------------------------------
#export PATH=$PATH:%{buildroot}%{geminstdir}/lib/plist
#export LD_LIBRARY_PATH=$PATH:%{buildroot}%{geminstdir}/lib
....
------------------------------------------------------------------
  - These comment lines include macros and rpmlint warns for these:
------------------------------------------------------------------
rubygem-plist.src:51: W: macro-in-comment %{buildroot}
rubygem-plist.src:51: W: macro-in-comment %{geminstdir}
------------------------------------------------------------------

    The reason rpmlint warns for "macro-in-comment" is that
    during rpmbuild macros are expanded anyway, even in comment lines.
    For this package this is safe, however if a defined macro contains
    several lines, this can cause some strange behavior.
    - For example, with "#%configure", configure script is executed
      against expectation

    So it is always recommended to excape macros in comment lines
    using %% instead of %.

Comment 2 Gerd Pokorra 2010-06-15 06:15:11 UTC
old SPEC file URL: ftp://ftp.uni-siegen.de/pub/review/rubygem-plist.spec.1

current Spec file URL: ftp://ftp.uni-siegen.de/pub/review/rubygem-plist.spec
current SRPM URL: 
ftp://ftp.uni-siegen.de/pub/review/rubygem-plist-3.1.0-2.fc12.src.rpm

current scratch-build-URL:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2250832


Test output (also added as comment in the SPEC file):

...
+ ruby test
ruby: Is a directory - test (Errno::EISDIR)
...

Comment 3 Mamoru TASAKA 2010-06-15 06:20:25 UTC
I have not checked your srpm, however:

(In reply to comment #1)
> * %check
>   - Doing "rake test" needs the attached patch.

Comment 5 Mamoru TASAKA 2010-06-16 14:20:23 UTC
For -2:

* For plist.patch
  - As this is a patch, please use "Patch0" instead of "Source1"
  - And please consider to rename this to something which indicates
    what this patch is for, and also write some comments
    about this patch briefly.

* Comments in %check
  - Please remove comments which are no longer needed
    ( ruby test or so is no longer needed )

* Fallback in %check
  - I don't think "|| :" after rake test is needed any longer.

* %doc in -doc
  - Well, I don't think you have to add %doc in -doc just to
    suppress rpmlint warnings.

Comment 6 Gerd Pokorra 2010-06-16 15:22:42 UTC
- use "Patch0" instead of "Source1" as tag for the patch
- add comments about the patch and rename the file

The rest was already done in release 3 of the spec file.

old SPEC file URL: ftp://ftp.uni-siegen.de/pub/review/rubygem-plist.spec.3

current Spec file URL: ftp://ftp.uni-siegen.de/pub/review/rubygem-plist.spec
current SRPM URL: 
ftp://ftp.uni-siegen.de/pub/review/rubygem-plist-3.1.0-4.fc12.src.rpm

current scratch-build-URL:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2253763

Comment 7 Mamoru TASAKA 2010-06-17 19:26:25 UTC
Well, -4:
-------------------------------------------------------------
   83  #  E: non-executable-script  (for *.rb files)
-------------------------------------------------------------
- This is because scripts under test/ dierctory have shebangs
  but does not have executable permission.

  Since these scripts need not have shebangs, simply remove
  shebangs from these scripts (under test/).

Comment 9 Mamoru TASAKA 2010-06-18 17:29:30 UTC
Okay.

-----------------------------------------------------------
    This package (rubygem-plist) is APPROVED by mtasaka
-----------------------------------------------------------

Comment 10 Gerd Pokorra 2010-06-18 20:20:46 UTC
Thank you for the patch and the review!

Comment 11 Gerd Pokorra 2010-06-18 20:24:55 UTC
New Package CVS Request
=======================
Package Name: rubygem-plist
Short Description: All-purpose Property List manipulation
Owners: gerd
Branches: F-12 F-13
InitialCC: gerd

Comment 12 Kevin Fenzi 2010-06-21 02:17:51 UTC
CVS done (by process-cvs-requests.py).

Comment 13 Fedora Update System 2010-06-21 09:13:29 UTC
rubygem-plist-3.1.0-5.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/rubygem-plist-3.1.0-5.fc13

Comment 14 Fedora Update System 2010-06-21 09:15:10 UTC
rubygem-plist-3.1.0-5.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/rubygem-plist-3.1.0-5.fc12

Comment 15 Fedora Update System 2010-06-21 21:30:05 UTC
rubygem-plist-3.1.0-5.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2010-06-21 21:33:50 UTC
rubygem-plist-3.1.0-5.fc12 has been pushed to the Fedora 12 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.