Support sass-embedded as alternative implementation #124

ntkme · 2021-09-16T20:08:17Z

This PR add support for dart based sass implementation.

I have enabled edit access for maintainers so please free feel to make changes.

New

New configuration key implementation that can be either sassc or sass-embedded. Default to sassc for compatibility.

Limitations

sass-embedded gem will not be added as a runtime dependency for now as dart-sass-embedded still beta. Meaning user who wish to use this will have to manually add sass-embedded to Gemfile.

ashmaroli · 2021-09-17T05:14:37Z

@ntkme Thanks for opening this WIP PR.
sass-embedded requires a minimum Ruby version of 2.6.0. So before further developments, please update the requirement in the gemspec and CI config files.

ashmaroli · 2021-09-17T09:41:21Z

@ntkme For future reference, you need not rebase unless there are merge conflicts, especially in a draft PR. We always squash all commits while merging a pull request.

ntkme · 2021-09-17T18:20:14Z

Appveyor with ruby 2.7 and 3.0 is having some issue that I don't quite understand.
Appveyor with ruby 2.6 is stuck installing sass-embedded. - This step is just downloading three artifacts from GitHub: protoc, embedded-protocol, dart-sass-embedded. Not sure how could that stuck for an hour. I think it is likely an environment setup issue, but I don't know how to debug this.

sass-embedded gem package has been tested on windows using GitHub actions. For example: https://github.com/ntkme/sass-embedded-host-ruby/actions/runs/1240509193

So, it should work in windows with the right setup, not sure what's wrong with Appveyor.

Shall we try debug Appveyor or maybe we can move windows tests to use GitHub actions?

ashmaroli · 2021-09-19T11:59:21Z

So, it should work in windows with the right setup, not sure what's wrong with Appveyor.
Shall we try debug Appveyor or maybe we can move windows tests to use GitHub actions?

@ntkme I've pushed a commit to migrate tests for Windows to GH Actions (just for this branch). While the installation succeeds, the test suite fails anyways since the expected output is not the same for sourcemaps for Linux and Windows for the two implementations.
Hopefully, you can fix it or is that for dart-sass to handle?

In the meantime, I'll think of another way to run the specs for two implementations without indenting the existing spec (to improve maintainability, ease running a single failing spec and to reduce the overall diff on this pull request.)

ntkme · 2021-09-19T17:44:29Z

The source map issue has to do with dart-sass converting D:/... to D/... for windows, and that breaks the native directory handling in ruby. It should to be trivial to fix and I will take a look.

ntkme · 2021-09-19T20:37:37Z

I fixed the the source map issue on windows. Tests now passes for ruby 2.7/3.0 on windows.

However, Ruby 2.6 on windows would get stuck during dependencies installation.

phisch · 2021-09-19T20:55:03Z

@ntkme that looks like a classic "rerun and succeed" CI issue...

ntkme · 2021-09-19T21:04:47Z

Attempted to run bundle install outside of setup-ruby action, and it worked. But bundle install inside setup-ruby with ruby 2.6 does not work, even with a new cache-version set. Not sure why.

ashmaroli · 2021-09-20T15:23:02Z

@ntkme I don't know what the issue is with Ruby 2.6 and Windows.
Since this was evident on AppVeyor as well, I have removed sass-embedded as a runtime dependency. Users will have to manually include the gem in their dev environment either via Gemfile or direct install.

Currently, the loading is within the :convert method. It is less than ideal but adequate for the time being.

Now that the CI has passed (I removed Windows-Ruby2.6 from the matrix), let us focus on the documentation aspect.

ntkme · 2021-09-20T15:40:33Z

@ashmaroli Sure. Let's do that for now. I opened ruby/setup-ruby#218, hope we can get some help.

ntkme · 2021-09-21T01:14:52Z

@ashmaroli Manged to get access to a windows machine and fixed ruby 2.6 on windows. Test now passes. Details is here if anyone is interested: ruby/setup-ruby#218 (comment)

ashmaroli · 2021-09-21T01:29:39Z

That is wonderful investigative work @ntkme. 🎉
Thanks for fixing things so quickly.

ashmaroli · 2021-09-21T03:01:42Z

@ntkme I tested this on an old system running on Windows 7 SP1. Latest versions of Ruby and Jekyll installs and runs fine on that system but it doesn't have a newer version of Powershell. (Windows 7 ships with just PS 2.0; PS 5+ is needed for running Expand-Archive cmdlet).

Therefore, in the interest of Jekyll users still on Windows 7, I suggest not having sass-embedded as a runtime dependency of this plugin.

ntkme · 2021-09-21T04:03:26Z

~~It looks like rubyinstaller2 provides gunzip command from msys2. I can check if gunzip is available and fallback to Expand-Archive.~~

Never mind. Forgot that gunzip does not work with zip that contains more than one files.

I looked at how sass/embedded-host-node handles this problem and it runs out they have added a dependency just to handle zip files. So, we can do the same here.

ashmaroli · 2021-09-21T05:10:17Z

@ntkme I have decided to not include sass-embedded as a runtime dependency of the upcoming release of this plugin.
Users willing to try dart-sass are welcome to make the switch willingly; so your work here will not go to waste.

Slowly, as the ecosystem stabilizes, we'll probably phase out sassc as the primary implementation and migrate to dart-sass out-of-the-box.

ntkme · 2021-09-21T05:45:28Z

@ashmaroli That's fine with me. Please feel free to push changes to do that.

ntkme · 2021-09-21T07:22:18Z

@ashmaroli By the way, excluding it from runtime dependencies means that we can have this feature released even with dart-sass-embedded being in beta, as ultimately it is end users' choice to manually install this unstable dependency. Is that what you meant by "upcoming release"?

ashmaroli · 2021-09-21T07:57:17Z

have this feature released even with dart-sass-embedded being in beta..

Yes, that is the plan.
I only mentioned "upcoming" in the lax sense. There will surely be a minor-version bump and this feature will be a nice addition.
However, there is no fixed date, so no hurry. You can contribute in your free time.

ntkme · 2021-09-21T08:02:01Z

Windows 7 now works with some VBS magic to do the unzip work: sass-contrib/sass-embedded-host-ruby@73a66b2

Manually tested with Windows 7 VM from https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/ and rubyinstaller2 (2.6.8-1)

ashmaroli · 2021-09-21T10:04:19Z

Windows 7 now works with some VBS magic

Whoa! That was a wrong move, @ntkme
Your project just lost a couple of security points.

Using gems with native extensions are always a security concern. In the current scope, the gem already downloads third-party archives from the internet without user intervention during the installation, installs a native executable (dart), (none of this is logged in the stdout) and now there is a custom non-Ruby script also bundled within the gem (irrespective of whether that script runs for 99% of user-base).

Agreed, that everything is open-source and verifiable.
Agreed, that each concern has workarounds:

Compile gem manually from source.
Download archives manually, validate them and point to them during installation.
Install PowerShell 5.1+ (if on Windows)

Nevertheless, the default behavior has a negative impact on trustworthiness.

ntkme · 2021-09-21T10:13:03Z

The bundled VBS script is very explicit that it can only be used to unzip, nothing else, and it is only used if user does not have powershell 5.0+. I don’t see any problem just because the common misconceptions of VBS being associated with malware. I think this is a fair use case since there is no other solution out of box.

Compile the dart code would require the whole dart tool chain. In addition, it will need git or unzip or otherwise bundle whole source code with the project that can introduce a whole lot more errors.

Ask user to install powershell 5.0 might be something fair, but the user experience is not as good as this.

ntkme · 2021-09-21T10:27:01Z

Regardless of using VBS or not, users have to decide what gems they trust. I mean, even if a malicious gem does not have any extension it can still do harm at runtime as soon as a user requires it.

If people are so paranoid about VBS I can definitely revert it and just call it a day saying Windows 7 is not supported, as it is already end of support. If it wasn’t pointed out here that it wouldn’t work with windows 7, I won’t even bother try supporting an end of support system.

ashmaroli · 2021-09-21T10:51:22Z

If people are so paranoid about VBS I can definitely revert it and just call it a day saying Windows 7 is not supported

It wasn't about VBS but having a custom script silently run as part of the installation. IMO, reverting the support would be best.
Also, if you cut a new release, you need not push a commit here to update the Gemfile. The version used in the action runner will get updated automatically (until you release a v0.8).

Also, I apologize for wasting your time by mentioning Windows 7.

ntkme · 2021-09-21T17:14:13Z

It wasn't about VBS but having a custom script silently run as part of the installation.

Overall the problem is that native dependencies which are not native language binding libraries does not really work with native extension build system (mkmf.rb or node-gyp), as those building systems are designed only for building native language binding libraries. sass team made the choice to to use dart compiled native binary and stdio communication instead of native binding as there is no easy way to create a dart-to-other-language binding. For example, to implement a dart-compiled-native-code-to-ruby-binding, the whole Dart VM need to be embedded into the ruby native binding library, and then a remote call interface need to be written to handle serialization of method, parameters, and return values between Dart VM methods/types and ruby c-extension methods/types. - That is possible but the development cost is too expensive.

On the other hand, use a custom script to handle installation of a native dependency that is not a native language binding library is actually pretty common nowadays. For example, in nodejs world, the "postinstall" hook can be used to run any script to handle additional setup or configuration to provide out of box user experience, and many projects use this feature to run custom script. In the official nodejs equivalent project sass/embedded-host-node, you can see that they use a custom script to download files during installation: https://github.com/sass/embedded-host-node/blob/46d600d239698fab9f811c059469d89efaaf3355/package.json#L24

You do have a good point about "silently run", but that's a design of rubygems. By default, extconf would only print Building native extensions. This could take a while..., but nothing else unless gem install is run with --verbose option. It's rubygems's design choice to "silently run" by default, I cannot really do anything about it. If user run try to install it with --verbose, I did make sure that each non-ruby command executed is also printed to the console, so that users can be aware of what has happened. There is nothing intentionally hidden.

By design ruby's mkmf.rb uses Makefile, thus any commands available on the system can be used under that. Practically running a VBS is not really different from writing the same functionality in ruby code or other language. embedded-host-node has dependencies extract-zip and tar to handle compressed file in purely nodejs code. We could do the same for ruby by adding rubyzip. However, there is a clear benefit of VBS: it's tiny and concise. From a security audit perspective, it's 113KB of ruby code (only counting files in the lib folder) vs 0.5KB of VBS to be inspected.

Finally, I understand people have concerns because the custom script "download" artifacts from internet. sass-embedded only download from sass' and protobuf's official GitHub releases. For restricted environment that may have limit access for public internet, it does offer an alternative offline installation method where user has the full control of dependencies being used:

https://github.com/ntkme/sass-embedded-host-ruby/blob/f1b0e66dfefdeeb1a3578da19a963a9da47aee1a/ext/sass/extconf.rb#L11-L24

An alternative that does not involve downloading or unzipping would be building platform native gem for each platform and embedding the uncompressed platform native dependencies directly in the gem itself. However, in the end that's less transparent (need to verify a 8.7MB binary from non-official distribution channel), therefore make it even worse than downloading from official release channel.

ntkme · 2021-09-22T19:04:52Z

@ashmaroli I did a rebase because merging conflict is getting crazy.

Also, I changed the way that test works so that now this PR does not indent the test cases, so the changes would be much easier to review. Now test script would run twice in the CI with the environment variable SASS_IMPLEMENTATION controls which implementation to be used in test. This environment variable is test only and won't affect users.

ashmaroli · 2021-09-23T05:22:24Z

I did a rebase because merging conflict is getting crazy.

No issues, it was inevitable.

I changed the way that test works so that now this PR does not indent the test cases,

Glad that fog has been cleared.

test script would run twice in the CI with the environment variable SASS_IMPLEMENTATION controls which implementation to be used in test.

This is less than ideal, but an acceptable trade-off. I'll see if we can get the best of both methods (former and current) after this PR gets merged.
Now, onto the review phase (although documentation for this feature is still scant).

Gemfile

README.md

Co-authored-by: Ashwin Maroli <[email protected]>

README.md

mattr-

ntkme · 2021-10-14T03:38:41Z

@parkr PTAL when you get a chance. Thanks!

parkr

A few questions from me.

parkr · 2021-10-14T20:39:28Z

README.md

  * **`style`**

    Sets the style of the CSS-output.
    Can be `nested`, `compact`, `compressed`, or `expanded`.
    See the [SASS_REFERENCE](https://sass-lang.com/documentation/cli/dart-sass#style)
    for details.

-    Defaults to `compact`.
+    Defaults to `compact` for `sassc`.
+    Defaults to `expanded` for `sass-embedded`.


Why would we have two different defaults for the implementation? That seems counter to what I'd expect.

See https://sass-lang.com/documentation/js-api/interfaces/LegacySharedOptions#outputStyle

The output style of the compiled CSS. There are four possible output styles:

"expanded" (the default for Dart Sass) writes each selector and declaration on its own line.

"compressed" removes as many extra characters as possible, and writes the entire stylesheet on a single line.

"nested" (the default for Node Sass, not supported by Dart Sass) indents CSS rules to match the nesting of the Sass source.

"compact" (not supported by Dart Sass) puts each CSS rule on its own single line.

The reason is that dart-sass has dropped support for compact and nested, and we don't want to change the default for sassc.

Gotcha. What happens if I specify an unsupported output style? Do I see an error? What does that error look like?

https://github.com/ntkme/sass-embedded-host-ruby/blob/8913a3f521db5c2485344701745bd2839a68a59f/lib/sass/embedded/compile_context.rb#L218

But in this PR we check for the input and fallback to the respective default value so user won’t see any error.

parkr · 2021-10-14T20:41:01Z

lib/jekyll/converters/scss.rb

@@ -113,9 +114,15 @@ def sass_dir
        jekyll_sass_configuration["sass_dir"]
      end

+      def sass_implementation
+        implementation = jekyll_sass_configuration["implementation"]
+        ALLOWED_IMPLEMENTATIONS.include?(implementation) ? implementation : "sassc"


Should we send a warn if the implementation is not allowed so the user can fix their config?

This is aligned with the style option that it fallbacks to default value without warning. Shall we add warning for both?

It makes sense to warn the user that we don't support the value they asked for. Maybe in another PR if you like.

lib/jekyll/converters/scss.rb

parkr · 2021-10-14T20:44:10Z

lib/jekyll/converters/scss.rb

+      def file_path
+        return nil if associate_page_failed?
+
+        File.join(site_source_relative_from_pwd, sass_page.path)


This might need to use Jekyll.sanitized_path, I think.

This should be safe as:

File.join is simply: base + '/' + path. It is concatenated as is without any expansion.

sass_page.path is the path to sass page e.g. css/main.scss - this is provided by jekyll. Since jekyll only process files inside current project, this should be safe to use.

What happens for symlinks?

@parkr I just tested and Jekyll.sanitized_path does not do anything different for symlinks. If file is a symlink, sass_page.path still returns the original unresolved path.

In the end when Jekyll process "pages" collection, it does not seem to care if a "page" is a symlink to a file outside of the project or not. I would say that is a problem of Jekyll pages collection, not something we should deal with here.

ashmaroli · 2021-10-15T06:37:33Z

Thank you @ntkme, for all the work put into this.
@jekyllbot: merge +minor

ntkme · 2021-10-15T06:48:00Z

Thanks @ashmaroli @mattr- @parkr for your help!

jekyll/jekyll-sass-converter#124 \o/

ntkme mentioned this pull request Sep 16, 2021

LibSass deprecated in October 2020 #116

Closed

ntkme marked this pull request as ready for review September 22, 2021 19:12

Support sass-embedded as alternative implementation

b7a6695

Update sass-embedded

7fd9cb6

ntkme added 5 commits September 28, 2021 16:32

Update sass-embedded

0c0b77d

Update sass-embedded

6e14bee

Update README.md

62e62b9

Update README.md

f991914

Update README.md

e012438

ashmaroli reviewed Sep 29, 2021

View reviewed changes

Gemfile Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

ntkme and others added 4 commits September 29, 2021 03:17

Update Gemfile

99faa86

Co-authored-by: Ashwin Maroli <[email protected]>

Update README.md

463adf7

Co-authored-by: Ashwin Maroli <[email protected]>

Update README.md

2046ba9

Co-authored-by: Ashwin Maroli <[email protected]>

Update README.md

b9ddc2f

Co-authored-by: Ashwin Maroli <[email protected]>

ashmaroli reviewed Sep 29, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

ashmaroli added 2 commits September 29, 2021 16:13

Update README.md

55a485f

Improve documentation on using sass-embedded

8f00f40

ashmaroli approved these changes Sep 29, 2021

View reviewed changes

ashmaroli requested review from mattr- and parkr September 29, 2021 11:25

mattr- approved these changes Sep 30, 2021

View reviewed changes

parkr reviewed Oct 14, 2021

View reviewed changes

Add a comment for default sass style

bb410b8

parkr approved these changes Oct 15, 2021

View reviewed changes

jekyllbot merged commit f1b9817 into jekyll:master Oct 15, 2021

jekyllbot added the enhancement label Oct 15, 2021

jekyllbot added a commit that referenced this pull request Oct 15, 2021

Update history to reflect merge of #124 [ci skip]

1b3e3aa

12beesinatrenchcoat added a commit to 12beesinatrenchcoat/12beesinatrenchcoat.github.io that referenced this pull request Oct 17, 2021

jekyll-sass-converter#124 got merged!

07961a8

jekyll/jekyll-sass-converter#124 \o/

BenSouchet mentioned this pull request Jan 17, 2022

Time for a new release #122

Closed

jekyll locked and limited conversation to collaborators Oct 15, 2022

jekyllbot added the frozen-due-to-age label Oct 15, 2022

Support sass-embedded as alternative implementation #124

Support sass-embedded as alternative implementation #124

Conversation

ntkme commented Sep 16, 2021 • edited Loading

New

Limitations

ashmaroli commented Sep 17, 2021

ashmaroli commented Sep 17, 2021

ntkme commented Sep 17, 2021 • edited Loading

ashmaroli commented Sep 19, 2021

ntkme commented Sep 19, 2021 • edited Loading

ntkme commented Sep 19, 2021

phisch commented Sep 19, 2021

ntkme commented Sep 19, 2021 • edited Loading

ashmaroli commented Sep 20, 2021

ntkme commented Sep 20, 2021

ntkme commented Sep 21, 2021 • edited Loading

ashmaroli commented Sep 21, 2021

ashmaroli commented Sep 21, 2021

ntkme commented Sep 21, 2021 • edited Loading

ashmaroli commented Sep 21, 2021 • edited Loading

ntkme commented Sep 21, 2021

ntkme commented Sep 21, 2021

ashmaroli commented Sep 21, 2021

ntkme commented Sep 21, 2021

ashmaroli commented Sep 21, 2021

ntkme commented Sep 21, 2021 • edited Loading

ntkme commented Sep 21, 2021 • edited Loading

ashmaroli commented Sep 21, 2021

ntkme commented Sep 21, 2021 • edited Loading

ntkme commented Sep 22, 2021

ashmaroli commented Sep 23, 2021

mattr- left a comment

Choose a reason for hiding this comment

ntkme commented Oct 14, 2021

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntkme Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntkme Oct 15, 2021 • edited Loading

Choose a reason for hiding this comment

ashmaroli commented Oct 15, 2021

ntkme commented Oct 15, 2021

ntkme commented Sep 16, 2021 •

edited

Loading

ntkme commented Sep 17, 2021 •

edited

Loading

ntkme commented Sep 19, 2021 •

edited

Loading

ntkme commented Sep 19, 2021 •

edited

Loading

ntkme commented Sep 21, 2021 •

edited

Loading

ntkme commented Sep 21, 2021 •

edited

Loading

ashmaroli commented Sep 21, 2021 •

edited

Loading

ntkme commented Sep 21, 2021 •

edited

Loading

ntkme commented Sep 21, 2021 •

edited

Loading

ntkme commented Sep 21, 2021 •

edited

Loading

ntkme Oct 14, 2021 •

edited

Loading

ntkme Oct 15, 2021 •

edited

Loading