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

build: define NOMINMAX in common.gypi #52794

Merged
merged 1 commit into from
May 4, 2024

Conversation

legendecas
Copy link
Member

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: nodejs/nan#968
Refs: nodejs/gyp-next#244

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.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels May 2, 2024
Copy link
Contributor

@agnat agnat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas added commit-queue Add this label to land a pull request using GitHub Actions. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v21.x labels May 4, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 4, 2024
@nodejs-github-bot nodejs-github-bot merged commit 8b2011a into nodejs:main May 4, 2024
70 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 8b2011a

@legendecas legendecas deleted the addon/minmax branch May 4, 2024 14:42
@mureinik
Copy link

mureinik commented May 5, 2024

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
Copy link
Member Author

legendecas commented May 7, 2024

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

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

Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this pull request May 8, 2024
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: nodejs#52794
Fixes: nodejs/nan#968
Refs: nodejs/gyp-next#244
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit that referenced this pull request May 8, 2024
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
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: nodejs#52794
Fixes: nodejs/nan#968
Refs: nodejs/gyp-next#244
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
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: nodejs#52794
Fixes: nodejs/nan#968
Refs: nodejs/gyp-next#244
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't compile simple program on Windows with Node.js 22
9 participants