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

deps,v8: cherry-pick dc704497 #23985

Merged
merged 1 commit into from
Nov 3, 2018
Merged

Conversation

refack
Copy link
Contributor

@refack refack 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 <jgruber@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  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 nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 30, 2018
@refack refack requested a review from targos October 30, 2018 23:31
@refack refack 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
@refack refack 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

node/node.gypi

Line 61 in bf7ed80

'NOMINMAX',

@refack
Copy link
Contributor Author

refack commented Oct 31, 2018

FWIW we define NOMINMAX in

node/node.gypi

Line 61 in bf7ed80

'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
Contributor Author

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 <jgruber@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#57053}

PR-URL: nodejs#23985
Refs: v8/v8@dc70449
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack refack merged commit d24756b into nodejs:master Nov 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 <jgruber@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#57053}

PR-URL: #23985
Refs: v8/v8@dc70449
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack refack deleted the cherry-pick-v8-dc704497 branch November 3, 2018 17:30
@refack refack removed their assignment Nov 6, 2018
Sign up for free to 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 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants