Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add cache for downloaded binary files #1566

Closed
wants to merge 6 commits into from
Closed

Add cache for downloaded binary files #1566

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7d77916
1554 - add Intellij Idea to .gitignore
pitgrap May 26, 2016
36ac14e
1554 - cache binary in tmp folder
pitgrap May 26, 2016
1000858
1554 - use mkdirp instead of fs-extra
pitgrap May 26, 2016
e244410
1554 - add version as subfolder for tmp binary file
pitgrap May 31, 2016
1ce1bc1
1554 - use single quotes, added subfolder to get the same folder stru…
pitgrap May 31, 2016
a07af5a
1554 - optimize install script
pitgrap May 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
@@ -1,4 +1,5 @@
*.log
.idea
.DS_Store
.sass-cache
build
Expand Down
66 changes: 58 additions & 8 deletions scripts/install.js
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ function download(url, dest, cb) {
return response.statusCode >= 200 && response.statusCode < 300;
};

var options = {
var options = {
rejectUnauthorized: false,
proxy: getProxy(),
headers: {
Expand Down Expand Up @@ -109,17 +109,67 @@ function checkAndDownloadBinary() {
return;
}

download(sass.getBinaryUrl(), sass.getBinaryPath(), function(err) {
if (err) {
console.error(err);
return;
}
var tmpPath = getTempPath(sass.getBinaryName());
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to handle a failure to create the temp directory.

if (!(sass.hasBinary(tmpPath))) { // download and install
Copy link
Contributor

Choose a reason for hiding this comment

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

@xzyfer this is the part I'm saying will always fall back to the first installed binary file in the temp folder

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said I've haven't look over the implementation yet. It's entirely likely there are issues that will need to be addressed.

Copy link
Author

@mriehema mriehema May 27, 2016 •
edited

Choose a reason for hiding this comment

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

The name of the binary file in the tmp folder based on sass.getBinaryName(). E.g. win32-x64-47_binding.node, not just binding.node. If you use another version of node-sass, the name will change too.

Copy link
Contributor

@xzyfer xzyfer May 27, 2016 •
edited

Choose a reason for hiding this comment

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

You're both correct. The binaries won't clash on different node/arch/os
combinations. However because we're no longer within node_modules changing
versions of node-sass would be an issue. When changing versions of
node-sass the installer would see a binary for their environment on disk
but it would not the correct binary for their node-sass version.

You'll need to create a folder for the node-sass version for this to work
as intended.

download(sass.getBinaryUrl(), tmpPath, function(err) {
if (err) {
console.error(err);
return;
}
console.log('Binary downloaded at', tmpPath);
copyBinary(tmpPath, sass.getBinaryPath());
Copy link
Contributor

@xzyfer xzyfer May 31, 2016 •
edited

Choose a reason for hiding this comment

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

copyBinary(tmpPath, sass.getBinaryPath());

});

console.log('Binary downloaded and installed at', sass.getBinaryPath());
});
} else { // install only
copyBinary(tmpPath, sass.getBinaryPath());
Copy link
Contributor

@xzyfer xzyfer May 31, 2016 •
edited

Choose a reason for hiding this comment

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

copyBinary(tmpPath, sass.getBinaryPath());

}
});
}

/**
* Find a temp folder for file
*
* @param {String} binaryName
* @returns {string}
* @api private
*/

function getTempPath(binaryName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't need to care about the binary name.

var candidateTmpDirs = [
Copy link
Contributor

@xzyfer xzyfer May 31, 2016 •
edited

Choose a reason for hiding this comment

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

var tmpdir = require('os-tmpdir');
// ...
var candidateTmpDirs = [
  tmpdir(), 
  process.env.npm_config_tmp, 
  sass.getBinaryPath(),
];

process.env.TMPDIR || process.env.TEMP || process.env.npm_config_tmp,
'/tmp',
Copy link
Contributor

Choose a reason for hiding this comment

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

Use https://github.com/sindresorhus/os-tmpdir

Copy link
Author

Choose a reason for hiding this comment

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

process.env.TMPDIR is the same as os-tmpdir. Line 140 is a fallback, which should never be reached. These lines are copied from the install script of phantomjs. They use it long ago without any problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's ok, I'd still prefer you use a tested package that abstracts away this concern.

path.join(process.cwd(), 'tmp')
Copy link
Contributor

Choose a reason for hiding this comment

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

The last resort fallback should the current behaviour of installing in the local vendor directory.

];

for (var i = 0; i < candidateTmpDirs.length; i++) {
var candidatePath = path.join(candidateTmpDirs[i], 'node-sass');

try {
mkdir.sync(candidatePath);
return path.join(candidatePath, binaryName);
} catch (err) {
console.error(candidatePath, 'is not writable:', err.message);
}
}
}

/**
* Copy file
*
* @param tmp
* @param dest
* @api private
*/
function copyBinary(tmp, dest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is named specifically for copying the binaries so you can make the binary name it's responsibility.

var from = path.join(tmp, sass.getBinaryName());

try {
fs.createReadStream(tmp).pipe(fs.createWriteStream(dest));

console.log('Binary installed at', sass.getBinaryPath());
} catch (err) {
console.err('Cannot install binary', err.message);
}
}

/**
* Skip if CI
*/
Expand Down