Bug 811418

- Are you planning to use this gem in EPEL, too? Because if yes, you will have to define the macros conditionally. Otherwise, please remove the defattr lines, see [1].
- The %{docdir} and TODO in doc subpackage should be marked as %doc, to distinguish them from other files in this subpackage, that are not documentation.
- I think you should exclude the .document file, it doesn't contain anything useful.
- For any other gem, I would tell you to run the test suite, but since it requires passwordless login on localhost [2], it would be pain to get it working and it probably wouldn't even work on koji.
- Please fix these rpmlint warnings (make the files executable):

rubygem-hydra-doc.noarch: E: non-executable-script /usr/share/gems/gems/hydra-0.24.0/test/fixtures/many_outputs_to_console.rb 0664L /usr/bin/env
rubygem-hydra-doc.noarch: E: non-executable-script /usr/share/gems/gems/hydra-0.24.0/test/fixtures/hello_world.rb 0664L /usr/bin/env


[1] https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
[2] https://github.com/ngauthier/hydra/wiki/development-environment

Thanks for the review!  I've updated the following here:

Spec URL: http://matthicksj.fedorapeople.org/reviews/rubygem-hydra.spec

SRPM URL:
http://matthicksj.fedorapeople.org/reviews/rubygem-hydra-0.24.0-1.fc16.src.rpm

I couldn't find a good example in the packaging guidelines of how to properly do the macros properly for EPEL - if you have an example, I'd be happy to add that, otherwise, getting into Fedora is probably a good start.

I made the following changes:
- Removed .document from the builddir
- Added %doc to TODO and %{docdir}
- Set 755 to the test scripts

Around the tests, I hit the same problem there, so they aren't running.  Also, I went back and forth about running sed to remove some of the rpmlint warnings on the wrong-file-end-of-line-encoding (see [1]) but saw some discussion about that potentially breaking things (see [2]) and decided not to.  Any suggestions there?


[1] https://fedoraproject.org/wiki/Common_Rpmlint_issues#wrong-file-end-of-line-encoding
[2] https://bugzilla.redhat.com/show_bug.cgi?id=728256

Thanks again

Ok, the specfile looks much better now, just a few catches:
- If you want to build in EPEL as well, you have to define the macros in the way they used to be defined in the previous packaging guidelines [1].
- Also, you have to conditionally require rubygems-devel or rubygems, depending on if you are in Fedora or in EPEL.
- A better way to make files executable is to run chmod in %install. Notice, that the way you are doing it now causes the rpmbuild to complain:

warning: File listed twice: /usr/share/gems/gems/hydra-0.24.0/test/fixtures/hello_world.rb
warning: File listed twice: /usr/share/gems/gems/hydra-0.24.0/test/fixtures/many_outputs_to_console.rb

- As for the wrong EOF encodings, these are standard rpmlint warnings when packaging gems with ri/rdoc, you don't have to worry about them.

[1] https://fedoraproject.org/wiki/Packaging:Old_Ruby

Okay, I think I've got the conditional macros defined and working for EPEL.  I've been testing this locally using mock on the epel-6 configuration, so if there is a better way to test, just let me know.  It appears to work on F17 and EPEL6 with no rpmlint warnings that I see outside of the normal ones.

Summary of changes:
- Switched %attr permission settings to 'chmod 755' in %install
- Defined conditionals per the OldRuby packaging guidelines.  One variation was that I changed 'gemdir' to 'gem_dir' to align with the new style macros
- Added the conditional requires and buildrequires for F17 and EPEL

Here are the updated spec and SRPMS:

Spec URL: http://matthicksj.fedorapeople.org/reviews/rubygem-hydra.spec

SRPM URL:
http://matthicksj.fedorapeople.org/reviews/rubygem-hydra-0.24.0-1.fc16.src.rpm

Thanks again for your help.

Hi Matt, this looks really good now, however there is a small issue: On RHEL 7, we will already have rubygems-devel and rubyabi 1.9.1. So I'd advise you to use conditionals like this one:

%if 0%{?rhel5}%{?rhel6}

Please note that you cannot use this:

%if 0%{?rhel} <= 6

because it would always evaluate to true on Fedora.
Also, there is one small needles Requires: rubygems in the %{?rhel} section.

Otherwise the package looks good, so once you fix these two minorities, I will approve it.

Latest round:

Spec URL: http://matthicksj.fedorapeople.org/reviews/rubygem-hydra.spec

SRPM URL:
http://matthicksj.fedorapeople.org/reviews/rubygem-hydra-0.24.0-1.fc16.src.rpm

I've updated the conditions to use el5 / el6.

Had one question around the 'Requires: rubygems'.  In the old packaging guidelines, they had:

The package must have a Requires and a BuildRequires on rubygems

However, I used the format above 'Requires: ruby(rubygems)' as I saw that used in a couple of other RPM's.  Any advice on the right approach?  I could do the following:

1. Change 'Require: ruby(rubygems)' to 'Require: rubygems'.  That would meet the old ruby packaging guidelines and I believe should functionally work.

2. I could modify the conditional to include the following

%if 0%{?el5}%{?el6}
Requires: rubygems
BuildRequires: rubygems
%else
Requires: ruby(rubygems)
BuildRequires: rubygems-devel
%endif

Thanks again for your help

(In reply to comment #6)
> Latest round:
> 
> Spec URL: http://matthicksj.fedorapeople.org/reviews/rubygem-hydra.spec
> 
> SRPM URL:
> http://matthicksj.fedorapeople.org/reviews/rubygem-hydra-0.24.0-1.fc16.src.rpm
> 
> I've updated the conditions to use el5 / el6.
> 
> Had one question around the 'Requires: rubygems'.  In the old packaging
> guidelines, they had:
> 
> The package must have a Requires and a BuildRequires on rubygems
> 
> However, I used the format above 'Requires: ruby(rubygems)' as I saw that used
> in a couple of other RPM's.  Any advice on the right approach?  I could do the
> following:
> 
> 1. Change 'Require: ruby(rubygems)' to 'Require: rubygems'.  That would meet
> the old ruby packaging guidelines and I believe should functionally work.
> 

Yes, that is the safest way, but I believe that all rubygems packages in relevant versions of systems also provide ruby(rubygems), so it shouldn't matter.

> 2. I could modify the conditional to include the following
> 
> %if 0%{?el5}%{?el6}
> Requires: rubygems
> BuildRequires: rubygems
> %else
> Requires: ruby(rubygems)
> BuildRequires: rubygems-devel
> %endif
> 
> Thanks again for your help

Yes, but again, I think that requiring just rubygems is a safe option.
Also, if you are planning to use the gem on Fedora 16 or 15, you will have to use the same as in RHEL 5 and 6. (You can achieve that similarly as with %{?el5}%{?el6}...)

Anyway, this package looks good now and is APPROVED. (please make the changes before committing).

Thanks again

Latest updates to close the loop:

Spec URL: http://matthicksj.fedorapeople.org/reviews/rubygem-hydra.spec

SRPM URL:
http://matthicksj.fedorapeople.org/reviews/rubygem-hydra-0.24.0-1.fc16.src.rpm

Great. I already set the review flag to +, so you can proceed with the SCM request.

New Package SCM Request
=======================
Package Name: rubygem-hydra
Short Description: Distributes tests for speed
Owners: matthicksj
Branches: f16 f17 el6
InitialCC:

Git done (by process-git-requests).

rubygem-hydra-0.24.0-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/rubygem-hydra-0.24.0-1.fc17

rubygem-hydra-0.24.0-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rubygem-hydra-0.24.0-1.fc16

rubygem-hydra-0.24.0-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/rubygem-hydra-0.24.0-1.el6

rubygem-hydra-0.24.0-1.fc17 has been pushed to the Fedora 17 testing repository.

rubygem-hydra-0.24.0-1.fc16 has been pushed to the Fedora 16 stable repository.

rubygem-hydra-0.24.0-1.fc17 has been pushed to the Fedora 17 stable repository.

rubygem-hydra-0.24.0-1.el6 has been pushed to the Fedora EPEL 6 stable repository.