-
Notifications
You must be signed in to change notification settings - Fork 85
Node.js: Add compatibility with base OpenEmbedded npm.bbclass #68
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.
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
Conversation
|
@mdavis777 Would you like to explain reasoning for this patch a bit more? |
|
Currently meta-nodejs does not play well with the base meta-oe npm.bbclass. 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: Path2: 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. |
|
Is there any other questions or concerns you have? |
|
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
For that reason, I've tried with 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? |
|
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 |
|
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 |
|
I did notice what may be an issue. I need to do some further testing on it tomorrow. |
|
Same here. My requirement is to have every single build completely reproduceable, even after years. |
|
False Alarm was just a messed up offline grunt recipe. |
|
Wow 14 commits - some overlapping current master? @mdavis777 would you like make sure that your master is rebased to meta-nodejs master |
|
Lol how did you catch it in the 3 minutes I was in the middle of doing that. |
|
Was adding in a change to 6.10.2 then saw you already did it and I was behind. |
|
All rebased and synced back up. |
|
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 |
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Merged. Thank you all for your contribution 👍 |
|
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? @mdavis77<https://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 GitHub<#68 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ATKzluERCdWlsVr_Jo8bpXWX6Zcohythks5r0P7cgaJpZM4NBtCs>.
|
|
@mdavis77 Sorry for bothering you. My typo, my fault 🤕 |
|
There is a near infinite supply of us MDavis's running around. |
|
Merged also to krogoth and morty branches. |
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