-
Notifications
You must be signed in to change notification settings - Fork 71
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: fix build torque on Windows #61
Conversation
6915796
to
03770e0
Compare
P.S. spec'ing at the target level gets this to build with |
Full CI: https://ci.nodejs.org/job/node-test-commit/18567/ (✔️ except for flake) |
@nodejs/gyp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a comment.
deps/v8/gypfiles/v8.gyp
Outdated
'defines!': [ | ||
'_HAS_EXCEPTIONS=0', | ||
'BUILDING_V8_SHARED=1', | ||
'BUILDING_UV_SHARED=1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these were the missing keys? (Or rather, not missing...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(To figure this out I run the GN
recipe and bisected the diff in the generated ninja files. That was not fun)
I guess BUILDING_UV_SHARED
is inconsequential.
deps/v8/gypfiles/v8.gyp
Outdated
'include_dirs': [ | ||
'../third_party/antlr4/runtime/Cpp/runtime/src', | ||
'../src/torque', | ||
], | ||
'default_configuration': 'Release', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
b2733b8
to
491eb0f
Compare
On windows the torque binary needs to be compiled and linked with exception semantics and assume V8 is embedded Fixes: nodejs#57
03770e0
to
d125781
Compare
CI is as green as it can be (test failure is a known issue). Thanks! |
Pushed to canary-base in nodejs/node@11c42e3 |
On Windows the torque binary needs to be compiled and linked
with exception semantics and assume V8 is embedded
Fixes: #57
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes