feat: port "add support for MSVC cross-compilation" from node #41
Conversation
This change means that GYP can now generate two sets of projects: one exclusively for a host x64 machine and one containing a mix of x64 and Arm targets. The names of host targets are fixed up to end with _host.exe, and any actions involving them are fixed up. This allows compilation of Node on an x64 server for a Windows on Arm target. PR-URL: #32867 Reviewed-By: Christian Clauss <[email protected]> Reviewed-By: João Reis <[email protected]>
There was a problem hiding this comment.
RSLGTM. I guess we can address @cclauss's comments in a single commit after the cherry-picked commit. Moving forwards, let's please make sure to close any PRs to gyp in the node/node-gyp source code and move them all to here.
Original commit message:
tools,gyp: add support for MSVC cross-compilation
This change means that GYP can now generate two sets of projects: one
exclusively for a host x64 machine and one containing a mix of x64 and
Arm targets. The names of host targets are fixed up to end with
_host.exe, and any actions involving them are fixed up. This allows
compilation of Node on an x64 server for a Windows on Arm target.
Refs: nodejs/node#32867
Closes: nodejs#40
|
Updated. Thanks for the review |
pylib/gyp/generator/msvs.py
Outdated
| @@ -3786,10 +3812,10 @@ def _GenerateMSBuildProject(project, options, version, generator_flags): | |||
| content += _GetMSBuildLocalProperties(project.msbuild_toolset) | |||
| content += import_cpp_props_section | |||
| content += import_masm_props_section | |||
| if spec.get("msvs_enable_marmasm"): | |||
| if spec.get("msvs_enable_marmasm") or True: | |||
There was a problem hiding this comment.
@richard-townsend-arm I copied this from the downstream commit but it doesn't look right.
There was a problem hiding this comment.
Yes, I think my git commit -av habit has struck again! It should compile fine without or True, and (if not) I'll fix that.
There was a problem hiding this comment.
A quick build on my own branch indicates that everything compiles fine without this or True. Feel free to remove it here and I can push a new patch to Node.js to remove it from there.
There was a problem hiding this comment.
Thank you for verifying! No need to push a patch to Node.js, we're going to downstream more changes from here.
|
I'd like to wait for an answrer on #41 (comment) before merging. |
|
@targos would you like to push a commit removing the unnecessary condition? Once that's done we could merge this 😄 |
|
@ryzokuken done |
Original commit message:
Closes: #40