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

gyp: Revert quote_cmd workaround #1153

Closed
wants to merge 1 commit into from

Conversation

kunalspathak
Copy link
Member

There was a workaround added to include the cmd in quotes but with changes in gyp this was no longer needed. This was fixed in node-chakracore here but #873 was not updated with the fix. Porting the fix to revert the quote_cmd fix.

Fixes: #1151

@refack
Copy link
Contributor

refack commented Mar 21, 2017

Not your direct fault, but IMHO we should start running the GYP test suite to protect from regressions...

@refack
Copy link
Contributor

refack commented Mar 21, 2017

Looks critical to me! ;)

Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

Confirmed that building lzma-native (before addaleax/lzma-native#32) is fixed by this.

CI: https://ci.nodejs.org/job/nodegyp-test-pull-request/3/

@joaocgreis
Copy link
Member

@rvagg should we roll out 3.6.1? Let me know if I can help.

kunalspathak referenced this pull request in npm/npm Mar 24, 2017
Support for VS2017.

Chakracore support.

Credit: @refack
Credit: @kunalspathak
@iarna iarna mentioned this pull request Mar 24, 2017
@refack
Copy link
Contributor

refack commented Mar 25, 2017

Added a regression test in #1156

@addaleax
Copy link
Member

@rvagg @bnoordhuis ping? anything I can do to help this get landed?

@addaleax addaleax mentioned this pull request Apr 18, 2017
4 tasks
@bnoordhuis
Copy link
Member

@joaocgreis signed off on it. It should be good to go.

@refack
Copy link
Contributor

refack commented Apr 18, 2017

I'm making a PR rebasing on GYP eb296f6
So it'll supercede this reversion.

@addaleax
Copy link
Member

@bnoordhuis Am I reading your comment correctly in that I should be landing it? I mean, I have write access as an org admin, but I would guess node-gyp has some formalities around that?

@bnoordhuis
Copy link
Member

It's no different from other repos: run CI, add commit metadata, merge. Anyone with write access can (and is welcome to) land patches.

@refack
Copy link
Contributor

refack commented Apr 21, 2017

It's no different from other repos: run CI, add commit metadata, merge. Anyone with write access can (and is welcome to) land patches.

I assume the "secret sauce" is in actually packaging a new release?

@bnoordhuis
Copy link
Member

npm publish =) You need to be a package owner, of course.

@refack
Copy link
Contributor

refack commented Apr 23, 2017

@gibfahn if you're on a roll, really should land this, it's a nasty one.

gibfahn pushed a commit that referenced this pull request Apr 23, 2017
PR-URL: #1153
Fixes: #1151
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented Apr 23, 2017

Landed in 8a76714

@gibfahn gibfahn closed this Apr 23, 2017
@refack
Copy link
Contributor

refack commented Apr 23, 2017

🙏

@refack
Copy link
Contributor

refack commented Apr 30, 2017

Sorry to ping on a second thread, should have done it here the first time.
This fix should call for a release of 3.6.1 as 3.6.0 has a regression in "gyp custom actions" #1151
/cc @bnoordhuis @rvagg
/cc2 @addaleax

@rvagg
Copy link
Member

rvagg commented Apr 30, 2017

published @3.6.1

@refack
Copy link
Contributor

refack commented May 1, 2017

Thank you!

refack added a commit to refack/node-gyp that referenced this pull request May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird error MSB6006: "cmd.exe" exited with code 1 on some packages
7 participants