-
Notifications
You must be signed in to change notification settings - Fork 340
Conversation
Thanks @digitalinfinity for finishing this change up! |
LGTM- @nodejs/platform-windows does the change in msvs.py look ok? Should we move that to upstream? It's not a problem in upstream generally since I don't believe regular Node.js supports compiling for Windows on ARM, but I'm curious where the right place to land that change is. |
I'm -0.5 for patching |
https://github.com/nodejs/node-chakracore/blob/xplat/configure#L765 |
if "%target_arch%"=="arm" ( | ||
if "%PROCESSOR_ARCHITECTURE%" NEQ "ARM" ( | ||
echo Skipping building ARM with Intl on a non-ARM device | ||
set configure_flags=%configure_flags% --without-intl |
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.
[FYI]
--without-intl
will disable the inspector
nodejs/node#12978
Not sure what's the equivalent for chakra
, but all the #if HAVE_INSPECTOR
code section will be off
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.
# Node for Windows on ARM requires at least the Windows 10 SDK | ||
# to build. If it hasn't already been provided, set the default | ||
# to v10.0 | ||
if "msvs_windows_sdk_version" not in configuration: |
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.
Should move to configure
as per my comment.
@refack ah, so that was the magic configure incantation! I think a case can still be made for making the change in gyp in case anyone else using gyp runs into it, but I don't feel too strongly and I think for our purposes, a configure change works better since I don't want node-chakracore's gyp to diverge from upstream. @kfarnung I verified that digitalinfinity@1fdbbd4 builds fine on my Windows machine with ARM- feel free to cherry-pick into your PR. |
I'd agree but |
cb8bd7f
to
7e3babf
Compare
@refack @digitalinfinity I'm not able to confirm in CI yet, but I've incorporated your recommendations and rebased against xplat. |
@@ -787,6 +787,9 @@ def configure_arm(o): | |||
|
|||
o['variables']['arm_fpu'] = options.arm_fpu or arm_fpu | |||
|
|||
if flavor == 'win': | |||
o['msvs_windows_sdk_version'] = 'v10.0' |
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.
Should be o['target_defaults']['msvs_windows_sdk_version']
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.
configure_arm
is called from configure_node
which is called from configure_engine
(https://github.com/nodejs/node-chakracore/blob/xplat/configure#L1338). At the point configure_engine
is called, output['target_defaults']
hasn't been defined yet- it gets defined later at https://github.com/nodejs/node-chakracore/blob/xplat/configure#L1376. So it looks like the way to get it propagated from configure_arm
is to set the property directly on o here? Or did I misunderstand something?
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.
Yep you are right.
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.
There's just a wierd hack at https://github.com/nodejs/node-chakracore/blob/xplat/configure#L1355 for the ['variables']
that always confuses me
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.
Haha yeah, I got thrown off by that too- it seemed like a roundabout way to do things 😄
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.
Thanks @joaocgreis for triggering the test build: https://nodejs.org/download/test/v8.0.0-test201705267e3babf033/
Given that the arm build is unblocked, this PR is fine from my side
@refack look good to you? |
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 (% CI as always)
Thanks for your help @refack! |
Several fixes needed: 1. Don't pass in the platform to ChakraCore- it'll figure out the right SDK version 2. Disable intl on ARM if cross-compiling 3. Set the default SDK version if compiling ARM to be v10.0 since Node doesn't seem to compile for Windows on ARM with the older SDKs. PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Several fixes needed: 1. Don't pass in the platform to ChakraCore- it'll figure out the right SDK version 2. Disable intl on ARM if cross-compiling 3. Set the default SDK version if compiling ARM to be v10.0 since Node doesn't seem to compile for Windows on ARM with the older SDKs. PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Several fixes needed: 1. Don't pass in the platform to ChakraCore- it'll figure out the right SDK version 2. Disable intl on ARM if cross-compiling 3. Set the default SDK version if compiling ARM to be v10.0 since Node doesn't seem to compile for Windows on ARM with the older SDKs. PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Several fixes needed: 1. Don't pass in the platform to ChakraCore- it'll figure out the right SDK version 2. Disable intl on ARM if cross-compiling 3. Set the default SDK version if compiling ARM to be v10.0 since Node doesn't seem to compile for Windows on ARM with the older SDKs. PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Several fixes needed: 1. Don't pass in the platform to ChakraCore- it'll figure out the right SDK version 2. Disable intl on ARM if cross-compiling 3. Set the default SDK version if compiling ARM to be v10.0 since Node doesn't seem to compile for Windows on ARM with the older SDKs. PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
PR-URL: nodejs#265 Reviewed-By: Kunal Pathak <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Several fixes needed:
it'll figure out the right SDK version
be v10.0 since Node doesn't seem to compile for
Windows on ARM with the older SDKs.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
build