Bug 646836

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.

(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

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.

One question.

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

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)

(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.

(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?

(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.

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

rubygem-thor has been updated to 0.14.6 in Fedora Rawhide

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.

Hm, this is evolving in some unexpected way :/

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

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.

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 ...

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

=======================
Package Name: rubygem-bundler
Short Description: Library and utilities to manages a Ruby application's gem dependencies
Owners: vondruch
Branches: 
InitialCC:

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.

Please submit a proper SCM request and re-raise the fedora-cvs flag.

=======================
Package Name: rubygem-bundler
Short Description: Library and utilities to manages a Ruby application's gem
dependencies
Owners: vondruch
Branches: 
InitialCC:

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.

there is not a complete scm branch request here. please fix and resubmit.

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:

Git done (by process-git-requests).

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.

Is it possible to build this for epel ?

(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.

(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.

Package Change Request
======================
Package Name: rubygem-bundler
New Branches: el5 el6
Owners: vondruch

Git done (by process-git-requests).

(In reply to comment #26)
Please ask for EPEL ACLs here: https://admin.fedoraproject.org/pkgdb/acls/name/rubygem-bundler