Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Add a platform variant for installing a musl binary #1836

Merged
merged 15 commits into from
Dec 18, 2016
Merged

Add a platform variant for installing a musl binary #1836

Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fd0238a
Add a platform variant for installing a musl binary
lox Dec 16, 2016
85193ea
Don't use glibc variant, default to linux
lox Dec 16, 2016
5773e8d
Add missing index check for indexOf
lox Dec 16, 2016
a3e3e6d
Fix linting errors
lox Dec 16, 2016
3cf4232
Drop glibc variant
lox Dec 16, 2016
83e3095
More linting fixes: == -> ===
lox Dec 16, 2016
78f4601
Handle readFileSync exceptions and 0.x Buffer quirks
lox Dec 17, 2016
b0a63a0
Only use .toString() when needed for speed
lox Dec 17, 2016
7e32a94
Fix linting errors
lox Dec 17, 2016
4647e02
fix getPlatformVariant, getHumanPlatform and getBinaryPath
Dec 17, 2016
df1803d
fix extra / argument
Dec 17, 2016
5892a85
Merge pull request #3 from gmaliar/add-musl-platform-variant
lox Dec 17, 2016
73199fa
Cleanup
xzyfer Dec 18, 2016
209b1a6
Always toString
xzyfer Dec 18, 2016
d38985a
Delay tostring
xzyfer Dec 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions lib/extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ function getHumanPlatform(platform) {
case 'darwin': return 'OS X';
case 'freebsd': return 'FreeBSD';
case 'linux': return 'Linux';
case 'linux_musl': return 'Linux/musl';
case 'win32': return 'Windows';
default: return false;
}
Expand Down Expand Up @@ -182,8 +183,14 @@ function getBinaryName() {
} else if (pkg.nodeSassConfig && pkg.nodeSassConfig.binaryName) {
binaryName = pkg.nodeSassConfig.binaryName;
} else {
var platform = process.platform;
var variant = getPlatformVariant();
if (variant !== '') {
platform += '_' + variant;
}

binaryName = [
process.platform, '-',
platform, '-',
process.arch, '-',
process.versions.modules
].join('');
Expand Down Expand Up @@ -256,7 +263,7 @@ function getBinaryPath() {
} else if (pkg.nodeSassConfig && pkg.nodeSassConfig.binaryPath) {
binaryPath = pkg.nodeSassConfig.binaryPath;
} else {
binaryPath = path.join(defaultBinaryPath, getBinaryName().replace(/_/, '/'));
binaryPath = path.join(defaultBinaryPath, getBinaryName().replace(/_(?=binding\.node)/, '/'));
}

return binaryPath;
Expand Down Expand Up @@ -359,6 +366,29 @@ function getVersionInfo(binding) {
].join(eol);
}

/**
* Gets the platform variant, currently either an empty string or 'musl' for Linux/musl platforms.
*
* @api public
*/

function getPlatformVariant() {
if (process.platform !== 'linux') {
return '';
}
var contents = '';
try {
contents = fs.readFileSync(process.execPath);
if (!contents.indexOf) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@S-YOU S-YOU Dec 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@S-YOU S-YOU Dec 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@S-YOU S-YOU Dec 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return 'musl';
}
} catch (err) { } // eslint-disable-line no-empty
return '';
}

module.exports.hasBinary = hasBinary;
module.exports.getBinaryUrl = getBinaryUrl;
module.exports.getBinaryName = getBinaryName;
Expand Down