Bug 847457 - Review Request: rubygem-transaction-simple - Simple object transaction support for Ruby
Review Request: rubygem-transaction-simple - Simple object transaction suppor...
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-11 12:10 EDT by Miroslav Suchý
Modified: 2012-09-17 19:02 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-26 23:26:59 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-11 12:10:31 EDT
Spec URL: http://miroslav.suchy.cz/fedora/rubygem-transaction-simple/rubygem-transaction-simple.spec
SRPM URL: http://miroslav.suchy.cz/fedora/rubygem-transaction-simple/rubygem-transaction-simple-1.4.0-8.fc17.src.rpm
Description: 
Transaction::Simple provides a generic way to add active transaction support
to objects. The transaction methods added by this module will work with most
objects, excluding those that cannot be Marshal-ed (bindings, procedure
objects, IO instances, or singleton objects).

Fedora Account System Username: msuchy
Comment 1 Vít Ondruch 2012-08-15 04:44:33 EDT
I'll take it for a review.
Comment 2 Vít Ondruch 2012-08-15 06:01:27 EDT
* Version 1.4.0.2 available
  - Is there any particular reason why not package the most recent version?

* Use %global in place of %define [1]

* Do not depend on rubygem(hoe)
  - This is just development dependency. You don't need it for runtime and there
    is only tiny chance that you would need it for build time.
  
* Drop s.cert_chain in .gemspec
  - I would say that better than changing the s.cert_chain would be to fallback
    to default, i.e. drop the line: 

    sed -i '/s.cert_chain = nil/ d' %{gem_name}.gemspec

* Leave the History.txt Install.txt Licence.txt Readme.txt Manifest.txt on
  original place
  - Please consider to keep the files on the original place. I am not sure what
    would be the benefit to move them into different place

* Move documentation into -doc subapackage
  - Please consider move of History.txt, Install.txt, Readme.txt and
    Manifest.txt files into -doc subpackage, since they are not required by
    runtime.

* Exclude %{gem_cache}
  - We discussed it before in different review, but I'd like to mention it
    here, although I don't expect you will do that ;)

* export CONFIGURE_ARGS not needed
  - Since this is not binary gem, the export CONFIGURE_ARGS could be omitted.

* Mark %{gem_docdir} as as %doc

* Please fix the executable bit
  - rpmlint complains about following files:

rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_transaction_simple.rb 0644L /usr/bin/env
rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_all.rb 0644L /usr/bin/env
rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_transaction_simple_group.rb 0644L /usr/bin/env
rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_transaction_simple_threadsafe.rb 0644L /usr/bin/env
rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/test/test_broken_graph.rb 0644L /usr/bin/env
rubygem-transaction-simple-doc.noarch: E: non-executable-script /usr/share/gems/gems/transaction-simple-1.4.0/Rakefile 0644L /usr/bin/env


[1] https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
Comment 3 Miroslav Suchý 2012-08-16 11:11:20 EDT
> Is there any particular reason why not package the most recent version?
Yes. 1.4.0.2 depends on hoe ~> 3.0 which is not in EPEL and F16. My plan is to later upgrade to 1.4.0.2 in rawhide.

> Do not depend on rubygem(hoe)
indeed it is used for creating tar.gz. And it builds without it.
Removing. Hmm... that may solve previous item.

> Use %global in place of %define
fixed

> Drop s.cert_chain in .gemspec
dropped

> * Leave the History.txt Install.txt Licence.txt Readme.txt Manifest.txt on
>  original place
I do not agree with you.

> Move documentation into -doc subapackage
Moved

> Exclude %{gem_cache}
Surprise! As long as it is in guidlines, I decided to obey it. Untill I succed in Guidelines change.

> export CONFIGURE_ARGS not needed
fixed

Spec URL: http://miroslav.suchy.cz/fedora/rubygem-transaction-simple/rubygem-transaction-simple.spec
SRC.RPM: http://miroslav.suchy.cz/fedora/rubygem-transaction-simple/rubygem-transaction-simple-1.4.0.2-2.fc17.src.rpm
Comment 4 Vít Ondruch 2012-08-17 06:34:38 EDT
(In reply to comment #3)
> > Is there any particular reason why not package the most recent version?
> Yes. 1.4.0.2 depends on hoe ~> 3.0 which is not in EPEL and F16. My plan is
> to later upgrade to 1.4.0.2 in rawhide.
> 
> > Do not depend on rubygem(hoe)
> indeed it is used for creating tar.gz. And it builds without it.
> Removing. Hmm... that may solve previous item.

:D

> > * Leave the History.txt Install.txt Licence.txt Readme.txt Manifest.txt on
> >  original place
> I do not agree with you.

Roger
 
* dot files
  - You remove one file in %build section while the other in %install, is it
    intentional?
  - I would put %exclude %{gem_instdir}.* into %files section instead
  - btw you should use the %{gem_instdir} macro in the rm in %build section.

* URL
  - Seems that the homepage URL was changed to
      http://trans-simple.rubyforge.org/

* rpmlint:
  - You should probably remove the chmods a+x in %install section and use
    chmod a-x test/test_broken_graph.rb

rubygem-transaction-simple-doc.noarch: E: script-without-shebang /usr/share/gems/gems/transaction-simple-1.4.0.2/test/test_transaction_simple_group.rb
rubygem-transaction-simple-doc.noarch: E: script-without-shebang /usr/share/gems/gems/transaction-simple-1.4.0.2/test/test_transaction_simple.rb
rubygem-transaction-simple-doc.noarch: E: script-without-shebang /usr/share/gems/gems/transaction-simple-1.4.0.2/Rakefile
rubygem-transaction-simple-doc.noarch: E: script-without-shebang /usr/share/gems/gems/transaction-simple-1.4.0.2/test/test_broken_graph.rb


Otherwise the package looks good, so I APPROVE it, but please consider to fix the first two nits and definitely fix the rpmlint errors prior importing into SCM.
Comment 5 Miroslav Suchý 2012-08-17 07:57:18 EDT
>  - You remove one file in %build section while the other in %install, is it
>    intentional?

No, it was completly random :)
Moved to %install

> I would put %exclude %{gem_instdir}.* into %files section instead
I prefer that rm (for no obvious reason).

>  btw you should use the %{gem_instdir} macro in the rm in %build section.
fixed

> Seems that the homepage URL was changed to
indeed, fixed

> You should probably remove the chmods a+x in %install section and use
>    chmod a-x test/test_broken_graph.rb
Ah, they change it from last version. Fixed.

Thanks for review.
Comment 6 Miroslav Suchý 2012-08-17 07:59:48 EDT
New Package SCM Request
=======================
Package Name: rubygem-transaction-simple
Short Description: Simple object transaction support for Ruby
Owners: msuchy
Branches: F-18, F-17, F-16, EL-6
InitialCC:
Comment 7 Gwyn Ciesla 2012-08-17 08:03:03 EDT
Git done (by process-git-requests).
Comment 8 Fedora Update System 2012-08-17 08:43:29 EDT
rubygem-transaction-simple-1.4.0.2-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-transaction-simple-1.4.0.2-3.fc17
Comment 9 Fedora Update System 2012-08-17 08:44:08 EDT
rubygem-transaction-simple-1.4.0.2-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rubygem-transaction-simple-1.4.0.2-3.fc16
Comment 10 Fedora Update System 2012-08-17 08:44:18 EDT
rubygem-transaction-simple-1.4.0.2-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/rubygem-transaction-simple-1.4.0.2-3.el6
Comment 11 Fedora Update System 2012-08-17 08:44:27 EDT
rubygem-transaction-simple-1.4.0.2-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-transaction-simple-1.4.0.2-3.fc18
Comment 12 Fedora Update System 2012-08-17 12:30:09 EDT
rubygem-transaction-simple-1.4.0.2-3.fc18 has been pushed to the Fedora 18 testing repository.
Comment 13 Fedora Update System 2012-08-26 23:26:59 EDT
rubygem-transaction-simple-1.4.0.2-3.fc16 has been pushed to the Fedora 16 stable repository.
Comment 14 Fedora Update System 2012-08-26 23:28:29 EDT
rubygem-transaction-simple-1.4.0.2-3.fc17 has been pushed to the Fedora 17 stable repository.
Comment 15 Fedora Update System 2012-09-01 13:24:28 EDT
rubygem-transaction-simple-1.4.0.2-3.el6 has been pushed to the Fedora EPEL 6 stable repository.
Comment 16 Fedora Update System 2012-09-17 19:02:55 EDT
rubygem-transaction-simple-1.4.0.2-3.fc18 has been pushed to the Fedora 18 stable repository.

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