Bug 621015

Some notes:

* Unused macro
  - %ruby_sitelib seems to be used nowhere.

* Latest version
  - Latest version seems 2.1.6.

* Requires
  - ">= 0" part on "R: rubygem(activesupport)" is redundant.
  - "R: ruby(abi) = 1.8" is mandatory

* Documents
  - "%{geminstdir}/test" should be marked as %doc
  - Also "%{geminstdir}/Rakefile" can be marked as %doc (this is
    like "Makefile" in autotools system).

* %check
  - rubygem(jeweler) is not strictly needed for "$ rake test",
    so please enable "$ rake test".

  ! Some notes:
    - Without rubygem(ruby-debug) being installed, "$ rake test"
      aborts like
----------------------------------------------------------------
$ rake test --trace
(in /home/tasaka1/rpmbuild/INSTROOT/rubygem-authlogic-2.1.5-1.fc-foo-tasaka1/usr/lib/ruby/gems/1.8/gems/authlogic-2.1.5)
Jeweler (or a dependency) not available. Install it with: sudo gem install jeweler
** Invoke test (first_time)
** Execute test
....
....
/usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:31:in `gem_original_require': no such file to load -- ruby-debug (LoadError)
	from /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:31:in `require'
	from ./test/test_helper.rb:3
	from ./test/i18n_test.rb:1:in `require'
	from ./test/i18n_test.rb:1
	from /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb:4:in `load'
	from /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb:4
	from /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb:4:in `each'
	from /usr/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb:4
rake aborted!
----------------------------------------------------------------
      However, in fact "ruby-debug" does not seem to be needed.

    - After commenting out 'require "ruby-debug"' from ./test/test_helper.rb,
      some tests fails like
----------------------------------------------------------------
$ rake test --trace
(in /home/tasaka1/rpmbuild/INSTROOT/rubygem-authlogic-2.1.5-1.fc-foo-tasaka1/usr/lib/ruby/gems/1.8/gems/authlogic-2.1.5)
Jeweler (or a dependency) not available. Install it with: sudo gem install jeweler
** Invoke test (first_time)
** Execute test
....
....
  1) Error:
test_transitioning_password(ActsAsAuthenticTest::PasswordTest):
NameError: uninitialized constant BCrypt
    /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/dependencies.rb:443:in `load_missing_constant'
    /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/dependencies.rb:80:in `const_missing'
    /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/dependencies.rb:92:in `const_missing'
    ./test/../lib/authlogic/crypto_providers/bcrypt.rb:83:in `new_from_hash'
    ./test/../lib/authlogic/crypto_providers/bcrypt.rb:60:in `matches?'
    ./test/../lib/authlogic/acts_as_authentic/password.rb:259:in `valid_password?'
    /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/callbacks.rb:207:in `each_with_index'
    ./test/../lib/authlogic/acts_as_authentic/password.rb:253:in `each'
    ./test/../lib/authlogic/acts_as_authentic/password.rb:253:in `each_with_index'
    ./test/../lib/authlogic/acts_as_authentic/password.rb:253:in `valid_password?'
    ./test/acts_as_authentic_test/password_test.rb:224:in `transition_password_to'
    ./test/acts_as_authentic_test/password_test.rb:221:in `each'
    ./test/acts_as_authentic_test/password_test.rb:221:in `transition_password_to'
    ./test/acts_as_authentic_test/password_test.rb:163:in `test_transitioning_password'
    /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/testing/setup_and_teardown.rb:62:in `__send__'
    /usr/lib/ruby/gems/1.8/gems/activesupport-2.3.5/lib/active_support/testing/setup_and_teardown.rb:62:in `run'
---------------------------------------------------------------
      Maybe it is better that you package bcrypt-ruby?
      http://bcrypt-ruby.rubyforge.org/

(In reply to comment #1)
> Some notes:
> 
> * Unused macro
>   - %ruby_sitelib seems to be used nowhere.

Removed

> 
> * Latest version
>   - Latest version seems 2.1.6.

Updated

> 
> * Requires
>   - ">= 0" part on "R: rubygem(activesupport)" is redundant.
>   - "R: ruby(abi) = 1.8" is mandatory

Done

> 
> * Documents
>   - "%{geminstdir}/test" should be marked as %doc
>   - Also "%{geminstdir}/Rakefile" can be marked as %doc (this is
>     like "Makefile" in autotools system).

Done

> 
> * %check
>   - rubygem(jeweler) is not strictly needed for "$ rake test",
>     so please enable "$ rake test".

Done

> 
>   ! Some notes:
>     - Without rubygem(ruby-debug) being installed, "$ rake test"
>       aborts like
> ----------------------------------------------------------------
> $ rake test --trace
> <snip>
> ----------------------------------------------------------------
>       However, in fact "ruby-debug" does not seem to be needed.
> 
>     - After commenting out 'require "ruby-debug"' from ./test/test_helper.rb,
>       some tests fails like

Done, patch0 added to do this.

> ----------------------------------------------------------------
> $ rake test --trace
> <snip>
> ---------------------------------------------------------------
>       Maybe it is better that you package bcrypt-ruby?
>       http://bcrypt-ruby.rubyforge.org/    

Package review request submitted 
http://bugzilla.redhat.com/show_bug.cgi?id=622946

rubygem(bcrypt-ruby) is added as a BR(check) dependency. Once added, rake test successfully completes.

Updated SRPM / Spec here:
pec URL: http://mo.morsi.org/files/gems/rubygem-authlogic.spec
SRPM URL: http://mo.morsi.org/files/gems/rubygem-authlogic-2.1.6-1.fc13.src.rpm

For 2.1.6-1

* BR
  - So would you at least add "BR(check): rubygem(bcrypt-ruby)" (and
    "BR: rubygem(rake)" so as to make %check section pass correctly?

Some additional notes
* BuildRoot
  - BuildRoot tag is no longer used (and needed) on Fedora and EPEL6
    https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

* ri files generation
  - As I said in other review request (I suppose), usually we
    generate ri documentation files, which can be used with
    ri (in ruby-ri) command.

* -doc files generation
  - And would you consider to create -doc subpackages and move
    document files (except for LICENSE, README.rdoc, CHANGELOG.rdoc)
    to -doc subpackage?

What is the status of this bug?

ping again?

Apologies for the delay

(In reply to comment #3)
> For 2.1.6-1
> 
> * BR
>   - So would you at least add "BR(check): rubygem(bcrypt-ruby)" (and
>     "BR: rubygem(rake)" so as to make %check section pass correctly?
> 
> Some additional notes
> * BuildRoot
>   - BuildRoot tag is no longer used (and needed) on Fedora and EPEL6
>     https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
> 
> * ri files generation
>   - As I said in other review request (I suppose), usually we
>     generate ri documentation files, which can be used with
>     ri (in ruby-ri) command.
> 
> * -doc files generation
>   - And would you consider to create -doc subpackages and move
>     document files (except for LICENSE, README.rdoc, CHANGELOG.rdoc)
>     to -doc subpackage?

Everything has been taken care of. Also had to add patch1 to remove the jeweler dependency which is not needed.

New uploads:
Spec URL: http://mo.morsi.org/files/gems/rubygem-authlogic.spec
SRPM URL: http://mo.morsi.org/files/gems/rubygem-authlogic-2.1.6-2.fc13.src.rpm

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

Two comments

- I usually think that writing %doc attribute on -doc subpackage is
  redundant because the name of the rpm already shows that the rpm
  is for documentation.

- Maybe "Patch0: remove-unneeded-rubydebug-dep.patch" is no longer
  needed, because now rubygem-ruby-debug is available on F-12/13/14/15.
  (please try to remove Patch0 and adding BR: rubygem(ruby-debug))

Not blockers, approving.
---------------------------------------------------------
    This package (rubygem-authlogic) is APPROVED
    by mtasaka
---------------------------------------------------------

Ok, thanks for the approval, just quickly took care of those last two things you mentioned. For reference here are the newest links:

Spec URL: http://mo.morsi.org/files/gems/rubygem-authlogic.spec
SRPM URL: http://mo.morsi.org/files/gems/rubygem-authlogic-2.1.6-3.fc13.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2530947

New Package SCM Request
=======================
Package Name: rubygem-authlogic
Short Description: A simple ruby authentication solution
Owners: mmorsi
Branches: f13 f14
InitialCC:

Git done (by process-git-requests).

Sorry, for directory ownership issue, -doc subpackage
should have "Requires: %{name} = %{version}-%{release}".

rubygem-authlogic-2.1.6-4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/rubygem-authlogic-2.1.6-4.fc14

rubygem-authlogic-2.1.6-4.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/rubygem-authlogic-2.1.6-4.fc13

Ah good catch, though I had already pushed the rpm to git before I noticed this.

Pushed a followup, adding the requires, and submitted the package to bodhi.

rubygem-authlogic-2.1.6-4.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-authlogic'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/rubygem-authlogic-2.1.6-4.fc13

Closing.

rubygem-authlogic-2.1.6-4.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

rubygem-authlogic-2.1.6-4.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.