Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1883732
Summary: | Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Pavel Valena <pvalena> |
Component: | Package Review | Assignee: | Vít Ondruch <vondruch> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | package-review, vondruch |
Target Milestone: | --- | Flags: | vondruch:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | rubygem-sassc-rails-2.1.2-1.fc34 | Doc Type: | If docs needed, set a value |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2020-11-27 04:34:57 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 1871622 |
Description
Pavel Valena
2020-09-30 04:29:38 UTC
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). Ping? 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! |