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.

Sign up for GitHub

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

Jump to bottom

deps,v8: cherry-pick dc704497 #23985

Merged
merged 1 commit into from
Nov 3, 2018
Merged

deps,v8: cherry-pick dc704497 #23985

merged 1 commit into from
Nov 3, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 30, 2018 •
edited
Loading

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
Copy link
Collaborator

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1430/pipeline

@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

node/common.gypi

Line 36 in 6f4721b

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

FWIW we define NOMINMAX in

node/node.gypi

Line 61 in bf7ed80

'NOMINMAX',

@refack refack force-pushed the cherry-pick-v8-dc704497 branch from a522112 to 46ddcf1 Compare October 31, 2018 00:59
@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 •
edited
Loading

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 •
edited
Loading

PR CI: https://ci.nodejs.org/job/node-test-pull-request/18307/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1809/

@refack
deps,v8: cherry-pick dc704497
d24756b 
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 force-pushed the cherry-pick-v8-dc704497 branch from 46ddcf1 to d24756b Compare November 3, 2018 00:31
@refack refack merged commit d24756b into nodejs:master Nov 3, 2018
targos pushed a commit that referenced this pull request Nov 3, 2018
@refack @targos
deps,v8: cherry-pick dc704497
6e4f238 
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
@BridgeAR BridgeAR mentioned this pull request Nov 14, 2018
Merged
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
Closed
@beevelop beevelop mentioned this pull request Nov 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@targos targos Awaiting requested review from targos

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

@bzoz bzoz Awaiting requested review from bzoz

@joaocgreis joaocgreis Awaiting requested review from joaocgreis

@seishun seishun Awaiting requested review from seishun

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@refack @nodejs-github-bot @richardlau @bnoordhuis @cjihrig @codebytere