Add a platform variant for installing a musl binary #1836

lox · 2016-12-16T02:26:36Z

This adds a new element platformvariant to the naming scheme for precompiled binding binaries:

{platform}_{platformvariant}-{arch}-{abiversion}_binding

This allows for statically compiled musl binaries to be made available for Alpine Linux.

The check for whether this is needed is done by detecting symbols in the node binary, as suggested in #1589 (comment). I don't love this approach, but it's a whole lot simpler than exec'ing out to ldd.

See #1589 for more details on background.

Tests:

$ docker run -v "$PWD:/code" --rm -w /code node:7-alpine node scripts/install.js 
Downloading binary from https://github.com/sass/node-sass/releases/download/v4.0.0/linux_musl-x64-51_binding.node

$ docker run -v "$PWD:/code" --rm -w /code node:7 node scripts/install.js 
Downloading binary from https://github.com/sass/node-sass/releases/download/v4.0.0/linux-x64-51_binding.node

nschonni · 2016-12-16T03:10:26Z

lib/extensions.js

+  if (fs.readFileSync(process.execPath).indexOf('libc.musl-x86_64.so.1') != -1) {
+    return 'musl';
+  }
+  return 'glibc';


Probably better to just return empty unless it's musl. Would simplify the code above

Yeah, fair.

xzyfer · 2016-12-16T03:51:13Z

lib/extensions.js

+  if (process.platform !== 'linux') {
+    return '';
+  }
+  if (fs.readFileSync(process.execPath).indexOf('libc.musl-x86_64.so.1') !== -1) {


fs.readFileSync can throw

var contents = ''; try { contents = fs.readFileSync('foo.bar'); if (contents.indexOf('libc.musl-x86_64.so.1') !== -1) { return 'musl'; } } catch (err) { } return ''

I guess node 0.x readFileSync return Buffer and its Buffer don't have .indexOf, and ci is failing on those. You might need to do like

EDIT: OUTDATED

~~fs.readFileSync(process.execPath).toString().indexOf('libc.musl-x86_64.so.1') > 0~~

also why I used > 0 instead of !== -1 is that it should never be 0, becuase it is not a valid executable, if you match it at 0.

EDIT: .toString() is slower, and unneeded on non 0.x, so longer version here

EDIT2: OUTDATED

try { var muslName = 'libc.musl-x86_64.so.1'; var contents = fs.readFileSync(process.execPath); var hasMusl = contents.indexOf ? contents.indexOf(muslName) > 0 : contents.toString().indexOf(muslName) > 0; if (hasMusl) { return 'musl'; } } catch (err) { console.log (err); } return ''; }

Thanks, both excellent points. Addressing.

S-YOU · 2016-12-17T03:38:15Z

lib/extensions.js

+  var contents = '';
+  try {
+    contents = fs.readFileSync('foo.bar').toString();
+    if (contents.indexOf('libc.musl-x86_64.so.1') !== -1) {


sorry I sent comment while you are updating this.
toString is slow on where Buffer.indexOf is available. so this would be longer version.

contents = fs.readFileSync('foo.bar'); if (!contents.indexOf) { contents = contents.toString(); }

Starts to get a bit micro-optimization-y for my tastes, but sure.

lox · 2016-12-17T05:28:45Z

This last failure has me baffled.

S-YOU · 2016-12-17T05:47:53Z

I think this test has timeout on that CI run.

https://github.com/sass/sass-spec/blob/master/spec/libsass-closed-issues/issue_657/default/input.scss

shouze · 2016-12-17T09:16:15Z

lib/extensions.js

+    if (!contents.indexOf) {
+      contents = contents.toString();
+    }
+    if (contents.indexOf('libc.musl-x86_64.so.1') !== -1) {


ping @ncopa can you confirm it's ok to cover all Alpine releases or should we look for a wider pattern?

Are you asking whether this test will work for all Alpine releases? If so, yes.

shouze · 2016-12-17T09:18:15Z

LGTM, thx @lox, asked @ncopa to validate the musl check ;)

S-YOU · 2016-12-17T10:48:26Z

lib/extensions.js

+  }
+  var contents = '';
+  try {
+    contents = fs.readFileSync('foo.bar');


Ah, its not 'foo.bar', its process.execPath

So much fail.

gmaliar · 2016-12-17T12:28:25Z

Hey guys, I've PR'ed on top of @lox because I've hit a few more issues down the road with the linux_musl variant.

@lox can merge it in https://github.com/lox/node-sass/pull/3 and it's passing CI.

https://travis-ci.org/gmaliar/node-sass/builds/184758295

fix getPlatformVariant, getHumanPlatform and getBinaryPath

lox · 2016-12-17T13:55:26Z

Thanks @gmaliar!

xzyfer · 2016-12-18T03:22:02Z

lib/extensions.js

+  var contents = '';
+  try {
+    contents = fs.readFileSync(process.execPath);
+    if (!contents.indexOf) {


Were you able to figure out how contents wasn't a string or buffer? This intent of line isn't clear.

just tried on 0.10. 0.12 and 7, they all return Buffer, indexOf is not available in 0.10 and 0.12

%docker run --rm -it mhart/alpine-node:0.12 sh # node > fs.readFileSync(process.execPath) <Buffer 7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00 03 00 3e 00 01 00 00 00 43 b3 2a 00 00 00 00 00 40 00 00 00 00 00 00 00 78 64 fa 00 00 00 00 00 00 00 ... > > fs.readFileSync(process.execPath).indexOf undefined

%docker run --rm -it mhart/alpine-node:7 sh # node > fs.readFileSync(process.execPath) <Buffer 7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00 03 00 3e 00 01 00 00 00 11 03 57 00 00 00 00 00 40 00 00 00 00 00 00 00 f8 1d 26 02 00 00 00 00 00 00 ... > > fs.readFileSync(process.execPath).indexOf [Function: indexOf]

Docs says, buf.indexOf is added in v1.5.0
https://nodejs.org/api/buffer.html#buffer_buf_indexof_value_byteoffset_encoding

and performance difference with/without .toString() on node 7.0

> console.time('find');fs.readFileSync(process.execPath).indexOf('libc.musl-x86_64.so.1') !== -1;console.timeEnd('find') find: 15.800ms > console.time('find');fs.readFileSync(process.execPath).toString().indexOf('libc.musl-x86_64.so.1') !== -1;console.timeEnd('find') find: 325.538ms

Also I think toString will use additional memory like 20M binary x UTF16 = 40M, If I am not wrong.

Of course there will be better performance by reading file by chunks, stop when found, but that's too much code to add, in my opinion.

Thanks for the clarification @S-YOU. I've gone back to lazily tostring'ing.

xzyfer · 2016-12-18T04:07:52Z

Pushed some minor cleanups. Happy 🚢 this when CI is green.

lox added 4 commits December 16, 2016 13:08

Add a platform variant for installing a musl binary

fd0238a

Don't use glibc variant, default to linux

85193ea

Add missing index check for indexOf

5773e8d

Fix linting errors

a3e3e6d

nschonni reviewed Dec 16, 2016

View reviewed changes

lox added 2 commits December 16, 2016 14:17

Drop glibc variant

3cf4232

More linting fixes: == -> ===

83e3095

xzyfer suggested changes Dec 16, 2016

View reviewed changes

Handle readFileSync exceptions and 0.x Buffer quirks

78f4601

S-YOU reviewed Dec 17, 2016

View reviewed changes

lox added 2 commits December 17, 2016 14:47

Only use .toString() when needed for speed

b0a63a0

Fix linting errors

7e32a94

shouze reviewed Dec 17, 2016

View reviewed changes

S-YOU reviewed Dec 17, 2016

View reviewed changes

fix getPlatformVariant, getHumanPlatform and getBinaryPath

4647e02

Guy Maliar and others added 2 commits December 17, 2016 15:08

fix extra / argument

df1803d

Merge pull request #3 from gmaliar/add-musl-platform-variant

5892a85

fix getPlatformVariant, getHumanPlatform and getBinaryPath

xzyfer reviewed Dec 18, 2016

View reviewed changes

xzyfer added 2 commits December 18, 2016 14:53

Cleanup

73199fa

Always toString

209b1a6

xzyfer approved these changes Dec 18, 2016

View reviewed changes

Delay tostring

d38985a

xzyfer merged commit 5521bb6 into sass:master Dec 18, 2016

renovate-tests mentioned this pull request Jul 7, 2017

Update dependency node-sass to v4 renovate-tests/discover-package-files#8

Open

xzyfer mentioned this pull request Apr 19, 2018

{node_abi}-{platform}-{arch}.tar.gz filename format doesn't work with SunOS/Solaris/SmartOS/OmniOS mapbox/node-pre-gyp#251

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a platform variant for installing a musl binary #1836

Add a platform variant for installing a musl binary #1836

lox commented Dec 16, 2016

nschonni Dec 16, 2016

lox Dec 16, 2016

xzyfer Dec 16, 2016

S-YOU Dec 17, 2016 •

edited

Loading

lox Dec 17, 2016

S-YOU Dec 17, 2016 •

edited

Loading

lox Dec 17, 2016

lox commented Dec 17, 2016

S-YOU commented Dec 17, 2016 •

edited

Loading

shouze Dec 17, 2016

lox Dec 17, 2016

shouze commented Dec 17, 2016

S-YOU Dec 17, 2016 •

edited

Loading

lox Dec 17, 2016

gmaliar commented Dec 17, 2016

lox commented Dec 17, 2016

xzyfer Dec 18, 2016

S-YOU Dec 18, 2016 •

edited

Loading

S-YOU Dec 18, 2016 •

edited

Loading

xzyfer Dec 18, 2016

xzyfer commented Dec 18, 2016

Add a platform variant for installing a musl binary #1836

Add a platform variant for installing a musl binary #1836

Conversation

lox commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S-YOU Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S-YOU Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lox commented Dec 17, 2016

S-YOU commented Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shouze commented Dec 17, 2016

S-YOU Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaliar commented Dec 17, 2016

lox commented Dec 17, 2016

Choose a reason for hiding this comment

S-YOU Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

S-YOU Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzyfer commented Dec 18, 2016

S-YOU Dec 17, 2016 •

edited

Loading

S-YOU Dec 17, 2016 •

edited

Loading

S-YOU commented Dec 17, 2016 •

edited

Loading

S-YOU Dec 17, 2016 •

edited

Loading

S-YOU Dec 18, 2016 •

edited

Loading

S-YOU Dec 18, 2016 •

edited

Loading