imyller / meta-nodejs

OpenEmbedded layer for latest Node.js releases
MIT License
79 stars 87 forks source link

npm-base: Move cache dir to persistent location #64

Closed dzudek closed 7 years ago

dzudek commented 7 years ago

Pointing NPM_CACHE_DIR_NATIVE to store cache under DL_DIR. It wouldn't be required anymore to download all the packages after removing TMPDIR before doing a clean build.

raymanfx commented 7 years ago

@imyller any news on this one? It was a pain to track down this issue on our servers.

raymanfx commented 7 years ago

(same story here: https://github.com/imyller/meta-nodejs-contrib/pull/5)

bachp commented 7 years ago

I think this makes sense. As far as I can tell the cache really only stores the downloaded tarball and the package.json.

imyller commented 7 years ago

I'm not saying no to this, but I'd like to explain earlier reasoning for placing npm cache to workdir:

It's not quite that simple. Earlier I learned by error-and-trial that npm cache is not guaranteed to be cross-platform portable and has limited portability between npm versions (especially if switching backwards).

This caused issues in environments where packages with different Node.js version dependencies exist and/or build is made for several machine architectures using same OE environment (e.g. repository builders). Sharing common npm cache for these situations caused unexpected package build failures. Especially for npm packages containing native libraries. npm cache simply is not designed to be quite that portable.

Safe solution, and the "OE way", is to make sure that each and every package build cycle/workdir gets clean starting point without leftovers from earlier builds on another architecture or build tool version. One way to safely do this with npm is to place npm's cache to workdir. Downside is that cache lifetime is only for the package build (especially if if oe_rmwork is enabled).

To be honest, I haven't checked the situation with npm cache portability for a while, so I'm open for suggestions how to get the best of the both solutions. Maybe prefix cache dir with proper {target/build}_arch and.. maybe if required.. npm version?

Your thoughts?

imyller commented 7 years ago

I'd like to get this in too along with #68, but issues remain with supporting multiple Node.js and NPM versions. NPM does not really handle well jumping versions back and forth with common cache dir.

With large builds having packages depending on different Node.js versions this causes very hard to debug build failures.

So far only reliable solution has been placing cache in (temporary) workdir.

Shared npm cache dir would be nice, but I'd hate to cause breakage.

Suggestions are welcome.

/cc @KurtSchluss @mdavis777

KurtSchluss commented 7 years ago

How does OE support different nodejs versions within one build? - Is that supported at all? I am not familiar with the download dir structure which is created for or from npm, but it sounds to me like there has to be version and architecture dependencies introduced.

imyller commented 7 years ago

I'm thinking we could append target arch and npm version to cache dir name? If those two remain the same, it should be safe to assume full compatibility.

mdavis777 commented 7 years ago

That would be my first guess on a solution. Target Arch is easy to add, but NPM version would be more difficult. I would assume you would have to have the nodejs recipe set some new global var and npm.bbclass then use it.

imyller commented 7 years ago

Since all npm usage goes through oe_runnpm wrapper we could do version detection there with simple npm -v?

imyller commented 7 years ago

Going further, we could also add variable like NPM_SHARED_CACHE ?= "0" (or "1") allowing bypassing this automatic cache dir prefixing.

That way you could have fully common/shared npm cache (with your own risk) or safely arch+npmver automatically prefixed cache dir.

mdavis777 commented 7 years ago

Did some rough work on it. Seemed to work fine. https://github.com/mdavis777/meta-nodejs/commit/c6a034cd66017db32cc4f5b175e9a9f8a31137ae

imyller commented 7 years ago

Exactly what I had in mind. I'll test this while jumping between major Node.js versions, but I do not expect any problems.

KurtSchluss commented 7 years ago

Looks nice. Just one question: Shouldn't npm -v be called via ${NPM}?

imyller commented 7 years ago

Yes, with ${NPM}. Good catch.

Also in my testing this breaks compatibility with recipes that define their own custom cache dir by overriding NPM_CACHE_DIR. We have to check if NPM_CACHE_DIR is defined when calling oe_runnpm. If it is, use that. If not, generate version prefixed dir.

mdavis777 commented 7 years ago

1e68570079ca7b3d34e781a3fd5daa0a04e0fc3b

KurtSchluss commented 7 years ago

Looks good to me. How can we proceed here? May you create a pull request?

imyller commented 7 years ago

Replaced by PR #70