Bug 845799 - (rubygem-hashr) Review Request: rubygem-hashr - Simple Hash extension to make working with nested hashes
Review Request: rubygem-hashr - Simple Hash extension to make working with n...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Vít Ondruch
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-05 02:19 EDT by Miroslav Suchý
Modified: 2012-08-27 13:03 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-08-09 09:49:26 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
vondruch: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Miroslav Suchý 2012-08-05 02:19:25 EDT
Spec URL: http://miroslav.suchy.cz/fedora/rubygem-hashr/rubygem-hashr.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-hashr/rubygem-hashr-0.0.21-1.fc17.src.rpm
Description: Simple Hash extension to make working with nested hashes (e.g. for
configuration) easier and less error-prone.
Fedora Account System Username: msuchy

rpmlint output:
rubygem-hashr.noarch: W: wrong-file-end-of-line-encoding /usr/share/gems/doc/hashr-0.0.21/ri/cache.ri
rubygem-hashr.noarch: W: unexpanded-macro /usr/share/gems/doc/hashr-0.0.21/ri/Hashr/%5b%5d-i.ri %5b
rubygem-hashr.noarch: W: unexpanded-macro /usr/share/gems/doc/hashr-0.0.21/ri/Hashr/%5b%5d-i.ri %5d
rubygem-hashr.noarch: W: unexpanded-macro /usr/share/gems/doc/hashr-0.0.21/ri/Hashr/%5b%5d%3d-i.ri %5b
rubygem-hashr.noarch: W: unexpanded-macro /usr/share/gems/doc/hashr-0.0.21/ri/Hashr/%5b%5d%3d-i.ri %5d
rubygem-hashr.noarch: W: unexpanded-macro /usr/share/gems/doc/hashr-0.0.21/ri/Hashr/%5b%5d%3d-i.ri %3d
rubygem-hashr.noarch: W: unexpanded-macro /usr/share/gems/doc/hashr-0.0.21/ri/Hash/deep_symbolize_keys%21-i.ri %21
rubygem-hashr.noarch: W: unexpanded-macro /usr/share/gems/doc/hashr-0.0.21/ri/Hashr/respond_to%3f-i.ri %3f
2 packages and 0 specfiles checked; 0 errors, 9 warnings.

This is generated by ri, and I'm not sure if I should fix it.

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=4360069
Comment 1 Vít Ondruch 2012-08-08 02:20:26 EDT
I'll take it for a review.
Comment 2 Vít Ondruch 2012-08-08 02:56:43 EDT
* Use %global instead of %define
  - Unless there is any special need, the %global macro should be used in place
    of %define according to FPG [1]

* Test suite
  - It would be nice to execute a test suite of the gem.

* License
  - According to README.md and MIT-LICENSE files, the license should be MIT

* -doc subpackage
  - Have you considered to move the documentation into -doc subpackage?

* Please use the new RubyGems packaging macros [2]
  - It would be nice to use the macros available for gem packaging.
  - For RHEL, you can use [3] as an example and optionally comment on Bug 788001
    to push these macros into RHEL ;)

* RHEL and older Fedora release conditioning
  - Please consider to use "%if 0%{?el6}%{?fc16}" macros instead of wordy
    "%if 0%{?rhel} == 6 || 0%{?fedora} < 17"

* Exclude gemcache
  - We typically exclude the cached version of gem, such as:
      %exclude %{gem_cache}


I would stop at this point and wait for fixes from your side. Thank you.


[1] https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
[2] https://fedoraproject.org/wiki/Packaging:Ruby#Macros
[3] http://pkgs.fedoraproject.org/cgit/rubygem-hydra.git/tree/rubygem-hydra.spec
Comment 3 Miroslav Suchý 2012-08-08 07:33:01 EDT
Updated:
Spec URL: http://miroslav.suchy.cz/fedora/rubygem-hashr/rubygem-hashr.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-hashr/rubygem-hashr-0.0.21-2.fc17.src.rpm

Fixed:
* Use %global instead of %define
* Test suite
* License
* -doc subpackage
* Please use the new RubyGems packaging macros

ad /Please consider to use "%if 0%{?el6}%{?fc16}" macros instead of wordy
    "%if 0%{?rhel} == 6 || 0%{?fedora} < 17"/
I do not like the shorter notation. It is less readable and thefore maintainable (of course IMHO).
And while 0%{?fedora} < 17 works for Fedora 15, shorter notation %{?fc16}. I know that F15 is not supported, but still.. I simply like the longer notation.

* Exclude gemcache
per our IRC discussion I will keep it here. I like the sideefect of repackaging, where - in case package will get patch - the repackage gem contains those patch(es) and developers, which develop for Fedora on their macs, can use this gem, which contains these patch(es).
Comment 4 Vít Ondruch 2012-08-08 08:13:21 EDT
(In reply to comment #3)
> Fixed:
> * Use %global instead of %define
> * Test suite
> * License
> * -doc subpackage
> * Please use the new RubyGems packaging macros

Thank you.
 
> ad /Please consider to use "%if 0%{?el6}%{?fc16}" macros instead of wordy
>     "%if 0%{?rhel} == 6 || 0%{?fedora} < 17"/
> I do not like the shorter notation. It is less readable and thefore
> maintainable (of course IMHO).
> And while 0%{?fedora} < 17 works for Fedora 15, shorter notation %{?fc16}. I
> know that F15 is not supported, but still.. I simply like the longer
> notation.

Ok, no problem.

> * Exclude gemcache
> per our IRC discussion I will keep it here. I like the sideefect of
> repackaging, where - in case package will get patch - the repackage gem
> contains those patch(es) and developers, which develop for Fedora on their
> macs, can use this gem, which contains these patch(es).

This is not show stopper, but something which should be re-discussed at Ruby-SIG. Any input is welcome. 


* Do not own the whole %{gem_instdir}
  - I would suggest to be more specific and instead of %{gem_instdir} use
    %dir %{gem_instdir}. This is a bit more work, but it will allow you to spot
    some major changes when updating the gem.
  - This is not show stopper, but I'd like you to consider it.

* Test suite execution
  - You can simplify the test suite execution command:

    testrb -Ilib test/*_test.rb"

* Non-essential files should be moved to -doc subpackage
  - Please move the test suite into -doc subpackage, since it is not needed for
    runtime
  - Please move Gemfile*, Rakefile and README.md as well

* The -doc subpackage should require the main package
  - Requires: %{name} = %{version}-%{release}
Comment 5 Miroslav Suchý 2012-08-08 08:43:54 EDT
Thank you for your suggestion, I really appreciate it.

All issues should be addressed:
Spec URL: http://miroslav.suchy.cz/fedora/rubygem-hashr/rubygem-hashr.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-hashr/rubygem-hashr-0.0.21-3.fc17.src.rpm
Comment 6 Vít Ondruch 2012-08-09 04:32:15 EDT
I have no other concerns => APPROVED
Comment 7 Miroslav Suchý 2012-08-09 05:29:44 EDT
New Package SCM Request
=======================
Package Name: rubygem-hashr
Short Description: Simple Hash extension to make working with nested hashes
Owners: msuchy
Branches: F-18, F-17, F-16, EL-6
InitialCC:
Comment 8 Gwyn Ciesla 2012-08-09 08:38:27 EDT
Git done (by process-git-requests).
Comment 9 Miroslav Suchý 2012-08-09 09:49:26 EDT
Build for Fedora 19
http://koji.fedoraproject.org/koji/taskinfo?taskID=4371819
Comment 10 Fedora Update System 2012-08-09 09:59:23 EDT
rubygem-hashr-0.0.21-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-hashr-0.0.21-3.fc17
Comment 11 Fedora Update System 2012-08-09 10:00:36 EDT
rubygem-hashr-0.0.21-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rubygem-hashr-0.0.21-3.fc16
Comment 12 Fedora Update System 2012-08-09 10:02:21 EDT
rubygem-hashr-0.0.21-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/rubygem-hashr-0.0.21-3.el6
Comment 13 Fedora Update System 2012-08-17 21:22:54 EDT
rubygem-hashr-0.0.21-3.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 14 Fedora Update System 2012-08-17 21:29:07 EDT
rubygem-hashr-0.0.21-3.fc16 has been pushed to the Fedora 16 stable repository.
Comment 15 Fedora Update System 2012-08-27 13:03:01 EDT
rubygem-hashr-0.0.21-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

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