-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
build: fix arm64 compilation #46231
build: fix arm64 compilation #46231
Conversation
Review requested:
|
Could you provide a bit more context? Environment MSVC version, error message, etc. We have been doing regular testing on node for Windows Arm64 and didn't run into this issue. |
Two fixed problems are mentioned in description. #45431 has similar problem on Windows |
I believe the V8 and gyp changes would instead need to be made upstream in their respective repositories. |
grouped_sources[group].append([element, {"Include": source}] + detail) | ||
element_node = [element, {"Include": source}] | ||
if element == "MARMASM": | ||
element_node.append(["PreprocessedFileName", source + ".pp"]) |
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.
This looks vaguely similar to nodejs/gyp-next#162.
(gyp-next is where this change should go but maybe you can coordinate with the author of that pull request.)
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.
This code must avoid generating files in the source tree. The generated files must go to the $(IntDir). PR #46228 solves this issue without changing GYP by adding directory.build.props
file to override PreprocessedFileName
locally.
Hey @eukarpov, thanks for this PR. It was helpful, especially when reviewing other similar PRs. The changes you proposed here should land in their respective repos (v8 and gyp-next) and then get pulled here as a dependency update. There is already a similar PR in gyp-next (nodejs/gyp-next#162) that fixes the build there. And since #46228 already fixes the VS2022 compilation for Release builds, I'd rather go with it. Thanks for getting involved and pushing this forward! |
arm64 build has been fixed in 19bcba0 |
#45431 Build failure for armv7 architecture (similar problem on Windows VS 2022 v17)
#46228