FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: port "add support for MSVC cross-compilation" from node by targos · Pull Request #41 · nodejs/gyp-next · GitHub

/ gyp-next Public

feat: port "add support for MSVC cross-compilation" from node - #41

Merged
ryzokuken merged 3 commits into
nodejs:masterfrom
targos:port-32867
May 29, 2020
ConversationCommits (3)ChecksFiles changed
Merged

feat: port "add support for MSVC cross-compilation" from node#41
ryzokuken merged 3 commits into
nodejs:masterfrom
targos:port-32867

Conversation

targos commented May 23, 2020

Copy link
Copy Markdown
Member

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.

Closes: #40

targos referenced this pull request in nodejs/node May 23, 2020
tools,gyp: add support for MSVC cross-compilation
66807e9 
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 <cclauss@me.com>
Reviewed-By: João Reis <reis@janeasystems.com>
cclauss reviewed May 23, 2020
View reviewed changes
Comment thread pylib/gyp/input.py Outdated
cclauss reviewed May 23, 2020
View reviewed changes
Comment thread test_gyp.py Outdated
ryzokuken approved these changes May 25, 2020
View reviewed changes

ryzokuken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

targos added 2 commits May 28, 2020 13:36
feat: port "add support for MSVC cross-compilation" from node
b298a9f 
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
chore: fix new issues reported by linter
8f83deb

targos commented May 28, 2020

Copy link
Copy Markdown
Member Author

Updated. Thanks for the review

ryzokuken approved these changes May 28, 2020
View reviewed changes

ryzokuken left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

New patch LGTM. @cclauss can you please re-review this? I'll land this once you approve.

cclauss approved these changes May 28, 2020
View reviewed changes

cclauss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

RSLGTM

targos commented May 28, 2020
View reviewed changes
Comment thread pylib/gyp/generator/msvs.py Outdated
content += import_cpp_props_section
content += import_masm_props_section
if spec.get("msvs_enable_marmasm"):
if spec.get("msvs_enable_marmasm") or True:

targos May 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@richard-townsend-arm I copied this from the downstream commit but it doesn't look right.

richard-townsend-arm May 28, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

richard-townsend-arm May 28, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

targos May 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for verifying! No need to push a patch to Node.js, we're going to downstream more changes from here.

targos commented May 28, 2020

Copy link
Copy Markdown
Member Author

I'd like to wait for an answrer on #41 (comment) before merging.

ryzokuken commented May 28, 2020

Copy link
Copy Markdown
Contributor

@targos would you like to push a commit removing the unnecessary condition? Once that's done we could merge this 😄

targos commented May 28, 2020
View reviewed changes
Comment thread pylib/gyp/generator/msvs.py Outdated

targos commented May 28, 2020

Copy link
Copy Markdown
Member Author

@ryzokuken done

ryzokuken merged commit 62b53cb into nodejs:master May 29, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cclauss cclauss approved these changes
+2 more reviewers
richard-townsend-arm richard-townsend-arm left review comments
ryzokuken ryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

upstream cross-compilation support for Windows on Arm

4 participants

Back | FazBrowse Home | New Git URL