Bug 811418 - Review Request: rubygem-hydra - distributes tests for speed
Review Request: rubygem-hydra - distributes tests for speed
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Bohuslav "Slavek" Kabrda
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-10 21:18 EDT by Matt Hicks
Modified: 2012-05-13 21:56 EDT (History)
4 users (show)

See Also:
Fixed In Version: rubygem-hydra-0.24.0-1.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-04 16:27:05 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bkabrda: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Matt Hicks 2012-04-10 21:18:36 EDT
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

Description:
Note - this is my first package and I will need it sponsored.

Hydra is a distributed testing framework. It allows you to distribute your tests locally across multiple cores and processors, as well as run your tests remotely over SSH.

Hydra’s goals are to make distributed testing easy. So as long as you can ssh into a computer and run the tests, you can automate the distribution with Hydra.

$ rpmlint -v ~/rpmbuild/SPECS/rubygem-hydra.spec 
/home/mhicks/rpmbuild/SPECS/rubygem-hydra.spec: I: checking-url http://rubygems.org/gems/hydra-0.24.0.gem (timeout 10 seconds)
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -v /var/lib/mock/fedora-17-i386/result/rubygem-hydra-0.24.0-1.fc17.noarch.rpm 
rubygem-hydra.noarch: I: checking
rubygem-hydra.noarch: I: checking-url http://github.com/ngauthier/hydra (timeout 10 seconds)
rubygem-hydra.noarch: W: hidden-file-or-dir /usr/share/gems/gems/hydra-0.24.0/.document
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
Comment 1 Bohuslav "Slavek" Kabrda 2012-04-16 05:02:56 EDT
- 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
Comment 2 Matt Hicks 2012-04-23 10:31:50 EDT
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
Comment 3 Bohuslav "Slavek" Kabrda 2012-04-24 02:11:20 EDT
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
Comment 4 Matt Hicks 2012-04-24 08:58:31 EDT
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.
Comment 5 Bohuslav "Slavek" Kabrda 2012-04-25 02:36:58 EDT
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.
Comment 6 Matt Hicks 2012-04-25 08:54:13 EDT
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
Comment 7 Bohuslav "Slavek" Kabrda 2012-04-25 09:22:28 EDT
(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).
Comment 8 Matt Hicks 2012-04-25 09:54:26 EDT
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
Comment 9 Bohuslav "Slavek" Kabrda 2012-04-25 10:01:40 EDT
Great. I already set the review flag to +, so you can proceed with the SCM request.
Comment 10 Matt Hicks 2012-04-25 11:10:33 EDT
New Package SCM Request
=======================
Package Name: rubygem-hydra
Short Description: Distributes tests for speed
Owners: matthicksj
Branches: f16 f17 el6
InitialCC:
Comment 11 Gwyn Ciesla 2012-04-25 12:04:06 EDT
Git done (by process-git-requests).
Comment 12 Fedora Update System 2012-04-25 12:56:49 EDT
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
Comment 13 Fedora Update System 2012-04-25 12:58:06 EDT
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
Comment 14 Fedora Update System 2012-04-25 12:58:45 EDT
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
Comment 15 Fedora Update System 2012-04-26 15:29:45 EDT
rubygem-hydra-0.24.0-1.fc17 has been pushed to the Fedora 17 testing repository.
Comment 16 Fedora Update System 2012-05-04 16:27:05 EDT
rubygem-hydra-0.24.0-1.fc16 has been pushed to the Fedora 16 stable repository.
Comment 17 Fedora Update System 2012-05-04 18:56:37 EDT
rubygem-hydra-0.24.0-1.fc17 has been pushed to the Fedora 17 stable repository.
Comment 18 Fedora Update System 2012-05-13 21:56:04 EDT
rubygem-hydra-0.24.0-1.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.