Bug 1051901

I'll take this for a review.

* %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

(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

The spec and srpm are updated at the same link.

* 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.

(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.

* 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.

(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!

New Package SCM Request
=======================
Package Name: rubygem-rgen
Short Description: Ruby Modelling and Generator Framework
Owners: skottler
Branches: f20 f19 el6 epel7
InitialCC:

Git done (by process-git-requests).

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

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

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

rubygem-rgen-0.6.6-2.el6 has been pushed to the Fedora EPEL 6 testing repository.

rubygem-rgen-0.6.6-2.fc20 has been pushed to the Fedora 20 stable repository.

rubygem-rgen-0.6.6-2.fc19 has been pushed to the Fedora 19 stable repository.

rubygem-rgen-0.6.6-2.el6 has been pushed to the Fedora EPEL 6 stable repository.