Skip to content

Commit

Permalink
build: define NOMINMAX in common.gypi
Browse files Browse the repository at this point in the history
V8 and Node.js had defined `NOMINMAX` on Windows for a long time.  In
recent changes, V8 added `std::numeric_limits::min` usages in its
header files which caused addons without `NOMINMAX` defines failed
to compile.

Define `NOMINMAX` in common.gypi so that addons can be compiled with
the latest V8 header files.

PR-URL: #52794
Fixes: nodejs/nan#968
Refs: nodejs/gyp-next#244
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
  • Loading branch information
legendecas authored and targos committed May 8, 2024
1 parent 4dc8a38 commit e1bd53c
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 4 deletions.
4 changes: 4 additions & 0 deletions common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,10 @@
'_HAS_EXCEPTIONS=0',
'BUILDING_V8_SHARED=1',
'BUILDING_UV_SHARED=1',
# Stop <windows.h> from defining macros that conflict with
# std::min() and std::max(). We don't use <windows.h> (much)
# but we still inherit it from uv.h.
'NOMINMAX',
],
}],
[ 'OS in "linux freebsd openbsd solaris aix os400"', {
Expand Down
4 changes: 0 additions & 4 deletions node.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@
'FD_SETSIZE=1024',
# we need to use node's preferred "win32" rather than gyp's preferred "win"
'NODE_PLATFORM="win32"',
# Stop <windows.h> from defining macros that conflict with
# std::min() and std::max(). We don't use <windows.h> (much)
# but we still inherit it from uv.h.
'NOMINMAX',
'_UNICODE=1',
],
'msvs_precompiled_header': 'tools/msvs/pch/node_pch.h',
Expand Down
5 changes: 5 additions & 0 deletions test/addons/hello-world/binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
'target_name': 'binding',
'sources': [ 'binding.cc' ],
'includes': ['../common.gypi'],
},
{
'target_name': 'binding2',
'sources': [ 'binding2.cc' ],
'includes': ['../common.gypi'],
}
]
}
21 changes: 21 additions & 0 deletions test/addons/hello-world/binding2.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Include uv.h and v8.h ahead of node.h to verify that node.h doesn't need to
// be included first. Disable clang-format as it will sort the include lists.
// clang-format off
#include <uv.h>
#include <v8.h>
#include <node.h>
// clang-format on

static void Method(const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Isolate* isolate = args.GetIsolate();
args.GetReturnValue().Set(
v8::String::NewFromUtf8(isolate, "world").ToLocalChecked());
}

static void InitModule(v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context) {
NODE_SET_METHOD(exports, "hello", Method);
}

NODE_MODULE(NODE_GYP_MODULE_NAME, InitModule)
3 changes: 3 additions & 0 deletions test/addons/hello-world/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ const binding = require(bindingPath);
assert.strictEqual(binding.hello(), 'world');
console.log('binding.hello() =', binding.hello());

const binding2 = require(require.resolve(`./build/${common.buildType}/binding2`));
assert.strictEqual(binding2.hello(), 'world');

// Test multiple loading of the same module.
delete require.cache[bindingPath];
const rebinding = require(bindingPath);
Expand Down

0 comments on commit e1bd53c

Please sign in to comment.