search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
104.27k stars 28.06k forks source link

build: define `NOMINMAX` in common.gypi #52794

Closed legendecas closed 1 week ago

legendecas commented 2 weeks ago

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.

NAN includes uv.h before node.h, which makes these defines effectiveless. Nevertheless, the include order should not be significant.

Fixes: https://github.com/nodejs/nan/issues/968 Refs: https://github.com/nodejs/gyp-next/pull/244

nodejs-github-bot commented 2 weeks ago

Review requested:

nodejs-github-bot commented 2 weeks ago

CI: https://ci.nodejs.org/job/node-test-pull-request/58869/

nodejs-github-bot commented 2 weeks ago

CI: https://ci.nodejs.org/job/node-test-pull-request/58888/

nodejs-github-bot commented 1 week ago

Landed in 8b2011a818a3ad324f4b44cc811fea8e56e2f15d

mureinik commented 1 week ago

Thanks @legendecas !

Any idea when there'll be a 22.2.0 (or even 22.1.1) release that includes this so that nan users on Windows can be unblocked?

legendecas commented 1 week ago

@mureinik Addons can be unblocked immediately with their own NOMINMAX definition.

I didn't find any upcoming release plan at https://github.com/nodejs/Release/issues/1001. @nodejs/releasers would you mind chiming in on this? Thank you!