Add cache for downloaded binary files #1566

mriehema · 2016-05-26T12:55:33Z

The binary file is downloaded to a tmp folder and copied to vendor destination. If the binary already exist in the tmp folder, it will be copied only.
With this change node-sass will work offline, cache the binary and save bandwidth.
The binary configuration parameters will work the same as before.

- the binary file is downloaded to a tmp folder and copied to vendor destination. If the binary already exist in the tmp folder, it will be copied only. - With this change node-sass will work offline, cache the binary and save bandwidth. - The binary configuration parameters will work the same as before.

xzyfer · 2016-05-26T14:47:50Z

Thanks for this @mriehema. Generally speak it looks good. I'll take a closer look over the weekend.

One thing, I don't like the addition of fs-extras just for mkdir -p. I'd prefer you changed this to use mkdirp instead.

xzyfer · 2016-05-27T00:02:20Z

Thanks for that update @mriehema.

@saper @nschonni I'd love for y'all to be over this as well.

nschonni · 2016-05-27T00:36:50Z

The code looks OK, but can't this just be achieved as-is by passing the temp directory (or the expression to test for what temp directory) to the existing SASS_BINARY_PATH

xzyfer · 2016-05-27T01:20:55Z

@nschonni there was a bunch of prior discussion in #1554. This is a comprise of sorts.

nschonni · 2016-05-27T01:33:23Z

I did read it, but apparently totally missed 2.
This will cause even more problems for when people change node versions:

Install node-sass 3.7, binary gets copied to temp
Upgrade node to 3.8, and rerun install. Copies over 3.7 binary from temp again
Clear all node_modules like our usual troubleshooting, still grabs node 3.7 binary from temp

I think a general extension point for people wanting to change the download behaviour might be something to consider.
If this one is going to proceed, it'll need some other folder layer to indicate node-sass version or at least version testing like PhantomJS uses to see if it can use the version in temp path.

xzyfer · 2016-05-27T01:47:45Z

@nschonni I'm not sure I'm following. The intent of this change is just changing the download directory from vendor to something out side of node_modules i.e. /tmp. The copying the downloaded binary to vendor. The idea being if you rm -rf node_modules subsequent install will hit disk before the network.

We will not be changing the directory structure at all so the version switching issue is no different than it is today. The thing that does changes is that rm -rf node_modules is not guaranteed to fetch the latest binary, but since binaries are immutable this is a non-issue.

I haven't had time confirmed the implementation here respects those constraints but it wont ship with out it.

nschonni · 2016-05-27T01:53:17Z

scripts/install.js

-        return;
-      }
+    var tmpPath = getTempPath(sass.getBinaryName());
+    if (!(sass.hasBinary(tmpPath))) { // download and install


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

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.

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.

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.

mriehema · 2016-05-27T11:33:38Z

The idea being if you rm -rf node_modules subsequent install will hit disk before the network.

Exactly.

We will not be changing the directory structure at all so the version switching issue is no different than it is today.

Correct. I kept everything the same, but split download and install the binary into two different steps and added a "temp layer".

saper · 2016-05-30T18:30:39Z

I think it goes towards a pretty good think. Can we have the cache format the same as for node-sass-binaries repository? In short, checking it out from git somewhere should be the easiest way to fill the cache.

xzyfer · 2016-05-30T23:37:49Z

I think this approach is solid as long as the binaries are downloaded into
a node-sass version specific temp directory.

mriehema · 2016-05-31T08:21:02Z

I added the package version as subfolder to the temp path. It looks now like the file-structure of a release.

Example:
Web: https://github.com/sass/node-sass/releases/download/v3.7.0/win32-x64-47_binding.node
Temp Win7: C:\Users\mriehema\AppData\Local\Temp\node-sass\releases\download\v3.7.0\win32-x64-47_binding.node

@saper, you should be able to easily fill the cache with scrips now.
@xzyfer, any more issues, questions, problems to be resolved?

…cture as in releases

xzyfer · 2016-05-31T08:54:05Z

scripts/install.js

+function getTempPath(binaryName) {
+  var candidateTmpDirs = [
+    process.env.TMPDIR || process.env.TEMP || process.env.npm_config_tmp,
+    '/tmp',


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

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.

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

- simplify getTempPath() - use os-tmpdir - add fallback to vendor - remove unnecessary parameters

mriehema · 2016-06-07T12:24:36Z

@xzyfer, still something missing, or does this this PR look good to you now?

xzyfer · 2016-06-07T12:38:00Z

Sorry @mriehema we don't get notified of new commits so I didn't see these changes.

xzyfer · 2016-06-07T12:40:06Z

scripts/install.js

+ */
+
+function getTempPath() {
+  var candidateTmpDir = tmpdir() || process.env.npm_config_tmp;


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(), ];

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.

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.

New order of operations 1. Look for existing binary in vendor folder 2. Create target vendor folder 3. Look to see if we’ve cached a copy in the NPM cache for this version 4. Download to current version’s cached NPM download 5. Copy the NPM cached version to the regular vendor directory Lookup the configured NPM cache folder. Closes sass#1566

mriehema · 2016-09-09T08:17:34Z

Closed. A) not mergable, B) see #1714

New order of operations 1. Look for existing binary in vendor folder 2. Create target vendor folder 3. Look to see if we’ve cached a copy in the configured or NPM cache 4. Download to current version’s /CACHE/node-sass/version/binding_file 5. Copy the cached download to the regular vendor directory Closes sass#1566

pitgrap added 2 commits May 26, 2016 14:24

1554 - add Intellij Idea to .gitignore

7d77916

xzyfer added Feature - Implementation Module - CLI npm install labels May 26, 2016

xzyfer added this to the next.minor milestone May 26, 2016

xzyfer self-assigned this May 26, 2016

1554 - use mkdirp instead of fs-extra

1000858

nschonni reviewed May 27, 2016
View reviewed changes

1554 - add version as subfolder for tmp binary file

e244410

1554 - use single quotes, added subfolder to get the same folder stru…

1ce1bc1

…cture as in releases

xzyfer reviewed May 31, 2016
View reviewed changes

1554 - optimize install script

a07af5a

- simplify getTempPath() - use os-tmpdir - add fallback to vendor - remove unnecessary parameters

xzyfer reviewed Jun 7, 2016
View reviewed changes

mriehema mentioned this pull request Jul 26, 2016

npm install timeouts on "node scripts/install.js" when installing node-sass 3.8.0 eirslett/frontend-maven-plugin#447

Open

nschonni mentioned this pull request Sep 9, 2016

User configurable cache folder for binary download #1714

Merged

mriehema closed this Sep 9, 2016

nschonni modified the milestones: 3.11.0, next.minor Sep 15, 2016

mriehema deleted the issue-1554-cache-binary branch October 4, 2016 08:30

jpike88 mentioned this pull request Aug 4, 2020

Cache binary for future installs Ribeiro-Tiago/bundletool#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cache for downloaded binary files #1566

Add cache for downloaded binary files #1566

mriehema commented May 26, 2016

xzyfer commented May 26, 2016

xzyfer commented May 27, 2016

nschonni commented May 27, 2016

xzyfer commented May 27, 2016

nschonni commented May 27, 2016

xzyfer commented May 27, 2016

nschonni May 27, 2016

xzyfer May 27, 2016

mriehema May 27, 2016 •

edited

xzyfer May 27, 2016 •

edited

mriehema commented May 27, 2016

saper commented May 30, 2016

xzyfer commented May 30, 2016 •

edited

mriehema commented May 31, 2016 •

edited

xzyfer May 31, 2016

mriehema May 31, 2016

xzyfer May 31, 2016

mriehema commented Jun 7, 2016

xzyfer commented Jun 7, 2016

xzyfer Jun 7, 2016

mriehema Jun 7, 2016

xzyfer Jun 7, 2016

mriehema commented Sep 9, 2016

Add cache for downloaded binary files #1566

Add cache for downloaded binary files #1566

Conversation

mriehema commented May 26, 2016

xzyfer commented May 26, 2016

xzyfer commented May 27, 2016

nschonni commented May 27, 2016

xzyfer commented May 27, 2016

nschonni commented May 27, 2016

xzyfer commented May 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mriehema May 27, 2016 • edited

Choose a reason for hiding this comment

xzyfer May 27, 2016 • edited

Choose a reason for hiding this comment

mriehema commented May 27, 2016

saper commented May 30, 2016

xzyfer commented May 30, 2016 • edited

mriehema commented May 31, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mriehema commented Jun 7, 2016

xzyfer commented Jun 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mriehema commented Sep 9, 2016

mriehema May 27, 2016 •

edited

xzyfer May 27, 2016 •

edited

xzyfer commented May 30, 2016 •

edited

mriehema commented May 31, 2016 •

edited