Skip to content

Conversation

@refack
Copy link
Contributor

@refackrefack commented Oct 30, 2018

Refs: #19630
Refs: v8/v8@dc70449

Original commit message:

 undef min,max macros on windows This blocks building with official clang-cl and Windows SDK Refs: https://github.com/nodejs/node/issues/19630 Change-Id: I41fdf934f486c660df7a9e0dd284f6eb3c294dd4 Reviewed-on: https://chromium-review.googlesource.com/c/1297479 Commit-Queue: Jakob Gruber <[email protected]> Reviewed-by: Jakob Gruber <[email protected]> Cr-Commit-Position: refs/heads/master@{#57053} 
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-botnodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 30, 2018
@refackrefack requested a review from targosOctober 30, 2018 23:31
@refackrefack added windows Issues and PRs related to the Windows platform. c++ Issues and PRs that require attention from people who are familiar with C++. labels Oct 30, 2018
@refackrefack self-assigned this Oct 30, 2018
@richardlau
Copy link
Member

Bump v8_embedder_string in

'v8_embedder_string': '-node.7',
?

FWIW we define NOMINMAX in

'NOMINMAX',

@refackrefackforce-pushed the cherry-pick-v8-dc704497 branch from a522112 to 46ddcf1CompareOctober 31, 2018 00:59
@refack
Copy link
ContributorAuthor

FWIW we define NOMINMAX in

'NOMINMAX',

That affects only the targets that node.gypi is included into (i.e. the node binary, and cctest). It is not transitive, and indeed with MSVS's cl.exe I get the warnings:

 stack_trace_win.cc d:\code\node\deps\v8\src\base\macros.h(406): warning C4003: not enough arguments for function-like macro invocation 'min' [D:\code\node\deps\v8\gypfiles\v8_libbase.vcxproj] d:\code\node\deps\v8\src\base\macros.h(408): warning C4003: not enough arguments for function-like macro invocation 'max' [D:\code\node\deps\v8\gypfiles\v8_libbase.vcxproj] d:\code\node\deps\v8\src\base\macros.h(411): warning C4003: not enough arguments for function-like macro invocation 'min' [D:\code\node\deps\v8\gypfiles\v8_libbase.vcxproj] d:\code\node\deps\v8\src\base\macros.h(414): warning C4003: not enough arguments for function-like macro invocation 'max' [D:\code\node\deps\v8\gypfiles\v8_libbase.vcxproj] logging.cc

and when trying to build with clang-cl I get a compilation error.


Bump v8_embedder_string in node/common.gypi

Done
https://github.com/nodejs/node/blob/46ddcf1cf0deae6046075ba870942e05ae2034a2/common.gypi#L36

@bnoordhuis
Copy link
Member

bnoordhuis commented Oct 31, 2018

Doesn't this work?

diff --git a/deps/v8/gypfiles/v8.gyp b/deps/v8/gypfiles/v8.gyp index f47cf075bb..b97dfb9324 100644 --- a/deps/v8/gypfiles/v8.gyp+++ b/deps/v8/gypfiles/v8.gyp@@ -2139,6 +2139,7 @@ 'libraries': [ '-lwinmm', '-lws2_32' ], }, },{+ 'defines': [ 'NOMINMAX' ], 'sources': [ '../src/base/debug/stack_trace_win.cc', '../src/base/platform/platform-win32.cc',

We can't land this PR as-is because of the 'no out-of-tree patches' policy. You'd have to open an upstream CL first.

Never mind, I somehow missed that this was a cherry-pick.

@refack
Copy link
ContributorAuthor

refack commented Nov 2, 2018

Original commit message: undef min,max macros on windows This blocks building with official clang-cl and Windows SDK Refs: nodejs#19630 Change-Id: I41fdf934f486c660df7a9e0dd284f6eb3c294dd4 Reviewed-on: https://chromium-review.googlesource.com/c/1297479 Commit-Queue: Jakob Gruber <[email protected]> Reviewed-by: Jakob Gruber <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#57053} PR-URL: nodejs#23985 Refs: v8/v8@dc70449 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@refackrefackforce-pushed the cherry-pick-v8-dc704497 branch from 46ddcf1 to d24756bCompareNovember 3, 2018 00:31
@refackrefack merged commit d24756b into nodejs:masterNov 3, 2018
targos pushed a commit that referenced this pull request Nov 3, 2018
Original commit message: undef min,max macros on windows This blocks building with official clang-cl and Windows SDK Refs: #19630 Change-Id: I41fdf934f486c660df7a9e0dd284f6eb3c294dd4 Reviewed-on: https://chromium-review.googlesource.com/c/1297479 Commit-Queue: Jakob Gruber <[email protected]> Reviewed-by: Jakob Gruber <[email protected]> Cr-Commit-Position: refs/heads/master@{#57053} PR-URL: #23985 Refs: v8/v8@dc70449 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@refackrefack deleted the cherry-pick-v8-dc704497 branch November 3, 2018 17:30
@refackrefack removed their assignment Nov 6, 2018
@BridgeARBridgeAR mentioned this pull request Nov 14, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.v8 engineIssues and PRs related to the V8 dependency.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@refack@nodejs-github-bot@richardlau@bnoordhuis@cjihrig@codebytere