Bug 652396

Summary: Review Request: rubygem-boxgrinder-core - Core files required by BoxGrinder
Product: [Fedora] Fedora Reporter: Marek Goldmann <mgoldman>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, gholms, mtasaka, notting
Target Milestone: ---Flags: mtasaka: fedora‑review+
tibbs: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: rubygem-boxgrinder-core-0.1.3-3.fc13 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-11-30 17:31:40 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 653407    
Bug Blocks: 652400    
Attachments:
Description Flags
mock build log for rawhide none

Description Marek Goldmann 2010-11-11 14:27:24 EST
Spec URL: http://goldmann.fedorapeople.org/package_review/boxgrinder/rubygem-boxgrinder-core.spec
SRPM URL: http://goldmann.fedorapeople.org/package_review/boxgrinder/rubygem-boxgrinder-core-0.1.2-1.fc14.src.rpm
Description:

I would like to add BoxGrinder Build (http://www.jboss.org/boxgrinder) to Fedora. BoxGrinder is a command line tool for building appliances (virtual machines) to various platforms (KVM, Xen, VMware, EC2).

This package (boxgrinder-core) contains core files required by BoxGrinder family of projects.
Comment 1 Mamoru TASAKA 2010-11-14 14:48:15 EST
Some notes

* Licensing
  - It seems that openhash/openhash.rb comes from hashery gem
    ( the latest version of hashery gem is 1.3.0). Also the license
    of lib/hashery/openhash.rb in hashery 1.3.0 is currently under
    ASL 2.0.
    - Would you tell me where openhash/openhash.rb (in boxgrinder-core
      gem) came from?
    - Also would you show us if the license of openhash.rb is really
      under MIT?
    - And. bundling files in other projects is generally forbidden
      and needs FPC's approval. Would you consider to package
      rubygem-hashery seperately?

* BuildRoot line / %clean section
  - BuildRoot line is no longer needed on Fedora and EPEL6
  - %clean section is no longer needed on Fedora 12+ and EPEL6.

* Unneeded version specific dependency
  - " >= 1.2" part on "R: rubygems" or so are all unneeded
    ( Only that "= %{abi}" part on "R: ruby(abi)" is needed )
    https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires
    ( Please see the last 2 sentences in this section )

* Output file on %check
  - If %check section produces some extra files, it is preferable
    that %check section won't touch %buildroot tree to avoid
    unneeded confusion on %files and will touch files under
    %_builddir
    ( i.e. In this case, it is preferable that gem is once installed
      under %_builddir and then copy the whole tree to %buildroot
      at %install, then execute %check under %_builddir)

* Ability to build
  - Your srpm does not build on koji for dist-rawhide (at least).
    At least, "BR: rubygem(rake) rubygem(open4)" is needed (for %check)
    (and perhaps also rubygem(rspec))
Comment 2 Marek Goldmann 2010-11-15 05:12:22 EST
(In reply to comment #1)
> * Licensing
>   - It seems that openhash/openhash.rb comes from hashery gem
>     ( the latest version of hashery gem is 1.3.0). Also the license
>     of lib/hashery/openhash.rb in hashery 1.3.0 is currently under
>     ASL 2.0.

Yes, this is the current state of OpenHash.

>     - Would you tell me where openhash/openhash.rb (in boxgrinder-core
>       gem) came from?

As you pointed out, it was used from hashery: https://github.com/rubyworks/hashery/blob/master/lib/hashery/openhash.rb

I added small modification in line 88 and license header.

>     - Also would you show us if the license of openhash.rb is really
>       under MIT?

It seems that they moved to Apache License at some point: https://github.com/rubyworks/hashery/commit/0934b591ee716235a17d4ee691257bed291e36fb

I used the code before the license switch.

>     - And. bundling files in other projects is generally forbidden
>       and needs FPC's approval. Would you consider to package
>       rubygem-hashery seperately?

Ouch. Yes, this sounds like a good idea but in my case still I would need to override the method_missing (which is ~50% of the code :)) code to meet my needs.
 
> * BuildRoot line / %clean section
>   - BuildRoot line is no longer needed on Fedora and EPEL6

I added BuildRoot tag because rpmlint highlighted it as a warning. Removing.

>   - %clean section is no longer needed on Fedora 12+ and EPEL6.

OK, good to know – this is my first package.

> * Unneeded version specific dependency
>   - " >= 1.2" part on "R: rubygems" or so are all unneeded
>     ( Only that "= %{abi}" part on "R: ruby(abi)" is needed )
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires
>     ( Please see the last 2 sentences in this section )

OK, I'll remove unnecessary requires.

> * Output file on %check
>   - If %check section produces some extra files, it is preferable
>     that %check section won't touch %buildroot tree to avoid
>     unneeded confusion on %files and will touch files under
>     %_builddir
>     ( i.e. In this case, it is preferable that gem is once installed
>       under %_builddir and then copy the whole tree to %buildroot
>       at %install, then execute %check under %_builddir)

Right, I didn't know how to deal with that. Should it look like this?

%install
rm -rf %{buildroot}
rm -rf %{_builddir}%{gemdir}

mkdir -p %{_builddir}%{gemdir}
gem install --local --install-dir %{_builddir}%{gemdir} \
            --force --rdoc %{SOURCE0}
mkdir -p %{buildroot}/%{gemdir}
cp -r %{_builddir}%{gemdir}/* %{buildroot}/%{gemdir}

%check
pushd %{_builddir}/%{geminstdir}
rake spec
popd

> * Ability to build
>   - Your srpm does not build on koji for dist-rawhide (at least).
>     At least, "BR: rubygem(rake) rubygem(open4)" is needed (for %check)
>     (and perhaps also rubygem(rspec))

OK, I'll take a look at this issue.

Thanks for the first review, I'll fix my issues and get back to you.
Comment 3 Marek Goldmann 2010-11-15 06:09:57 EST
I'll move to Hashery gem, therefore I created a new review request for rubygem-hashery: BZ#653407.
Comment 5 Mamoru TASAKA 2010-11-15 14:21:16 EST
Will review.
Comment 6 Marek Goldmann 2010-11-15 14:27:18 EST
I found unnecessary "Requires: rubygems" – I'll remove it in the next version with all your comments for this spec file.
Comment 7 Mamoru TASAKA 2010-11-17 15:07:08 EST
Created attachment 461153 [details]
mock build log for rawhide

For 0.1.3-1:

Well, the spec file in the srpm and the spec file in
your comment 4 differ, however:

* BR
  - Still your srpm does not build (build log attached).
    At least "BR: rubygem(echoe)" is needed.

! Cleaning up %buildroot at %install
-------------------------------------------
%install
rm -rf %{buildroot}
rm -rf %{_builddir}%{gemdir}
-------------------------------------------
  - Well, on Fedora these lines are no longer needed
    (%buildroot is anyway cleaned at the beginning of %install)

* About output file in %check
> > * Output file on %check
> >   - If %check section produces some extra files, it is preferable
> >     that %check section won't touch %buildroot tree to avoid
> >     unneeded confusion on %files and will touch files under
> >     %_builddir
> >     ( i.e. In this case, it is preferable that gem is once installed
> >       under %_builddir and then copy the whole tree to %buildroot
> >       at %install, then execute %check under %_builddir)
> 
> Right, I didn't know how to deal with that. Should it look like this?
  - Please use "%setup -q -T". Usually we do like
---------------------------------------------
%prep
%setup -q -c -T
mkdir -p .%{gemdir}

gem install -V --local --install-dir .%{gemdir} \
	--force --rdoc %{SOURCE0}

%build

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

* Dependency
  - I think installed .gemspec (under %{gemdir}/specifications/)
    should have runtime dependency for "hashery".
Comment 8 Marek Goldmann 2010-11-17 16:36:11 EST
(In reply to comment #7)
> Created attachment 461153 [details]
> mock build log for rawhide
> 
> For 0.1.3-1:
> 
> Well, the spec file in the srpm and the spec file in
> your comment 4 differ, however:
> 
> * BR
>   - Still your srpm does not build (build log attached).
>     At least "BR: rubygem(echoe)" is needed.

Added! Sorry for missing that...

> ! Cleaning up %buildroot at %install
> -------------------------------------------
> %install
> rm -rf %{buildroot}
> rm -rf %{_builddir}%{gemdir}
> -------------------------------------------
>   - Well, on Fedora these lines are no longer needed
>     (%buildroot is anyway cleaned at the beginning of %install)

Removed.

> > Right, I didn't know how to deal with that. Should it look like this?
>   - Please use "%setup -q -T". Usually we do like
> ---------------------------------------------
> %prep
> %setup -q -c -T
> mkdir -p .%{gemdir}
> 
> gem install -V --local --install-dir .%{gemdir} \
>  --force --rdoc %{SOURCE0}
> 
> %build
> 
> %install
> mkdir -p %{buildroot}%{gemdir}
> cp -a .%{gemdir}/* %{buildroot}%{gemdir}/
> ---------------------------------------------

Ah, OK, adjusted.

> * Dependency
>   - I think installed .gemspec (under %{gemdir}/specifications/)
>     should have runtime dependency for "hashery".

True, added!

Spec URL:
http://goldmann.fedorapeople.org/package_review/boxgrinder/rubygem-boxgrinder-core.spec
SRPM URL:
http://goldmann.fedorapeople.org/package_review/boxgrinder/rubygem-boxgrinder-core-0.1.3-3.fc14.src.rpm

Thank you for your help!
Comment 9 Mamoru TASAKA 2010-11-18 13:23:12 EST
Okay.

-------------------------------------------------------
    This package (rubygem-boxgrinder-core) is
    APPROVED by mtasaka
-------------------------------------------------------
Comment 10 Mamoru TASAKA 2010-11-18 14:18:52 EST
By the way I would appreciate it if you have some time to
review my rubygem-related review request (bug 639636)
Comment 11 Marek Goldmann 2010-11-18 17:03:21 EST
Thanks for review! Will try to review your package shortly!

New Package SCM Request
=======================
Package Name:      rubygem-boxgrinder-core
Short Description: Core files required by BoxGrinder
Owners:            goldmann
Branches:          f13 f14
Comment 12 Jason Tibbitts 2010-11-18 18:57:38 EST
Git done (by process-git-requests).
Comment 13 Mamoru TASAKA 2010-11-20 12:37:38 EST
For F-15, you can build this package using chain-build
(or just wait a few hours after building rubygem-hashery on rawhide)

For F-14/13, if you want to build this package now (i.e. without
waiting one week before rubygem-hashery being pushed into stable),
you can ask rel-eng team to get rubygem-hashery tagged as dist-fXX-override
to get rubygem-hashery pulled in buildroot.
Comment 14 Marek Goldmann 2010-11-21 14:58:15 EST
Rawhide build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2613414
Comment 15 Marek Goldmann 2010-11-22 15:03:06 EST
rubygem-hashery tagged dist-f13-override and dist-f14-override
Comment 16 Mamoru TASAKA 2010-11-22 15:33:31 EST
Umm.. for F-14/13, it seems that you have to wait for rubygem-echoe
(owned by mfojtik) to get pushed into stable...
(It seems that today 7 testing days delay will be completed so
 I expected that F-14/13 rubygem-echoe will appear on stable within
 1-2 days)
Comment 17 Marek Goldmann 2010-11-22 15:43:13 EST
(In reply to comment #16)
> Umm.. for F-14/13, it seems that you have to wait for rubygem-echoe
> (owned by mfojtik) to get pushed into stable...

Yes, just saw it too (after my build failed). BTW, shouldn't rubygem-hashery be available now automatically for my build? Or do I need to wait some time too?

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

From root.log:

DEBUG util.py:260:  No Package Found for rubygem(echoe)
DEBUG util.py:260:  No Package Found for rubygem(hashery)
Comment 18 Mamoru TASAKA 2010-11-22 15:51:40 EST
(In reply to comment #17)
> (In reply to comment #16)
> > Umm.. for F-14/13, it seems that you have to wait for rubygem-echoe
> > (owned by mfojtik) to get pushed into stable...
> 
> Yes, just saw it too (after my build failed). BTW, shouldn't rubygem-hashery be
> available now automatically for my build? Or do I need to wait some time too?
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=2617437
> 
> From root.log:
> 
> DEBUG util.py:260:  No Package Found for rubygem(echoe)
> DEBUG util.py:260:  No Package Found for rubygem(hashery)

For rubygem-hashery, perhaps now okay (it seems that
your build was about 10 min earlier than override-tagged rubygem-hashery
came into F-13 buildroot)
Comment 19 Marek Goldmann 2010-11-23 02:50:36 EST
Now waiting for rubygem-echoe only to build the package, which was pushed to stable. Should be there today.
Comment 20 Fedora Update System 2010-11-26 06:52:07 EST
rubygem-boxgrinder-core-0.1.3-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/rubygem-boxgrinder-core-0.1.3-3.fc14
Comment 21 Fedora Update System 2010-11-26 06:54:54 EST
rubygem-boxgrinder-core-0.1.3-3.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/rubygem-boxgrinder-core-0.1.3-3.fc13
Comment 22 Fedora Update System 2010-11-26 16:05:40 EST
rubygem-boxgrinder-core-0.1.3-3.fc14 has been pushed to the Fedora 14 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-boxgrinder-core'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/rubygem-boxgrinder-core-0.1.3-3.fc14
Comment 23 Fedora Update System 2010-11-30 17:31:10 EST
rubygem-boxgrinder-core-0.1.3-3.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 24 Fedora Update System 2010-11-30 17:32:39 EST
rubygem-boxgrinder-core-0.1.3-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 25 Marek Goldmann 2011-01-25 03:01:47 EST
Package Change Request
======================
Package Name: rubygem-boxgrinder-core
New Branches: el5 el6
Owners: goldmann
Comment 26 Jason Tibbitts 2011-01-25 09:08:10 EST
Git done (by process-git-requests).