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

windows related fix #118

Merged
merged 2 commits into from Aug 16, 2021
Merged

windows related fix #118

merged 2 commits into from Aug 16, 2021

Conversation

gengjiawen
Copy link
Member

@gengjiawen gengjiawen commented Aug 13, 2021

Partly revert 4d0f93c for windows issue. See nodejs/node-gyp#2481.

And add related github action to prevent future regression.

@cclauss @rvagg

@rvagg
Copy link
Member

rvagg commented Aug 16, 2021

I really have no idea and am not the right person to 👍 this. I assume this fixes CI? I don't know what the implications for the cp1251 change are. @cclauss: are we risking some edge we're not accounting for properly?

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

RSLGTM, but I agree with @rvagg that we should verify this via tests of have someone test it manually on the platform.

@gengjiawen
Copy link
Member Author

I assume this fixes CI? I don't know what the implications for the cp1251 change are.

It recently add by 4d0f93c.
But it break our node-gyp windows test. So I also add python 3.6 to ensure gyp changes will be tested on python 3.6 too.

@gengjiawen gengjiawen merged commit 3462d4c into nodejs:main Aug 16, 2021
@gengjiawen gengjiawen deleted the win_fix branch August 16, 2021 06:21
owl-from-hogvarts added a commit to owl-from-hogvarts/gyp-next that referenced this pull request Jul 31, 2022
proper fix for nodejs#118
Special thanks to @cclauss
owl-from-hogvarts added a commit to owl-from-hogvarts/gyp-next that referenced this pull request Sep 7, 2022
proper fix for nodejs#118
Special thanks to @cclauss
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants