Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://mmorsi.fedorapeople.org/staging/rubygem-compass-core.spec SRPM URL: http://mmorsi.fedorapeople.org/staging/rubygem-compass-core-1.0.1-1.fc20.src.rpm Description: The Compass core stylesheet library and minimum required ruby extensions. This library can be used stand-alone without the compass ruby configuration file or compass command line tools. Fedora Account System Username: mmorsi Additional Info: Required to update rubygem-compass to the latest version
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).