Spec URL: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01689521-rubygem-sassc-rails/rubygem-sassc-rails.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01689521-rubygem-sassc-rails/rubygem-sassc-rails-2.1.2-1.fc34.src.rpm Description: Integrate SassC-Ruby into Rails. Fedora Account System Username: pvalena Automated test: https://git.io/JUMd4 Fedora-review: https://git.io/JUMdv This is a successor of rubygem-sass-rails: https://src.fedoraproject.org/rpms/rubygem-sass-rails/pull-request/1
Note that te test suite is troublesome at this moment. I didn't manage to make the tests work, due to paths (assets not found, or even SassC). And CSS 'compressor' doesn't seem to work. I will try to resolve those issues upstream.
I'm taking this for a review.
* Weird RexExp - The 'g' in the following RegExp is probably not needed, since you are matching single line anyway: ~~~ sed -i -e '/require .pry./ s/^/#/g' test/test_helper.rb ~~~ * Test suite - It seems you got completely drown. If you started with the rubygem-sass-rails, it would be easier. This is where I got: ~~~ ... snip ... BuildRequires: rubygem(bundler) BuildRequires: rubygem(mocha) BuildRequires: rubygem(railties) BuildRequires: rubygem(sassc) BuildRequires: rubygem(sprockets-rails) BuildRequires: rubygem(tilt) ... snip ... %check pushd .%{gem_instdir} # Copy in .gemspec and use the sass-rails sources cp %{buildroot}%{gem_spec} sassc-rails.gemspec # Avoid unnecessary dependency sed -i -e '/require .pry./ s/^/#/g' test/test_helper.rb sed -i -e '/dependency.*pry./ s/^/#/' sassc-rails.gemspec sed -i -e '/dependency.*rake./ s/^/#/' sassc-rails.gemspec ruby -e 'Dir.glob "./test/**/*.rb", &method(:require)' popd ... snip ... ~~~ - BTW some of the failures could be, because you have required `rubygem(sass)` instead of `rubygem(sassc)` (note the 'c' difference).
* Licensing - Please check the license of test/dummy/app/assets/stylesheets/erb_render_with_context.css.erb. It seems the package should include the `OFL` license. But probably better to check with upstream.
Ping?
Hello, sorry for the delay. Unfortunately, I forgot to continue to work on this, after getting initially stuck with the test suite. Now I was able to debug it, and all the tests pass. I've a also fixed the license for the font (interesting is that licensecheck didn't find it). Resulting in changes on top of original spec: https://git.io/JTiyQ Up-to-date Copr build (also has spec file): https://copr.fedorainfracloud.org/coprs/build/1722749 I've also run some automated checks: - Tests: ok - Syntax check: ok - Dependent packages: ok - Smoke test: ok - rpmlint: ok (false positives) _ _ _ _ Test log: cpr/rubygem-sassc-rails_test.log gem2rpm diff: cpr/rubygem-sassc-rails_gem2rpm.diff _ _ _ _ Additionaly: > * Weird RexExp > - The 'g' in the following RegExp is probably not needed, since you are matching single line anyway: Doesn't make much difference then? I'm used to write regexes like that... (removed) > * Test suite > - It seems you got completely drown. If you started with the rubygem-sass-rails, it would be easier. This is where I got: Yes, I was in a hurry to have it built :) ... (weird that I forgot afterwards).
Sorry, this is still on my TODO list
Just to make it clear, I am reviewing this .spec file and SRPM: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01722749-rubygem-sassc-rails/rubygem-sassc-rails.spec https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01722749-rubygem-sassc-rails/rubygem-sassc-rails-2.1.2-1.3.fc34.src.rpm * Requiring sass-rails - I wonder is the `BuildRequires: rubygem(sass-rails)` really needed? If the answer is yes, then you should do something about this: ~~~ Problem: conflicting requests - nothing provides (rubygem(railties) >= 4.0.0 with rubygem(railties) < 6) needed by rubygem-sass-rails-5.0.7-7.fc33.noarch (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages) ~~~ Obviously, we still have rubygem-sass-rails, which was not build for Rails 6+. So you need bootstrap or rethink the BRs.
(In reply to Vít Ondruch from comment #9) > Just to make it clear, I am reviewing this .spec file and SRPM: Yes, that's the .spec file I was pointing to, for review. > Obviously, we still have rubygem-sass-rails, which was not build for > Rails 6+. So you need bootstrap or rethink the BRs. I've built rubygem-sass-rails in the side-tag f34-build-side-32997: https://koji.fedoraproject.org/koji/taskinfo?taskID=56151529 Yes, you're right, It's needed: ```nothing provides (rubygem(sassc-rails) >= 2.1 with rubygem(sassc-rails) < 3 with rubygem(sassc-rails) >= 2.1.1) needed by rubygem-sass-rails-6.0.0-1.fc34.noarch``` I've added appropriate bcond_with macros. I opted to add them for this package, as only build phase is affected. Change: https://git.io/JkPvI Spec: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01785216-rubygem-sassc-rails/rubygem-sassc-rails.spec SRPM: https://download.copr.fedorainfracloud.org/results/pvalena/rubygems/fedora-rawhide-x86_64/01785216-rubygem-sassc-rails/rubygem-sassc-rails-2.1.2-1.4.fc34~bootstrap.src.rpm Koji build (in side-tag): https://koji.fedoraproject.org/koji/taskinfo?taskID=56152403 COPR build: https://copr.fedorainfracloud.org/coprs/build/1785216 Note: For real build I'll reset the release to `1`, off course.
* Hidden files: - There is a hidden file reported: ~~~ rubygem-sassc-rails-doc.noarch: W: hidden-file-or-dir /usr/share/gems/gems/sassc-rails-2.1.2/test/dummy/app/assets/images/.keep ~~~ - Given its nature, this is probably not a big deal. > Note: For real build I'll reset the release to `1`, off course. Just FTR, rpmlint complains about "incoherent-version-in-changelog 2.1.2-1.3 ['2.1.2-1.4.fc34', '2.1.2-1.4']" but I guess you are going to fix this. Otherwise, LGTM => I APPROVE this package.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-sassc-rails
Thank you for the review!