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 - Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
Summary: Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
Keywords:
Status: CLOSED RAWHIDE
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: 1871622
TreeView+ depends on / blocked
 
Reported: 2020-09-30 04:29 UTC by Pavel Valena
Modified: 2020-11-27 04:34 UTC (History)
2 users (show)

Fixed In Version: rubygem-sassc-rails-2.1.2-1.fc34
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-11-27 04:34:57 UTC
Type: ---
Embargoed:
vondruch: fedora-review+


Attachments (Terms of Use)

Comment 1 Pavel Valena 2020-09-30 04:35:17 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.

Comment 2 Vít Ondruch 2020-10-12 16:09:56 UTC
I'm taking this for a review.

Comment 3 Vít Ondruch 2020-10-12 16:41:58 UTC
* 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).

Comment 4 Vít Ondruch 2020-10-12 16:46:33 UTC
* 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.

Comment 5 Vít Ondruch 2020-10-26 14:46:00 UTC
Ping?

Comment 6 Pavel Valena 2020-10-27 12:49:02 UTC
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).

Comment 7 Pavel Valena 2020-11-12 23:53:12 UTC
Ping?

Comment 8 Vít Ondruch 2020-11-13 09:20:44 UTC
Sorry, this is still on my TODO list

Comment 9 Vít Ondruch 2020-11-19 11:10:53 UTC
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.

Comment 10 Pavel Valena 2020-11-23 22:15:16 UTC
(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.

Comment 11 Vít Ondruch 2020-11-24 16:22:48 UTC
* 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.

Comment 12 Gwyn Ciesla 2020-11-27 03:42:23 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-sassc-rails

Comment 13 Pavel Valena 2020-11-27 04:34:57 UTC
Thank you for the review!


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