imyller / meta-nodejs

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

Node.js: Add compatibility with base OpenEmbedded npm.bbclass #68

Closed mdavis777 closed 7 years ago

mdavis777 commented 7 years ago

This patch enables the --no-registry functionality to allow for offline builds with new nodejs versions.

Signed-off-by: Michael Davis michael.davis@essvote.com

imyller commented 7 years ago

@mdavis777 Would you like to explain reasoning for this patch a bit more?

mdavis777 commented 7 years ago

Currently meta-nodejs does not play well with the base meta-oe npm.bbclass. The base npm has a few nice features like being able to specify exact version numbers and hashes for dependencies, integration with the do_fetchall function for offline builds, and ability to build recipes generated from the devtool.

This patch basically copies the --no-registry flag from the meta-oe layer so we can share that functionality as well as maintain how it currently works.

Path 1: Inherit npm-install-global: How it works currently. It does an online npm install during the build and pulls whatever dependencies the application defines. One day you might get package 3.1 the next you might get 3.1.1. Dependencies download during the do_npm_install step.

Path2: Inherit npm: How it works on meta-oe side. You have to specify all the packages in the recipe. Normally the list is generated by devtool. You will get the exact same version and hashed dependency each build. Dependencies download during the do_fetch step.

I am also working on recipes for nodejs-contrib that are locked down dependency wise so everyone has a choice of which one they want. Either the latest and greatest or one they can guarantee pulls the same source and dependencies each time.

Some background on the built in NPM functionality. https://wiki.yoctoproject.org/wiki/TipsAndTricks/NPM

mdavis777 commented 7 years ago

Is there any other questions or concerns you have?

imyller commented 7 years ago

I'm not against landing this, but I'd like to share few thoughts on this issue:

To be honest, I'm not big fan of the way OE has decided to bypass parts of NPM packaging and dependency management. Initially it looked like they did not "get it" and implemented current method just to get "immutable" library versions. All this breaks the npm conventions and goes against the node package manager core idea. That is my opinion.

We should improve current implementation (npm caching in build host etc.), but I wouldn't want to steer people into "hardcoding" npm sourced libraries as separate packages while bypassing standard package.json process. This causes issues for compilation of native components and might skip some dynamic build scripts initiated by npm install process etc. There are lot of reasons to allow npm do its own magic.

meta-nodes-contrib gives many examples how to properly cross-compile and package full Node.js applications (npm installable) to a static, immutable, OE packages. For that we offer oe_runnpm from npm-base class to provide 100% standard npm with added capability to safely cross-compile for target platform. Functionality is intended to be analogous to oe_runmake from base class in OE core.

For that reason, I've tried with meta-nodejs and meta-nodes-contrib to promote proper usage of npm as-is while still allowing creation of installable, static and fully "OE way" packages for applications powered by Node.js runtime. The idea is that only the Node.js runtime (preferably without npm!) can be deployed to embedded system (target) and applications can be pre-compiled and pre-npm-installed on build host.

All that said, I'm not opposing landing these changes if we can truly have both implementations co-existing without conflicts. Biggest concern I have is the Node.js version specific patch we must maintain going forward. @mdavis777 Would you like to assist with maintaining this patch?

mdavis777 commented 7 years ago

I have no problem helping maintain the patch. I don't pretend to be an expert in javascript, but I have been a software engineer long enough to pick up syntax and understand a compiler error. I can say it has lasted from the 0.% to the 7.% series with only small tweaks, so it's not a heavily edited area of code.

It might help if I give a bit of a background on why I made this patch. I come from an industry that has very strict requirements on offline builds and package signing. I know that isn't a normal use case and contrary to how Nodejs/npm is normally run. I am adding the patch for the offline build compatibility(do_fetchall) more than anything, and I am willing to assist in any development / testing effort to implement that under a different path. That being said I think that having a guarantee of sameness between builds is important for anyone using it in a commercial embedded product. It is nice to know you have the same building blocks when all the sudden the product that you haven't touched in 5 years needs a small bug fix or tweak

imyller commented 7 years ago

You rationale for this sounds reasonable.

I too support commercial embedded product for industrial environment - powered by Node.js runtime. However, I've chosen the path of using npm shrinkwrapping in release branches and outside the scope of OE packaging process to manage npm dependy lock-down. I know that it does not guarantee that underlying public repository does not change (or go away!), but for that we can have custom npm repositories if that level of security required.

For me, sacrificing standard npm install process is not preferable, but I'm willing to support both approaches in meta-nodejs if that is possible without conflicts.

mdavis777 commented 7 years ago

I did notice what may be an issue. I need to do some further testing on it tomorrow.
Don't pull it just yet.

KurtSchluss commented 7 years ago

Same here. My requirement is to have every single build completely reproduceable, even after years. So I really need a way to keep every single download, preferably fetched to $downloaddir by Bitbake's do_fetch. Even, there is not always a connection to the npm servers available, currently https://github.com/imyller/meta-nodejs/pull/64 is helping me to keep at least single downloads (Otherwise I have to download everything at each clean build.) And last but not least, it would be really great to have Bitbake's caching mechanisms working for nodejs, too.

mdavis777 commented 7 years ago

False Alarm was just a messed up offline grunt recipe. There are a few small annoyances with where files end up on the sdk, but that is a meta-oe issue. Probably something that can be worked around in the recipes that use the inherit npm. I hope to have some examples up for the base ones like grunt, bower, and typings up some time next week. It is good to pull.

imyller commented 7 years ago

Wow 14 commits - some overlapping current master? @mdavis777 would you like make sure that your master is rebased to meta-nodejs master

mdavis777 commented 7 years ago

Lol how did you catch it in the 3 minutes I was in the middle of doing that.

mdavis777 commented 7 years ago

Was adding in a change to 6.10.2 then saw you already did it and I was behind.

mdavis777 commented 7 years ago

All rebased and synced back up.

imyller commented 7 years ago

One minor thing: can you please increment PR version for .inc files (the ${INC_PR}.x) (with x+1) in context of this same PR

imyller commented 7 years ago

Merged. Thank you all for your contribution 👍

mdavis77 commented 7 years ago

FYI, the contributor to this was mdavis777 (Michael Davis)…not mdavis77. Believe it or not, that is a different person.

-Mark Davis

From: Ilkka Myller [mailto:notifications@github.com] Sent: Thursday, April 27, 2017 4:47 PM To: imyller/meta-nodejs meta-nodejs@noreply.github.com Cc: Mark Davis mark.davis@websitepipeline.com; Mention mention@noreply.github.com Subject: Re: [imyller/meta-nodejs] Node.js: Add compatibility with base OpenEmbedded npm.bbclass (#68)

Wow 14 commits - some overlapping current master? @mdavis77https://github.com/mdavis77 would you like make sure that your master is rebased to meta-nodejs master

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/imyller/meta-nodejs/pull/68#issuecomment-297834748, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ATKzluERCdWlsVr_Jo8bpXWX6Zcohythks5r0P7cgaJpZM4NBtCs.

imyller commented 7 years ago

@mdavis77 Sorry for bothering you. My typo, my fault 🤕

mdavis777 commented 7 years ago

There is a near infinite supply of us MDavis's running around.

imyller commented 7 years ago

Merged also to krogoth and morty branches.