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.

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

Should we backport node_module_version to v4.x? #124

Closed
MylesBorins opened this issue Jul 18, 2016 · 10 comments
Closed

Should we backport node_module_version to v4.x? #124

MylesBorins opened this issue Jul 18, 2016 · 10 comments

Comments

@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 18, 2016

This question was raised in an issue over at https://github.com/nodejs/node

Thoughts @nodejs/lts?

/cc @bnoordhuis

@rvagg
Copy link
Member

rvagg commented Jul 19, 2016

I believe this will come across if we pull in nodejs/node#6994 which is already on the agenda.

@Fishrock123
Copy link
Contributor

Anyone able to provide a synopsis of that that does? I'm not familiar with it.

@saper
Copy link

saper commented Jul 20, 2016

@Fishrock123 This adds process.config.variables.node_module_version to config.gypi. This information is available as process.versions.modules if the node is running, but we don't know the value if we are for example cross-compiling a module for some node version other than currently running. See nodejs/node-gyp#855 and nodejs/node-gyp#855 (comment)

@saper
Copy link

saper commented Jul 20, 2016

(Proof of concept: backport of node_module_version to node v0.10: nodejs/node#7808 )

@jasnell
Copy link
Member

jasnell commented Jul 20, 2016

It is worth noting that process.config is frequently overridden by certain userland modules and depending on process.config is unwise. We've had regressions creep up recently in v4 because of the addition of FIPS mode checks in process.config that failed because a userland module decided to overwrite process.config entirely.

@bnoordhuis
Copy link
Member

The difference though is that node_module_version is also available in a binding.gyp.

@mhdawson
Copy link
Member

Does seem useful and as a backport at least for 4.x

@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

The @nodejs/lts WG discussed this today, and while we didn't see a pressing justification for backporting it, we also couldn't come up with any objections. So... SGTM

@saper
Copy link

saper commented Aug 19, 2016

v4.x version is in nodejs/node#8171

@MylesBorins
Copy link
Contributor Author

This landed 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants