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 all 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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -63,6 +63,7 @@
"mkdirp": "^0.5.1",
"nan": "^2.3.2",
"node-gyp": "^3.3.1",
"os-tmpdir": "^1.0.1",
"request": "^2.61.0",
"sass-graph": "^2.1.1"
},
Expand Down
62 changes: 54 additions & 8 deletions scripts/install.js
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@
var fs = require('fs'),
eol = require('os').EOL,
mkdir = require('mkdirp'),
tmpdir = require('os-tmpdir'),
path = require('path'),
sass = require('../lib/extensions'),
request = require('request'),
Expand Down Expand Up @@ -34,7 +35,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 +110,62 @@ function checkAndDownloadBinary() {
return;
}

download(sass.getBinaryUrl(), sass.getBinaryPath(), function(err) {
if (err) {
console.error(err);
return;
}
var tmpPath = getTempPath();
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);
//copy only, if not already in vendor
if (tmpPath !== sass.getBinaryPath()) {
copyBinary(tmpPath);
}
});

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

/**
* Find a temp folder for file
*
* @returns {string} temp folder including binary file name
* @api private
*/

function getTempPath() {
var candidateTmpDir = tmpdir() || process.env.npm_config_tmp;
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't correct anymore. It's not either or, it's an array of candidates. What you used to have was correct.

var candidateTmpDirs = [
  tmpdir(), 
  process.env.npm_config_tmp, 
  sass.getBinaryPath(),
];

Copy link
Author

Choose a reason for hiding this comment

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

Please take a look at the whole function. I removed the unnecessary foreach() loop of this list. It's now either tmpdir(), or process.env.npm_config_tmp. If they fail, sass.getBinaryPath() is used as fallback.

Maybe we should (temporary) disable the skip_ci part of this install script to test it on your CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw the change and IMHO it's incorrect since tmpdir() can never return a falsy value. Also, even if there is a tmpdir there is no guarantee it can be written to. The previous behaviour was correct because it asserted that either tmpdir() or process.env.npm_config_tmp existed and could be written to.

var candidatePath = path.join(candidateTmpDir, 'node-sass', 'releases', 'download', 'v' + pkg.version);

try {
mkdir.sync(candidatePath);
return path.join(candidatePath, sass.getBinaryName());
} catch (err) {
console.error(candidatePath, 'is not writable:', err.message);
return sass.getBinaryPath(); //fallback to vendor
}
}

/**
* Copy file
*
* @param from
* @api private
*/
function copyBinary(from) {
try {
fs.createReadStream(from).pipe(fs.createWriteStream(sass.getBinaryPath()));

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

/**
* Skip if CI
*/
Expand Down