Add sass support via sass-embedded #1911

ntkme · 2023-03-15T18:29:58Z

This PR adds support for sass back via sass-embedded (backed by currently maintained official dart-sass-embedded).

It requires tilt>=2.0.11, which is the first version that introduced support for sass-embedded.

ntkme · 2023-03-15T21:25:09Z

Not sure what was causing the glitch during bundle install in the CI test, I triggered the same CI test in my fork and everything was fine: https://github.com/ntkme/sinatra/actions/runs/4430689681/jobs/7772740876

dentarg · 2023-03-15T21:38:09Z

Saw that, super strange (but interesting?). I can trigger re-run here.

EDIT: Link to the failed job https://github.com/sinatra/sinatra/actions/runs/4429712174/jobs/7770993289

ntkme · 2023-03-15T21:47:18Z

Here is my guess:

In a good run:

Installing sass-embedded 1.59.3 with native extensions
...
Installing psych 5.1.0 with native extensions

In a failed run:

Using psych 5.1.0
...
Installing sass-embedded 1.59.3 with native extensions

I suspect this different order is causing some trouble. - rubygems 3.2.33 / bundler 2.2.33 bundled in ruby 3.0.5 is supposed to be paired with psych 3.3.2, I guess having 5.1.0 installed and present is the root cause of the issue.

Side note: In setup-ruby, if ruby 2.x is picked, it will update rubygems/bundler to latest, but if ruby 3.x is picked, it uses whatever rubygems/bundler version comes with that ruby version. - So in the end, ruby 3.0 is the unlucky one that ended up with the most outdated rubygems/bundler...

ntkme · 2023-03-15T22:23:22Z

I was able to reproduce this with a branch that has psych 5.1.0 in the GitHub Action's cache and then attempt to install sass-embedded. I also found two ways to fix it:

Purge the build cache and rebuild. This does not fix the fundamental compatibility issue between old rubygems and new psych, but due to the installation order of dependencies, it gets lucky.
Update rubygems to latest using setup-ruby action.

dentarg · 2023-03-16T01:25:20Z

Thanks for investigating that @ntkme

FYI ⬆️ to @eregon and @deivid-rodriguez, maybe something good to be aware of if you aren't already

ntkme · 2023-03-16T02:02:31Z

Minimal reproduction:

The default versions of rubygems + psych work fine:

docker run --rm -i --entrypoint /bin/sh ruby:3.0.5 <<'EOF'
gem install sass-embedded --platform ruby
EOF

The updated psych breaks default rubygems:

docker run --rm -i --entrypoint /bin/sh ruby:3.0.5 <<'EOF'
gem install psych
gem install sass-embedded --platform ruby
EOF

If both are updated, then it works again:

docker run --rm -i --entrypoint /bin/sh ruby:3.0.5 <<'EOF'
gem update --system
gem install psych
gem install sass-embedded --platform ruby
EOF

ntkme · 2023-03-16T02:22:49Z

@dentarg The issue turns out to be related to this: rubygems/rubygems@189f658.

Edit: The commit above is related, but the root cause is slightly different:

The default rubygems 3.2.33 generates flag -I/usr/local/lib/ruby/3.0.0
The upgraded rubygems 3.4.8 generates flag -I/usr/local/lib/ruby/site_ruby/3.0.0

However, this difference is not because the code is different, in fact the code is the same. The problem is __dir__ becomes different on a default rubygems installation and a upgraded installation.

"-I#{File.expand_path("../..", __dir__)}"

What leads to the problem is the way ruby loads native extension. With two copies of psych installed (one system copy, one user copy).

-I/usr/local/lib/ruby/3.0.0 is broken because it loads ruby code from system copy (e.g. /usr/local/lib/ruby/3.0.0/psych.rb) but native extension from user copy (e.g. /usr/local/lib/ruby/gems/3.0.0/gems/psych-5.1.0/lib/psych.so).
-I/usr/local/lib/ruby/site_ruby/3.0.0 works because this directory has no copy of psych at all so both are loaded from user copy (e.g. /usr/local/lib/ruby/gems/3.0.0/gems/psych-5.1.0/lib/psych.rb + /usr/local/lib/ruby/gems/3.0.0/gems/psych-5.1.0/lib/psych.so)
-I/usr/local/lib/ruby/3.0.0 -I/usr/local/lib/ruby/3.0.0/x86_64_linux works because it loads both the ruby file and native ext from system copy. This is what it should be. (e.g. /usr/local/lib/ruby/3.0.0/psych.rb + /usr/local/lib/ruby/3.0.0/x86_64_linux/psych.so)

So upgrade rubygems is really just a workaround. The real fix is: rubygems/rubygems#6490

ntkme · 2023-03-23T20:28:04Z

The rubygems bug fix for bundle install issue has been released with version rubygems 3.4.9.

ntkme · 2023-03-31T16:39:59Z

As far as I know, there is one project that was using this feature and might be interested in this PR: gma/nesta@91c5725

cc @gma

dentarg · 2023-05-15T05:43:57Z

lib/sinatra/base.rb

@@ -862,7 +876,10 @@ def render(engine, data, options = {}, locals = {}, &block)
        catch(:layout_missing) { return render(layout_engine, layout, options, locals) { output } }
      end

-      output.extend(ContentTyped).content_type = content_type if content_type
+      if content_type
+        output = +output


Was this a problem before, that this string was frozen?

Maybe we should commit this change separately? With a test for it?

This is due to google-protobuf gem decodes the output as frozen string. There is a performance concern on eagerly unfreezing when not necessary. Therefore, sass-embedded returns the css output as frozen string as is, for which dependencies need to unfrozen the string as needed.

Can we only unfreeze when sass-embedded is used? Can we at least add a comment to the code with the above information?

I added a comment.

As for the +string method:

Returns self if self is not frozen.
Otherwise. returns self.dup, which is not frozen.

I did a microbenchmark on different ways to write this, and output = +output is the most performant one:

user system total real s.freeze; s = s.dup if s.frozen?; 1.553977 0.006994 1.560971 ( 1.561001) s.freeze; s = +s if s.frozen?; 0.919195 0.002047 0.921242 ( 0.921248) s.freeze; s = +s; 0.763196 0.001567 0.764763 ( 0.764776)

dentarg · 2023-05-15T05:53:05Z

Looks good to me, I think we want this. @sinatra/sinatras-helpers any comments?

jkowens · 2023-05-15T13:05:06Z

@dentarg LGTM!

ntkme · 2023-05-15T20:42:49Z

I guess I was wrong on rubygems version for ruby 3.0.6. It still need a rubygems update.

dentarg · 2023-05-15T20:44:33Z

Can you add that for the Ruby 3.0 job? We can remove that if there ever is a Ruby 3.0.x release with RubyGems that includes rubygems/rubygems#6490

Sass support was removed from Sinatra 3 due to the `ruby-sass` gem becoming unmaintained[1]. Support has since been re-added[2], backed by the modern `sass-embedded` gem, but this hasn't landed in a Sinatra release yet. Until that happens, let's shim the rendering method. [1] sinatra/sinatra#1768 [2] sinatra/sinatra#1911

Add sass support via sass-embedded

10b6e96

ntkme mentioned this pull request Mar 16, 2023

Fix $LOAD_PATH in rake and ext_conf builder rubygems/rubygems#6490

Merged

4 tasks

dentarg reviewed May 15, 2023

View reviewed changes

dentarg added the waiting for feedback label May 15, 2023

Address review feedback

f15392a

dentarg removed the waiting for feedback label May 15, 2023

Update rubygems

818b58c

dentarg merged commit 5f4dde1 into sinatra:main May 15, 2023

ntkme deleted the sass-embedded branch May 15, 2023 21:34

deviant mentioned this pull request Jul 10, 2023

Fix Sass compilation lejonet/irclogger-viewer#2

Merged

dentarg mentioned this pull request Jul 11, 2023

Release 3.1.0 #1929

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sass support via sass-embedded #1911

Add sass support via sass-embedded #1911

ntkme commented Mar 15, 2023

ntkme commented Mar 15, 2023

dentarg commented Mar 15, 2023 •

edited

Loading

ntkme commented Mar 15, 2023 •

edited

Loading

ntkme commented Mar 15, 2023

dentarg commented Mar 16, 2023

ntkme commented Mar 16, 2023

ntkme commented Mar 16, 2023 •

edited

Loading

ntkme commented Mar 23, 2023

ntkme commented Mar 31, 2023

dentarg May 15, 2023

ntkme May 15, 2023

dentarg May 15, 2023

ntkme May 15, 2023

dentarg commented May 15, 2023

jkowens commented May 15, 2023

ntkme commented May 15, 2023

dentarg commented May 15, 2023

Add sass support via sass-embedded #1911

Add sass support via sass-embedded #1911

Conversation

ntkme commented Mar 15, 2023

ntkme commented Mar 15, 2023

dentarg commented Mar 15, 2023 • edited Loading

ntkme commented Mar 15, 2023 • edited Loading

ntkme commented Mar 15, 2023

dentarg commented Mar 16, 2023

ntkme commented Mar 16, 2023

ntkme commented Mar 16, 2023 • edited Loading

ntkme commented Mar 23, 2023

ntkme commented Mar 31, 2023

dentarg May 15, 2023

Choose a reason for hiding this comment

ntkme May 15, 2023

Choose a reason for hiding this comment

dentarg May 15, 2023

Choose a reason for hiding this comment

ntkme May 15, 2023

Choose a reason for hiding this comment

dentarg commented May 15, 2023

jkowens commented May 15, 2023

ntkme commented May 15, 2023

dentarg commented May 15, 2023

dentarg commented Mar 15, 2023 •

edited

Loading

ntkme commented Mar 15, 2023 •

edited

Loading

ntkme commented Mar 16, 2023 •

edited

Loading