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: ---
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.