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

fix: support msvs_quote_cmd in ninja generator #117

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Aug 11, 2021

This fix came from https://codereview.chromium.org/10384100/, but it did not get into upstream gyp because Chromium was deprecating the uses of msvs_quote_cmd.

However Node.js still uses msvs_quote_cmd in its gyp files and I don't see a good way to remove it, so we have to support msvs_quote_cmd in ninja generator if we want to build Node.js with ninja on Windows.

pylib/gyp/generator/ninja.py Outdated Show resolved Hide resolved
pylib/gyp/msvs_emulation.py Outdated Show resolved Hide resolved
pylib/gyp/msvs_emulation.py Outdated Show resolved Hide resolved
pylib/gyp/msvs_emulation.py Outdated Show resolved Hide resolved
pylib/gyp/msvs_emulation.py Outdated Show resolved Hide resolved
@zcbenz zcbenz force-pushed the ninja_msvs_quote_cmd branch from eaeb0a8 to 30e1456 Compare August 12, 2021 02:31
@zcbenz zcbenz force-pushed the ninja_msvs_quote_cmd branch from 30e1456 to 623c476 Compare August 12, 2021 02:48
@gengjiawen gengjiawen requested a review from cclauss August 12, 2021 03:33
@gengjiawen
Copy link
Member

@cclauss ping

@gengjiawen
Copy link
Member

@cclauss ping.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

The Python looks good to me but I am not a Windows guys.

@gengjiawen gengjiawen merged commit 46486ac into nodejs:main Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants