Bug 646836 - Review Request: rubygem-bundler - Library and utilities to manages a Ruby application's gem
Summary: Review Request: rubygem-bundler - Library and utilities to manages a Ruby app...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mo Morsi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 649028
Blocks: 665560
TreeView+ depends on / blocked
 
Reported: 2010-10-26 12:34 UTC by Jozef Zigmund
Modified: 2012-10-09 06:51 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-02-02 14:24:52 UTC
mmorsi: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Jozef Zigmund 2010-10-26 12:34:24 UTC
Spec URL: http://st.fri.uniza.sk/~zigmundj/fedorapkg/rubygem-bundler.spec
SRPM URL: http://st.fri.uniza.sk/~zigmundj/fedorapkg/rubygem-bundler-1.0.3-1.fc13.src.rpm
Description: It manages an application's dependencies through its entire life, across many machines, systematically and repeatably

Comment 1 Mamoru TASAKA 2010-10-26 19:35:47 UTC
Some notes:

* ruby(abi) dependency
  - Writing "Requires: ruby(abi) = 1.8" is a must
    https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines

* Consistent macro usage
  - As you defined %geminstdir macro, please use this macro also
    in %files section

* Document files
  - Please mark document files as %doc properly.
    - CHANGELOG.md or so should be marked as %doc
-------------------------------------------
CHANGELOG.md
ISSUES.md
README.md
UPGRADING.md
-------------------------------------------
    - Also, the following files
-------------------------------------------
Rakefile
bundler.gemspec (under %geminstdir, not the one under specifications/)
spec/
-------------------------------------------
      are not needed on runtime and should be marked as %doc.
      We usually move these "unneeded" files to -doc subpackage.
    - .gitignore file is not needed and should be removed

* Shipping external project
  - This gem bundles external project (for this package thor)
    under lib/bundler/vendor/. On Fedora shipping external project in
    the same package should (must) be avioded, see:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Bundling_of_multiple_projects

    As Fedora already ships rubygem-thor, please remove this vendorlized
    directory and modify scripts in bundler gem to use rubygem-thor rpm.

Comment 2 Jozef Zigmund 2010-11-02 16:17:11 UTC
(In reply to comment #1)
> Some notes:
> 
> * ruby(abi) dependency
>   - Writing "Requires: ruby(abi) = 1.8" is a must
>     https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Packaging_Guidelines

+ dependency Requires: ruby(abi) = 1.8

> * Consistent macro usage
>   - As you defined %geminstdir macro, please use this macro also
>     in %files section

+ macro %geminstdir in %files section
 
> * Document files
>   - Please mark document files as %doc properly.
>     - CHANGELOG.md or so should be marked as %doc
> -------------------------------------------
> CHANGELOG.md
> ISSUES.md
> README.md
> UPGRADING.md
> -------------------------------------------
>     - Also, the following files
> -------------------------------------------
> Rakefile
> bundler.gemspec (under %geminstdir, not the one under specifications/)
> spec/
> -------------------------------------------
>       are not needed on runtime and should be marked as %doc.
>       We usually move these "unneeded" files to -doc subpackage.

+ Add subpackage %files doc with these files, but i'm confused about this line "bundler.gemspec (under %geminstdir, not the one under specifications/)" because in http://fedoraproject.org/wiki/Packaging:Ruby#Ruby_Gems, is written:

The package must own the following files and directories:
%{gemdir}/gems/%{gemname}-%{version}/
%{gemdir}/cache/%{gemname}-%{version}.gem
%{gemdir}/specifications/%{gemname}-%{version}.gemspec

>     - .gitignore file is not needed and should be removed

+ Removed

> * Shipping external project
>   - This gem bundles external project (for this package thor)
>     under lib/bundler/vendor/. On Fedora shipping external project in
>     the same package should (must) be avioded, see:
>    
> https://fedoraproject.org/wiki/Packaging/Guidelines#Bundling_of_multiple_projects
> 
>     As Fedora already ships rubygem-thor, please remove this vendorlized
>     directory and modify scripts in bundler gem to use rubygem-thor rpm.

+Rubygem-thor has already been in fedora repos, so i removed this vendor directory and add require of this gem to Requires part.

New SPEC file: http://people.redhat.com/jzigmund/rubygem-bundler.spec
New SRPM file: http://people.redhat.com/jzigmund/rubygem-bundler-1.0.3-2.fc13.src.rpm

Comment 3 Jozef Zigmund 2010-11-02 16:45:24 UTC
Hi Mamoru, i want to request you if you will have time can you check my reviews for these packages, if it's correct?

https://bugzilla.redhat.com/show_bug.cgi?id=642583
https://bugzilla.redhat.com/show_bug.cgi?id=642592
https://bugzilla.redhat.com/show_bug.cgi?id=642601

Because I'm not still in Packager group so i made just nonformal reviews.

Thank you.

Comment 4 Mamoru TASAKA 2010-11-02 18:00:53 UTC
One question.

Did you already find someone who is willing to sponsor you?

Comment 5 Mamoru TASAKA 2010-11-02 19:25:21 UTC
Ah,
  currently removing vendorlized thor (in bundler gem) breaks bundler
  gem because of lower version of thor in Fedora than required by
  bundler... (reported on bug 649028)

Comment 6 Jozef Zigmund 2010-11-03 10:15:53 UTC
(In reply to comment #4)
> One question.
> 
> Did you already find someone who is willing to sponsor you?

Marek Mahut wrote me that he will be my sponsor (https://bugzilla.redhat.com/show_bug.cgi?id=642577#c5), but it hasn't happened yet. I'm a bit pissed off, because it takes long time.

Comment 7 Jozef Zigmund 2010-11-03 10:20:59 UTC
(In reply to comment #5)
> Ah,
>   currently removing vendorlized thor (in bundler gem) breaks bundler
>   gem because of lower version of thor in Fedora than required by
>   bundler... (reported on bug 649028)

OK, so now do we need to wait for updating of thor gem? How long time does it take?

Comment 8 Jozef Zigmund 2010-11-04 12:51:35 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > One question.
> > 
> > Did you already find someone who is willing to sponsor you?
> 
> Marek Mahut wrote me that he will be my sponsor
> (https://bugzilla.redhat.com/show_bug.cgi?id=642577#c5), but it hasn't happened
> yet. I'm a bit pissed off, because it takes long time.

Today, I became an user in Fedora 'packager' group.

Comment 9 Mo Morsi 2011-01-19 03:46:00 UTC
I have updated thor and will be pushing it to rawhide soon.

You may use this srpm / spec to finish up the review in the meantime 

SRPM: http://mo.morsi.org/files/rpms/rubygem-thor-0.14.6-1.fc14.src.rpm
SPEC: http://mo.morsi.org/files/rpms/rubygem-thor.spec

Comment 10 Mo Morsi 2011-01-23 17:25:43 UTC
rubygem-thor has been updated to 0.14.6 in Fedora Rawhide

Comment 11 Vít Ondruch 2011-01-24 17:23:54 UTC
Hello, I am taking over this gem from jzigmund (right Jozef?).

So here is update of the package:

Spec URL: http://people.redhat.com/vondruch/rubygem-bundler.spec
SRPM URL: http://people.redhat.com/vondruch/rubygem-bundler-1.0.9-1.fc14.src.rpm

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2739694

* I have bumped the specs to the latest Bundler.
* I have added installation of manual pages.

Unfortunately, I was not able to execute specs during build for 2 reasons:

1) The spec is RSpec 2 based, so there are some minor differences, however not a show stopper.
2) The removed vendorized Thor makes serious problems during testing. I wanted to run the specs against Thor installed as an dependency, however, there is heavily modificated the ruby load path during specs runtime, in the way, that manually specified path to external Thor is lost.

Also note that there is some strange relation between Thor and Bundler. I have reported this issue upstream: https://github.com/carlhuda/bundler/issues/#issue/982 

Nonetheless, non of this issues should prevent us from releasing this gem.

Comment 12 Vít Ondruch 2011-01-24 20:39:31 UTC
Hm, this is evolving in some unexpected way :/

https://github.com/carlhuda/bundler/issues/issue/982/#issue/982/comment/703782

Comment 13 Mo Morsi 2011-01-25 17:51:25 UTC
Hey Jozef, Vit thanks for the work on this so far. Is this package under official review, Jozef why did you flag fedora-review for a package you submitted? I can do the official review if noone else claimed it yet.

Regardless, I spent some time looking into the vendor'd thor issue, and luckily I think there is a way we can get around this. We might not need to though, as it seems removing the vendor directory will not affect the bundler runtime, just the test suite. Problems just running the test suite in the spec aren't blockers, as functionality can be independently verified on release / updates. I've verified thor is loaded correctly when bundler is used via an rpm install with the vendor directory removed, and that rails 3 builds and works fine ontop of it.

In any case the issue when running the spec suite after removing the vendor directory occurs because bundler foobars the ruby load_path and the rubygem load process in order to do its thing. Thus in lib/bundler/gem_helper.rb and lib/bundler/cli.rb, the "require 'thor'" relies on the vendor directory added to the load path and fails.

The thing when running the spec suite, rspec is loaded successfully. It is able to because the rspec library itself modifies the load path (at least rspec 2 does), appending the /usr/lib/ruby/gems/1.8/gems/rspec-* directories. So to get thor to load correctly in this situation, we need to modify the load path as well. So before the "require 'thor'" in cli.rb and gem_helper.rb, comment out the bit adding the vendor directory and add

$:.unshift "/usr/lib/ruby/gems/1.8/gems/thor-0.14.6/lib"

Now obviously this isn't an optimal solution (or even an acceptable one) due to the inclusion of the hard coded path. We can get some support from ruby itself to replace some of the fixed components, for example the Config::CONFIG hash (available via the 'rbconfig' module) can be leveraged to give us some of the ruby installation directories, and perhaps we can use rpm itself to give us the thor install location. We can't use Gem.path though as bundler overwrites that during the setup process.

Regardless, I've verified the spec suite and the package itself is working against the rubygem-thor package in Fedora (with the vendor directory removed). There are two errors (468 successes) in the test suite, but these also occur when the vendorized thor directory is used (we might be able to patch the spec suite to fix those two tests). Thus as you said, this non-inclusion of the test suite is not a blocker.

I will officially review this package soon if noone else has claimed it first.

Comment 14 Vít Ondruch 2011-01-26 15:59:13 UTC
I am comparing the bundler Thor versus the gem Thor. It seems that the bundled version is Thor 0.14.0 with backported most of the patches contained in Thor 0.14.2. 

Current Thor 0.14.6 seems to be superset (which is not surprising) and there were no API changes, just additions.

So it seems that removing bundled Thor should be safe for runtime.

Looking once more into the spec issue ...

Comment 15 Mo Morsi 2011-01-26 20:32:50 UTC
OK, here is my official review

* Koji looks good

* rpmlint looks good

* package works

* package complies to guidelines

* The Summary is a little vague

http://fedoraproject.org/wiki/Packaging/Guidelines#Summary_and_description

Perhaps it can be reworded to state exactly what it does ("Library and utilities to manages a Ruby application's gem dependencies" or something similar). Also please consider substituting "It" for "Bundler" in the %description section, though this is just a minor nit.


Other than the Summary, this package looks good.


APPROVED

Comment 16 Vít Ondruch 2011-01-27 09:18:06 UTC
=======================
Package Name: rubygem-bundler
Short Description: Library and utilities to manages a Ruby application's gem dependencies
Owners: vondruch
Branches: 
InitialCC:

Comment 17 Vít Ondruch 2011-01-27 09:20:36 UTC
BTW I have finally found how to execute the test suite:
RUBYOPT="$RUBYOPT I/usr/lib/ruby/gems/1.8/gems/thor-0.14.6/lib" spec spec

I just have finish conversion for RSpec 1.3 and integrate it.

Comment 18 Jason Tibbitts 2011-01-27 15:04:44 UTC
Please submit a proper SCM request and re-raise the fedora-cvs flag.

Comment 19 Vít Ondruch 2011-01-28 08:23:40 UTC
=======================
Package Name: rubygem-bundler
Short Description: Library and utilities to manages a Ruby application's gem
dependencies
Owners: vondruch
Branches: 
InitialCC:

Comment 20 Vít Ondruch 2011-01-28 12:05:01 UTC
Spec URL: http://people.redhat.com/vondruch/rubygem-bundler.spec
SRPM URL:
http://people.redhat.com/vondruch/rubygem-bundler-1.0.9-2.fc14.src.rpm

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

Mo, could you please one more take a look? I did there changes in attempt to make the test suite alive (however not successful for many reasons :/ ), etc.

Comment 21 Dennis Gilmore 2011-01-31 00:09:27 UTC
there is not a complete scm branch request here. please fix and resubmit.

Comment 22 Vít Ondruch 2011-01-31 08:51:49 UTC
New Package SCM Request
=======================
Package Name: rubygem-bundler
Short Description: Library and utilities to manages a Ruby application's gem
dependencies
Owners: vondruch
Branches: 
InitialCC:

Comment 23 Jason Tibbitts 2011-01-31 16:45:24 UTC
Git done (by process-git-requests).

Comment 24 Mo Morsi 2011-01-31 19:11:44 UTC
The updated rpm looks good, though I didn't extensively go into your patch changing the spec suite since we're not using that anyways. Perhaps it might not be a bad idea to comment out / remove that patch for the time being until we can get the spec suite fully working in Fedora, but it doesn't affect the build or runtime so its not a blocker.

Minor nit, in the 'summary' the word 'manages' should be 'manage'. nbd though. Would say go ahead a push the package as is, we can work on getting the spec suite working in the rpm from them.

Comment 25 Shreyank Gupta 2012-10-05 15:02:27 UTC
Is it possible to build this for epel ?

Comment 26 Vít Ondruch 2012-10-08 09:56:01 UTC
(In reply to comment #25)
> Is it possible to build this for epel ?

Well, I am not interested in EPEL ATM, but if you are willing to maintain it, I can create the branch for you and confirm the ACL.

Comment 27 Shreyank Gupta 2012-10-08 10:42:51 UTC
(In reply to comment #26)
> (In reply to comment #25)
> > Is it possible to build this for epel ?
> 
> Well, I am not interested in EPEL ATM, but if you are willing to maintain
> it, I can create the branch for you and confirm the ACL.

That will be great. Thanks.

Comment 28 Vít Ondruch 2012-10-08 11:15:25 UTC
Package Change Request
======================
Package Name: rubygem-bundler
New Branches: el5 el6
Owners: vondruch

Comment 29 Jason Tibbitts 2012-10-08 16:20:33 UTC
Git done (by process-git-requests).

Comment 30 Vít Ondruch 2012-10-09 06:51:39 UTC
(In reply to comment #26)
Please ask for EPEL ACLs here: https://admin.fedoraproject.org/pkgdb/acls/name/rubygem-bundler


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