-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: update module version mismatch error message #8391
Conversation
It might be kind of cool to have the file name in there, too. LGTM with or without that |
I don't believe there's currently a way of testing this in our CI but... just to make sure I didn't break anything: https://ci.nodejs.org/job/node-test-pull-request/3929/ |
"This module was compiled against a different Node.js version " | ||
"using NODE_MODULE_VERSION %d. This version of Node.js requires " | ||
"NODE_MODULE_VERSION %d. Please try re-compiling or re-installing " | ||
"the module (for instance, using `npm install` or `npm rebuild`).", |
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.
I would use the same order than the line before: npm rebuild corresponds to re-compiling and npm install to re-installing.
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.
Good catch :-)
Did you try locally with a manual bump of NODE_MODULE_VERSION ? |
before adding the |
If you want CI testing for this, an addon like this should always fail when #include <node_version.h>
#undef NODE_MODULE_VERSION
#define NODE_MODULE_VERSION 42
#include <node.h>
namespace {
inline void Initialize(v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context) {
}
}
NODE_MODULE_CONTEXT_AWARE(binding, Initialize) |
Added a quick test case! Thanks @addaleax :-) |
"The module '%s' was compiled against a different Node.js version " | ||
"using NODE_MODULE_VERSION %d. This version of Node.js requires " | ||
"NODE_MODULE_VERSION %d. Please try re-compiling or re-installing " | ||
"the module (for instance, using `npm rebuild` or `npm install`).", |
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.
Some line breaks would be nice.
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.
Line breaks added
2e042f5
to
47dfb13
Compare
CI green except for known flaky |
LGTM |
@@ -2402,8 +2402,11 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) { | |||
char errmsg[1024]; | |||
snprintf(errmsg, | |||
sizeof(errmsg), | |||
"Module version mismatch. Expected %d, got %d.", | |||
NODE_MODULE_VERSION, mp->nm_version); | |||
"The module '%s' was compiled against a different Node.js version" |
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.
I think it may be useful to split after the module name, e.g. output can currently become:
Error: The module '/Users/Jeremiah/Documents/node/test/addons/node-module-version/build/Release/binding.node' was compiled against a different Node.js version
using NODE_MODULE_VERSION 42. This version of Node.js requires
NODE_MODULE_VERSION 48. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or`npm install`).
which renders as
Error: The module '/Users/Jeremiah/Documents/node/test/addons/node-module-
version/build/Release/binding.node' was compiled against a different Node.js version
using NODE_MODULE_VERSION 42. This version of Node.js requires
NODE_MODULE_VERSION 48. Please try re-compiling or re-installing
the module (for instance, using `npm rebuild` or`npm install`).
hm, ping @jasnell? Jeremiah’s suggestion sounds good to me. |
Yep, agreed. Just haven't been able to get back to it yet. Will try to update this later today. |
47dfb13
to
b3a40fd
Compare
Updated, PTAL |
const assert = require('assert'); | ||
|
||
assert.throws(() => require('./build/Release/binding'), | ||
/was compiled against a different Node.js version/); |
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.
Could you add the module versions to the error message check?
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.
Done
b3a40fd
to
a1abcd4
Compare
@cjihrig ... PTAL |
Landed in a6e1be4. |
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
src
Description of change
Fixes: #8379