Bug 878243 - Review Request: rubygem-compass-rails - Integrate Compass into Rails 2.3 and up
Summary: Review Request: rubygem-compass-rails - Integrate Compass into Rails 2.3 and up
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Vít Ondruch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-11-19 22:41 UTC by Troy Dawson
Modified: 2013-01-12 00:23 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-01-12 00:23:51 UTC
Type: ---
vondruch: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Troy Dawson 2012-11-19 22:41:02 UTC
Spec URL: http://tdawson.fedorapeople.org/rubygems/rubygem-compass-rails.spec
SRPM URL: http://tdawson.fedorapeople.org/rubygems/rubygem-compass-rails-1.0.3-1.fc18.src.rpm
Description: Integrate Compass into Rails 2.3 and up.
Fedora Account System Username: tdawson

Comment 1 Troy Dawson 2012-11-19 22:42:25 UTC
RPMLINT OUTPUT:

$ rpmlint /home/quake/rpmbuild/SRPMS/rubygem-compass-rails-1.0.3-1.fc18.src.rpm /home/quake/rpmbuild/RPMS/noarch/rubygem-compass-rails-1.0.3-1.fc18.noarch.rpm /home/quake/rpmbuild/RPMS/noarch/rubygem-compass-rails-doc-1.0.3-1.fc18.noarch.rpm rubygem-compass-rails.spec
3 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 2 Vít Ondruch 2012-12-05 09:36:36 UTC
I'm going to review this.

Comment 3 Vít Ondruch 2012-12-05 11:23:55 UTC
* Wrong license
  - The LICENSE file states MIT. Please fix the license

* Remove "Requires: ruby"
  - Since we are going to have better integration with JRuby in F19, this would
    force the dependency on MRI, while it should work with JRuby as well.

* compass-rails is incompatible with compass in Fedora
  - Testing with F19:

    $ cd compass-rails-1.0.3/
    $ ruby -Ilib -e 'require "compass-rails"'
    /compass-rails-1.0.3/lib/compass-rails.rb:228:in `<top (required)>':
            undefined method `register' for Compass::AppIntegration:Module
            (NoMethodError)
	from /usr/share/rubygems/rubygems/custom_require.rb:36:in `require'
	from /usr/share/rubygems/rubygems/custom_require.rb:36:in `require'
	from -e:1:in `<main>'

* Test suite does not passes
  - It is executed, but 0 tests is executed, since they fail on missing
    dependencies. You need to add "BuildRequires: rubygem(compass)" at least,
    but then you'll be bitten by the incompatibility with rubygem-compass
    mentioned above.
  - I would suggest to use rubygem(minitest) for testing instead of
    rubygem(test-unit), if possible.
  - It seems that the tests depends on Bundler. It will be needed to get rid of
    it I guess.

* Move file not needed for runtime into -doc subpackage
  - Following files are not needed by runtime IMO

    %{gem_instdir}/Appraisals
    %{gem_instdir}/Gemfile
    %{gem_instdir}/Guardfile

    Please consider to move them into -doc subpackage.
  - I would move the %doc %{gem_instdir}/README.md into -doc as well, but I'll
    keep it up to you.

Comment 4 Troy Dawson 2012-12-05 22:32:23 UTC
Spec URL: http://tdawson.fedorapeople.org/rubygems/rubygem-compass-rails.spec
SRPM URL: http://tdawson.fedorapeople.org/rubygems/rubygem-compass-rails-1.0.3-2.fc18.src.rpm

License - Fixed
Doc subpackage - moved all suggested files in there
Requires: ruby - removed  (I'll have to start doing that from now on)

Test Suite:
I didn't notice that it had bypassed all the tests and returned with a positive outcome.  I wish it wouldn't do that.
I tried with minitest, building and installing all the dependancies (compass > 0.12, and rainbow), and after all that, it didn't run any tests either.

Even without the tests, this package cannot be installed, because it requires rubygem-compass > 0.12.2.  It looks like mmorsi is trying to get it working but it's currently failling on F19 (while it build on F18 for me)

http://koji.fedoraproject.org/koji/packageinfo?packageID=10224

Comment 5 Vít Ondruch 2012-12-06 08:50:38 UTC
(In reply to comment #4)
> Spec URL: http://tdawson.fedorapeople.org/rubygems/rubygem-compass-rails.spec
> SRPM URL:
> http://tdawson.fedorapeople.org/rubygems/rubygem-compass-rails-1.0.3-2.fc18.
> src.rpm
> 
> License - Fixed
> Doc subpackage - moved all suggested files in there
> Requires: ruby - removed  (I'll have to start doing that from now on)

Thank you. Will check that later.

> Test Suite:
> I didn't notice that it had bypassed all the tests and returned with a
> positive outcome.  I wish it wouldn't do that.
> I tried with minitest, building and installing all the dependancies (compass
> > 0.12, and rainbow), and after all that, it didn't run any tests either.
> 
> Even without the tests, this package cannot be installed, because it
> requires rubygem-compass > 0.12.2.  It looks like mmorsi is trying to get it
> working but it's currently failling on F19 (while it build on F18 for me)
> 
> http://koji.fedoraproject.org/koji/packageinfo?packageID=10224

Yes, I noticed that as well. Are you in contact with him? I'll continue with this review as soon as updated compass gets into Fedora.

Comment 6 Troy Dawson 2012-12-06 21:02:45 UTC
I sent mmorsi an email today.  We'll see how quick it can be updated.
His package builds on F18, but it fails on the tests in rawhide (F19)

I'll make some noise on this when rubygem-compass is updated.

Comment 7 Troy Dawson 2012-12-20 22:30:01 UTC
rubygem-compass 0.12.2 is now in Fedora 19, 18, and 17.
Please continue with the review.

Comment 8 Vít Ondruch 2012-12-21 15:40:30 UTC
(In reply to comment #4)
> Requires: ruby - removed  (I'll have to start doing that from now on)

I am negative about this change:
$ grep "Requires: *ruby$" rubygem-compass-rails.spec 
Requires:      ruby


> Test Suite:
> I didn't notice that it had bypassed all the tests and returned with a
> positive outcome.  I wish it wouldn't do that.
> I tried with minitest, building and installing all the dependancies (compass
> > 0.12, and rainbow), and after all that, it didn't run any tests either.

Hm, the test suite is insane :/ It looks more like test suite of Bundler then test suite of simple gem. ATM, I would suggest you to disable the test suite including all the BR and if you don't mind, please ask the upstream if it is possible to run the test suite with limited usage of bundler and just against Rails 3.2 and document the situation in .spec file.

BTW it would be better to run the test suite in .%{gem_instdir} if it will be possible one day in the future.

And I have on last minor nit:

* Keep the %{gem_instdir}/gemfiles
  - I would keep this files in -doc subpackage. Upstream distributes them, so
    let's keep them

Otherwise, the package looks good => APPROVED. Please fix the above mentioned issue prior importing package into Fedora.

Comment 9 Troy Dawson 2012-12-21 17:05:39 UTC
New Package SCM Request
=======================
Package Name: rubygem-compass-rails0
Short Description: Integrate Compass into Rails 2.3 and up
Owners: tdawson
Branches: f17 f18
InitialCC:

Comment 10 Gwyn Ciesla 2012-12-21 17:08:05 UTC
WARNING: Requested package name rubygem-compass-rails0 doesn't match bug
summary rubygem-compass-rails

Comment 11 Troy Dawson 2012-12-21 17:22:06 UTC
New Package SCM Request
=======================
Package Name: rubygem-compass-rails
Short Description: Integrate Compass into Rails 2.3 and up
Owners: tdawson
Branches: f17 f18
InitialCC:

Comment 12 Gwyn Ciesla 2012-12-21 17:28:53 UTC
Git done (by process-git-requests).

Comment 13 Troy Dawson 2012-12-21 18:15:09 UTC
Vit, just a word on the version I'm building

- requires: ruby - removed (really removed this time)
- %{gem_instdir}/gemfiles - not removed, now in -doc
- Tests:
-- pushd ./%{gem_instdir}, with corresponding popd
-- commented out all tests with note to clean them up upstream

Thanks for the review.

Comment 14 Fedora Update System 2012-12-21 20:11:42 UTC
rubygem-compass-rails-1.0.3-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/rubygem-compass-rails-1.0.3-3.fc18

Comment 15 Fedora Update System 2012-12-22 21:14:35 UTC
rubygem-compass-rails-1.0.3-3.fc18 has been pushed to the Fedora 18 testing repository.

Comment 16 Fedora Update System 2013-01-12 00:23:54 UTC
rubygem-compass-rails-1.0.3-3.fc18 has been pushed to the Fedora 18 stable repository.


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