Bug 1051901 - Review Request: rubygem-rgen - Ruby Modelling and Generator Framework
Summary: Review Request: rubygem-rgen - Ruby Modelling and Generator Framework
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1051613 1059391 1059395 1059397
TreeView+ depends on / blocked
 
Reported: 2014-01-12 18:20 UTC by Sam Kottler
Modified: 2014-06-18 08:14 UTC (History)
4 users (show)

Fixed In Version: rubygem-rgen-0.6.6-2.el6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-01-25 13:55:36 UTC
vondruch: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Sam Kottler 2014-01-12 18:20:35 UTC
Spec URL: http://skottler.fedorapeople.org/reviews/rubygem-rgen/rubygem-rgen.spec
SRPM URL: http://skottler.fedorapeople.org/reviews/rubygem-rgen/rubygem-rgen-0.6.6-1.fc21.src.rpm
Description: RGen is a framework for Model Driven Software Development (MDSD) in Ruby. This means that it helps you build Metamodels, instantiate Models, modify and transform Models and finally generate arbitrary textual content from it.
Fedora Account System Username: skottler

Comment 1 Vít Ondruch 2014-01-13 12:26:37 UTC
I'll take this for a review.

Comment 2 Vít Ondruch 2014-01-13 12:57:44 UTC
* %clean may not be required
  - %clean is required just for EPEL5 [1], are you planning to submit this
    package for EPEL5? Make this section optional just for EPEL5 might be good
    idea as well. Not a blocker though.

* License file in main package
  - The license file have to be shipped in the main package [2].

* Wrong conditionalized ruby requires
  - "%if 0%{?rhel} <= 6" is on Fedora always true, since %{rhel} macro is not
    defined. This results in "Error: No Package found for ruby(abi) = 1.8"
    during build.

* BuildRequires: ruby
  - There is no need for "BuildRequires: ruby" as long as the gem is noarch.
    "BuildRequires: ruby(release)" should be enough.

* Move test and Rakefile into -doc subpackage
  - Files which are not needed for runtime are typically placed into -doc
    subpackage.



[1] https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
[2] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

Comment 3 Sam Kottler 2014-01-13 13:31:28 UTC
(In reply to Vít Ondruch from comment #2)
> * %clean may not be required
>   - %clean is required just for EPEL5 [1], are you planning to submit this
>     package for EPEL5? Make this section optional just for EPEL5 might be
> good
>     idea as well. Not a blocker though.

Nope, definitely not going to put the package in EL5. Removed.

> 
> * License file in main package
>   - The license file have to be shipped in the main package [2].

Moved it.

> 
> * Wrong conditionalized ruby requires
>   - "%if 0%{?rhel} <= 6" is on Fedora always true, since %{rhel} macro is not
>     defined. This results in "Error: No Package found for ruby(abi) = 1.8"
>     during build.

I removed the conditional completely - when I pull the package into EPEL6 I'll just change the package names.

> 
> * BuildRequires: ruby
>   - There is no need for "BuildRequires: ruby" as long as the gem is noarch.
>     "BuildRequires: ruby(release)" should be enough.

Removed.

> 
> * Move test and Rakefile into -doc subpackage
>   - Files which are not needed for runtime are typically placed into -doc
>     subpackage.

Moved them into the -doc subpackage.

> 
> 
> 
> [1] https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
> [2] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

Comment 4 Sam Kottler 2014-01-13 13:32:04 UTC
The spec and srpm are updated at the same link.

Comment 5 Vít Ondruch 2014-01-15 14:28:51 UTC
* Bump release
  - It is good habit to bump release for each review cycle. It is easier to see
    changes and compare with previous version, if needed.

* Don't mark test and Rakefile by %doc macro
  - They are not documentation, therefore they should not be marked by %doc
    documentation.
  - Some even thinks that nothing in -doc subpackage should be marked by as
    %doc, but I am not in that camp ;) So take it just as a remark.

* Remove test and Rakefile from main package
  - You keep them twice now. Please remove them from the main package and leave
    them just in -doc subpackage

* rpmlint
  - rpmlint complains about wrong-file-end-of-line-encoding a lot. This might be
    better to check with upstream.
  - rubygem-rgen-doc.noarch: W: file-not-utf8 /usr/share/gems/gems/rgen-
      0.6.6/test/testmodel/ea_testmodel.xml error seems to be false positive,
    since the XML explicitly says it is windows-1252 encoded. Probably nothing
    we can do about it.

* Test suite
  - Please execute the test suite during build.

Comment 6 Sam Kottler 2014-01-22 12:02:30 UTC
(In reply to Vít Ondruch from comment #5)
> * Bump release
>   - It is good habit to bump release for each review cycle. It is easier to
> see

I've done that this time:

http://skottler.fedorapeople.org/reviews/rubygem-rgen/rubygem-rgen-0.6.6-2.fc21.src.rpm
http://skottler.fedorapeople.org/reviews/rubygem-rgen/rubygem-rgen.spec

>     changes and compare with previous version, if needed.
> 
> * Don't mark test and Rakefile by %doc macro
>   - They are not documentation, therefore they should not be marked by %doc
>     documentation.
>   - Some even thinks that nothing in -doc subpackage should be marked by as
>     %doc, but I am not in that camp ;) So take it just as a remark.

Done.

> 
> * Remove test and Rakefile from main package
>   - You keep them twice now. Please remove them from the main package and
> leave
>     them just in -doc subpackage

Whoops, fixed.

> 
> * rpmlint
>   - rpmlint complains about wrong-file-end-of-line-encoding a lot. This
> might be
>     better to check with upstream.
>   - rubygem-rgen-doc.noarch: W: file-not-utf8 /usr/share/gems/gems/rgen-
>       0.6.6/test/testmodel/ea_testmodel.xml error seems to be false positive,
>     since the XML explicitly says it is windows-1252 encoded. Probably
> nothing
>     we can do about it.

I'll start working with the upstream to fix this.

> 
> * Test suite
>   - Please execute the test suite during build.

Done.

Comment 7 Vít Ondruch 2014-01-24 13:31:19 UTC
* Don't introduce additional requires
  - Upstream does not like them
  - They are useless, probably except tests with Ruby 1.8.7

* Mark the license by %doc macro
  - Please mark the %{gem_instdir}/MIT-LICENSE as a documentation.

> > * rpmlint
> >   - rpmlint complains about wrong-file-end-of-line-encoding a lot. This
> > might be
> >     better to check with upstream.
> >   - rubygem-rgen-doc.noarch: W: file-not-utf8 /usr/share/gems/gems/rgen-
> >       0.6.6/test/testmodel/ea_testmodel.xml error seems to be false positive,
> >     since the XML explicitly says it is windows-1252 encoded. Probably
> > nothing
> >     we can do about it.
> 
> I'll start working with the upstream to fix this.

Interesting, now I cannot reproduce these issues. Rpmlint now complains just about CHANGELOG. This is probably updated file utility.

Nevertheless, is there any upstream ticket for this?

Otherwise, the package looks ok => APPROVED. Please fix the minor nits I have mentioned above prior importing.

Comment 8 Sam Kottler 2014-01-24 13:41:43 UTC
(In reply to Vít Ondruch from comment #7)
> * Don't introduce additional requires
>   - Upstream does not like them
>   - They are useless, probably except tests with Ruby 1.8.7

I just changed it to use RUBYOPTS to load rubygems before running the test suite.

> 
> * Mark the license by %doc macro
>   - Please mark the %{gem_instdir}/MIT-LICENSE as a documentation.
> 
> > > * rpmlint
> > >   - rpmlint complains about wrong-file-end-of-line-encoding a lot. This
> > > might be
> > >     better to check with upstream.
> > >   - rubygem-rgen-doc.noarch: W: file-not-utf8 /usr/share/gems/gems/rgen-
> > >       0.6.6/test/testmodel/ea_testmodel.xml error seems to be false positive,
> > >     since the XML explicitly says it is windows-1252 encoded. Probably
> > > nothing
> > >     we can do about it.
> > 
> > I'll start working with the upstream to fix this.
> 
> Interesting, now I cannot reproduce these issues. Rpmlint now complains just
> about CHANGELOG. This is probably updated file utility.
> 
> Nevertheless, is there any upstream ticket for this?

Not yet, I'll create one today. I haven't actually tried to reproduce it, but will do that again to see if I can recreate what you've seen before filing the ticket.

> 
> Otherwise, the package looks ok => APPROVED. Please fix the minor nits I
> have mentioned above prior importing.

Thanks, Vit!

Comment 9 Sam Kottler 2014-01-24 15:53:22 UTC
New Package SCM Request
=======================
Package Name: rubygem-rgen
Short Description: Ruby Modelling and Generator Framework
Owners: skottler
Branches: f20 f19 el6 epel7
InitialCC:

Comment 10 Gwyn Ciesla 2014-01-24 16:04:32 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2014-01-24 16:51:08 UTC
rubygem-rgen-0.6.6-2.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/rubygem-rgen-0.6.6-2.fc20

Comment 12 Fedora Update System 2014-01-24 17:01:14 UTC
rubygem-rgen-0.6.6-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/rubygem-rgen-0.6.6-2.fc19

Comment 13 Fedora Update System 2014-01-24 17:14:16 UTC
rubygem-rgen-0.6.6-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/rubygem-rgen-0.6.6-2.el6

Comment 14 Fedora Update System 2014-01-25 05:08:46 UTC
rubygem-rgen-0.6.6-2.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 15 Fedora Update System 2014-01-25 13:55:36 UTC
rubygem-rgen-0.6.6-2.fc20 has been pushed to the Fedora 20 stable repository.

Comment 16 Fedora Update System 2014-01-31 04:28:42 UTC
rubygem-rgen-0.6.6-2.fc19 has been pushed to the Fedora 19 stable repository.

Comment 17 Fedora Update System 2014-01-31 20:07:48 UTC
rubygem-rgen-0.6.6-2.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.