-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a Sass executable wrapper that forwards to the correct exe #313
Conversation
This allows users to run `npx sass` when they've just installed `sass-embedded` and directly run the Dart VM. Due to npm/cmd-shim#152, there's no way to make this work on Windows CMD/Powershell without substantially curtailing the performance on other operating systems.
cc @ntkme since this seems in your wheelhouse |
Or maybe we just take a small bite in performance and create the wrapper as node JS script to use |
Apparently Yarn doesn't support non-JS executables at all, so I guess it's really swimming upstream to try to do it this way. The performance hit is non-negligible—it looks like about 450ms of overhead on my laptop, which will definitely add up if anyone tries running this in a tight loop. But it does seem like the infrastructure is just not there for doing this the faster way. |
Filed yarnpkg/berry#6422 to track this on the Yarn end. |
Is that measuring Either way, that is really slow. For reference, the overhead of invoking ruby wrapper directly without using I haven't got a chance to measure it myself, but my guess is that even |
This is installing it via |
The current implementation of isLinuxMusl is very brute force that it reads the whole node binary into a buffer and then search for a substring which is unfortunately slow. A faster and more reliable way is to implement an ELF parser (only need bare minimum capability of parsing elf headers and program headers), extract the interpreter string, and check if For reference, this is a Ruby implementation to extract the ELF interpreter string: https://github.com/sass-contrib/sass-embedded-host-ruby/blob/main/lib/sass/elf.rb. I tested it on Linux and it only took less than 1ms on my M1 MBP to get the interpreter string. require 'bundler/inline'
gemfile do
source 'https://rubygems.org'
gem 'sass-embedded', require: false
# https://github.com/protocolbuffers/protobuf/issues/16853
gem 'google-protobuf', force_ruby_platform: true if Gem::Platform.local.to_s == 'aarch64-linux-musl'
end
module Sass
starting = Process.clock_gettime(Process::CLOCK_MONOTONIC)
require 'sass/elf'
ending = Process.clock_gettime(Process::CLOCK_MONOTONIC)
puts "interpreter: #{ELF::INTERPRETER}"
puts "is-musl: #{File.basename(ELF::INTERPRETER).start_with?('ld-musl-')}"
elapsed = ending - starting
puts elapsed
end Sample outputs:
|
I've filed #314 to track making this check more efficient. |
Shall we remove the I've put up a PR for that: #322. |
bin/sass
Outdated
child_process.execFileSync( | ||
compilerCommand[0], [...compilerCommand.slice(1), ...process.argv.slice(2)], { | ||
stdio: 'inherit', | ||
windowsHide: true | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made me realize there's not a great way to get this file covered by the linter/formatter as-is, so I converted it to TypeScript.
@nex3 Is there a timeline for making a new release of |
@jgerigmeyer We're planning to cut a release as soon as @jathak's work on making deprecations available to the legacy API lands. |
This allows users to run
npx sass
when they've just installedsass-embedded
and directly run the Dart VM.