Skip to content
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

Closed

Conversation

eukarpov
Copy link

@eukarpov eukarpov commented Jan 17, 2023

#45431 Build failure for armv7 architecture (similar problem on Windows VS 2022 v17)
#46228

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. dont-land-on-v14.x gyp Issues and PRs related to the GYP tool and .gyp build files needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Jan 17, 2023
@niyas-sait
Copy link
Contributor

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.

@eukarpov
Copy link
Author

eukarpov commented Jan 17, 2023

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
Env: (github runner windows latest or self-hosted arm64 device) VS 2022 v17+

@mscdex
Copy link
Contributor

mscdex commented Jan 17, 2023

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"])
Copy link
Member

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.)

Copy link
Member

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.

@StefanStojanovic
Copy link
Contributor

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!

@eukarpov
Copy link
Author

arm64 build has been fixed in 19bcba0

@eukarpov eukarpov closed this Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. gyp Issues and PRs related to the GYP tool and .gyp build files needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants