Bug 1132670

I'll take this for a review.

* The .spec differences
  - The .spec file you posted differs from the one included in SRPM. It is just
    small difference in you name in %changelog, so no big deal.

* Update to the latest guidelines
  - The .spec file looks to be generated by old gem2rpm and does not conform
    to the latest Ruby packaging guidelines. Please remove the Requires, which
    are now autogenerated and Provides.

* Test suite is not executed.
  - Please execute the test suite.
  - Although it is not shipped with the gem, it is available upstream [1].

* md5 usage
  - it seems that Compass uses md5 in
    lib/compass/core/sass_extensions/functions/files.rb . Since this hash
    algorithm is considered insecure and it is not supported on FIPS compliant
    machines, could you please query upstream about it? This is not blocker,
    just NTH issue I noticed.


[1] https://github.com/Compass/compass/tree/stable/core

Thx for review. Updated:

Spec: https://mmorsi.fedorapeople.org/staging/rubygem-compass-core.spec
SRPM: http://mmorsi.fedorapeople.org/staging/rubygem-compass-core-1.0.1-2.fc20.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=7476096

The upstream compass test suite isn't minitest5 compatible yet, filed a few PR's and will update the latest to the most recent master shortly. After that we can backport the updates to 1.0.1 and should be able to include the test suite in compass.

See this issue regarding the md5 algorithm:

https://github.com/Compass/compass/pull/1266

(In reply to Mo Morsi from comment #3)
> The upstream compass test suite isn't minitest5 compatible yet, filed a few
> PR's and will update the latest to the most recent master shortly. After
> that we can backport the updates to 1.0.1 and should be able to include the
> test suite in compass.

Well there are several options how to execute the test suite:

1) Patch, including submitting it upstream.
2) Monkey patching, e.g. rubygem-backports [1]
3) BR: rubygem(minitest) < 5


> See this issue regarding the md5 algorithm:
> 
> https://github.com/Compass/compass/pull/1266

Thanks, I added comment there.

[1] http://pkgs.fedoraproject.org/cgit/rubygem-backports.git/tree/rubygem-backports.spec#n55

Spec: https://mmorsi.fedorapeople.org/staging/rubygem-compass-core.spec
SRPM: http://mmorsi.fedorapeople.org/staging/rubygem-compass-core-1.0.1-3.fc20.src.rpm

Updated to run tests. Updated test suite pulls in rubygem-true, which has been seperately packaged / submitted. Minitest5 patch backported.

* Test suite
  - I'd suggest to execute the test suite using following command:

    ruby -Ilib:test/units \
      -e 'Dir.glob "./test/units/*_test.rb", &method(:require)'

  - Please expand the test suite in %check section. This way you keep in RPM
    just the content which was originally shipped by upstream. This also avoids
    several rpmlint complains.
  - It seems that the timecop and true are needed just for integration
    tests. Since there is circular dependency between true and compass, I'd
    suggest to use boostrap macro [1] to disable the integration tests during
    bootstrap.

Otherwise the package is legit => APPROVED


[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Bootstrapping

(In reply to Vít Ondruch from comment #6)
> * Test suite
>   - I'd suggest to execute the test suite using following command:
> 
>     ruby -Ilib:test/units \
>       -e 'Dir.glob "./test/units/*_test.rb", &method(:require)'

Updated

> 
>   - Please expand the test suite in %check section. This way you keep in RPM
>     just the content which was originally shipped by upstream. This also
> avoids
>     several rpmlint complains.

Assuming this meant exclude the test suite in the %files section. In which case done.

>   - It seems that the timecop and true are needed just for integration
>     tests. Since there is circular dependency between true and compass, I'd
>     suggest to use boostrap macro [1] to disable the integration tests during
>     bootstrap.

Currently the integration tests are not being run anyways so just ommitting this extra bit since its not needed. Once true is in we can update the spec to run those tests.

> 
> Otherwise the package is legit => APPROVED
> 

Thanks

Final spec: https://mmorsi.fedorapeople.org/staging/rubygem-compass-core.spec
Final srpm: https://mmorsi.fedorapeople.org/staging/rubygem-compass-core-1.0.1-4.fc22.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=8467442

New Package SCM Request
=======================
Package Name: rubygem-compass-core
Short Description:  The Compass core stylesheet library
Upstream URL: http://rubygems.org/gems/compass-core
Owners: mmorsi
Branches: f20 f21
InitialCC:

Git done (by process-git-requests).