Bug 640627 - Review Request: rubygem-factory_girl - Framework and DSL for defining and using model instance factories
Summary: Review Request: rubygem-factory_girl - Framework and DSL for defining and usi...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mo Morsi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-10-06 13:51 UTC by Michal Fojtik
Modified: 2017-07-14 08:45 UTC (History)
3 users (show)

Fixed In Version: rubygem-factory_girl-1.3.2-3.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-11-04 23:48:24 UTC
mmorsi: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Bugzilla 1470580 None None None Never

Internal Links: 1470580

Description Michal Fojtik 2010-10-06 13:51:02 UTC
Spec URL: http://mifo.sk/RPMS/rubygem-factory_girl.spec
SRPM URL: http://mifo.sk/RPMS/rubygem-factory_girl-1.3.2-1.fc13.src.rpm
Description:

Framework and DSL for defining and using factories - less error-prone,
more explicit, and all-around easier to work with than fixtures.

Comment 1 Mo Morsi 2010-10-13 02:20:05 UTC
Will take this one. Overall looks good, some specific comments though

* Can you change source0 to point to the official gem hosted at http://rubygems.org/downloads/%{name}-%{version}.gem

* The LICENSE file should be part of the main package and marked as %doc. This is so that if only the main package is installed and not the docs subpackage (as usually is the case) the LICENSE is still included. 

http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

* Are sqlite3-ruby & activerecord really Requires or just BuildRequires (for tests)? Looking at the factory_girl source it seems like they are only pulled in for testing purposes.

* Can you fix the Rakefile with a patch in the spec (eg %patch0) instead of the hotfix in %install section.

* The %files section for both packages need default attributes:
 %defattr(-, root, root, -)

Thanks for this submission.

Comment 2 Michal Fojtik 2010-10-13 15:01:09 UTC
(In reply to comment #1)

Thanks!

> Will take this one. Overall looks good, some specific comments though
> 
> * Can you change source0 to point to the official gem hosted at
> http://rubygems.org/downloads/%{name}-%{version}.gem

FIXED.

> 
> * The LICENSE file should be part of the main package and marked as %doc. This
> is so that if only the main package is installed and not the docs subpackage
> (as usually is the case) the LICENSE is still included. 
> 
> http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text


FIXED.

> 
> * Are sqlite3-ruby & activerecord really Requires or just BuildRequires (for
> tests)? Looking at the factory_girl source it seems like they are only pulled
> in for testing purposes.

FIXED (Runtime dependencies removed)

> * Can you fix the Rakefile with a patch in the spec (eg %patch0) instead of the
> hotfix in %install section.

FIXED.

> * The %files section for both packages need default attributes:
>  %defattr(-, root, root, -)

FIXED.

=================== 1.3.2-1 ====================

http://koji.fedoraproject.org/koji/taskinfo?taskID=2532551

* Wed Oct 13 2010 Michal Fojtik <mfojtik@redhat.com> - 1.3.2-2
- Rakefile fixing moved to a separate patch
- Fixed unneeded Requires
- Fixed directory ownership on doc subpackage
- README and LICENSE moved back to main package

Spec URL: http://mifo.sk/RPMS/rubygem-factory_girl.spec
SRPM URL: http://mifo.sk/RPMS/rubygem-factory_girl-1.3.2-2.fc13.src.rpm

Comment 3 Mo Morsi 2010-10-13 17:24:50 UTC
Everything looks good save the patch for the Rakefile, the proper way to do it would be something like this in the %prep section:

%setup -q -c -T

mkdir -p ./%{gemdir}
gem install --local --install-dir ./%{gemdir} \
            --force --rdoc %{SOURCE0}

pushd ./%{geminstdir}
%patch0
popd



and then in the %install section

%install
rm -rf %{buildroot}
mkdir -p %{buildroot}%{gemdir}
cp -a .%{gemdir}/* %{buildroot}%{gemdir}


See rubygem-activerecord as an example of how to do this

http://pkgs.fedoraproject.org/gitweb/?p=rubygem-activerecord.git;a=blob;f=rubygem-activerecord.spec


Other than that, everything else looks great. rpmlint is fine, it builds on koji, and I did a surface functionality test.

Comment 4 Michal Fojtik 2010-10-14 11:30:15 UTC
(In reply to comment #3)
> Everything looks good save the patch for the Rakefile, the proper way to do it
> would be something like this in the %prep section:
> 
> %setup -q -c -T
> 
> mkdir -p ./%{gemdir}
> gem install --local --install-dir ./%{gemdir} \
>             --force --rdoc %{SOURCE0}
> 
> pushd ./%{geminstdir}
> %patch0
> popd
> 
> 
> 
> and then in the %install section
> 
> %install
> rm -rf %{buildroot}
> mkdir -p %{buildroot}%{gemdir}
> cp -a .%{gemdir}/* %{buildroot}%{gemdir}
> 
> 
> See rubygem-activerecord as an example of how to do this

Thanks for noticing that, it looks definitely better right now.
Fixed. 

> http://pkgs.fedoraproject.org/gitweb/?p=rubygem-activerecord.git;a=blob;f=rubygem-activerecord.spec
> 
> 
> Other than that, everything else looks great. rpmlint is fine, it builds on
> koji, and I did a surface functionality test.

==================== 1.3.2-2 ====================

* Thu Oct 14 2010 Michal Fojtik <mfojtik@redhat.com> - 1.3.2-3
- Replaced path with path macro

Spec URL: http://mifo.sk/RPMS/rubygem-factory_girl.spec
SRPM URL: http://mifo.sk/RPMS/rubygem-factory_girl-1.3.2-2.fc13.src.rpm

Comment 5 Mo Morsi 2010-10-14 18:31:13 UTC
It seems you forgot to bump the 'release' in the last spec to '3'. That;s the last nit that needs to be fixed though, everything else looks good.


APPROVED rubygem-factory_girl    [mmorsi]

Comment 6 Michal Fojtik 2010-10-17 19:22:16 UTC
Thanks!

New Package SCM Request
=======================
Package Name:      rubygem-factory_girl
Short Description: Framework and DSL for defining and using model instance factories
Owners:            mfojtik
Branches:          f12 f13 f14

Comment 7 Kevin Fenzi 2010-10-19 04:02:53 UTC
Git done (by process-git-requests).

Comment 8 Fedora Update System 2010-10-19 11:14:14 UTC
rubygem-factory_girl-1.3.2-3.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/rubygem-factory_girl-1.3.2-3.fc13

Comment 9 Fedora Update System 2010-10-21 05:57:33 UTC
rubygem-factory_girl-1.3.2-3.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update rubygem-factory_girl'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/rubygem-factory_girl-1.3.2-3.fc13

Comment 10 Fedora Update System 2010-11-04 23:48:19 UTC
rubygem-factory_girl-1.3.2-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.


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