[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 Pavel Valena changed: What|Removed |Added Status|ASSIGNED|CLOSED Fixed In Version||rubygem-sassc-rails-2.1.2-1 ||.fc34 Resolution|--- |RAWHIDE Last Closed||2020-11-27 04:34:57 --- Comment #13 from Pavel Valena --- Thank you for the review! -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 --- Comment #12 from Gwyn Ciesla --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/rubygem-sassc-rails -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 Vít Ondruch changed: What|Removed |Added Flags|fedora-review? |fedora-review+ --- Comment #11 from Vít Ondruch --- * 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 --- Comment #10 from Pavel Valena --- (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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 --- Comment #9 from Vít Ondruch --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 Vít Ondruch changed: What|Removed |Added Flags|needinfo?(vondruch@redhat.c | |om) | --- Comment #8 from Vít Ondruch --- Sorry, this is still on my TODO list -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 Pavel Valena changed: What|Removed |Added Flags||needinfo?(vondruch@redhat.c ||om) --- Comment #7 from Pavel Valena --- Ping? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 --- Comment #6 from Pavel Valena --- 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). -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 --- Comment #5 from Vít Ondruch --- Ping? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 --- Comment #4 from Vít Ondruch --- * 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732
--- Comment #3 from Vít Ondruch ---
* 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).
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct:
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives:
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 Vít Ondruch changed: What|Removed |Added Status|NEW |ASSIGNED CC||vondr...@redhat.com Assignee|nob...@fedoraproject.org|vondr...@redhat.com Flags||fedora-review? --- Comment #2 from Vít Ondruch --- I'm taking this for a review. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 Pavel Valena changed: What|Removed |Added Blocks||1871622 Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=1871622 [Bug 1871622] F34FailsToInstall: rubygem-sass-rails -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
[Bug 1883732] Review Request: rubygem-sassc-rails - Integrate SassC-Ruby into Rails
https://bugzilla.redhat.com/show_bug.cgi?id=1883732 --- Comment #1 from Pavel Valena --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org
