Skip to content

gyp: Revert quote_cmd workaround#1153

Closed
kunalspathak wants to merge 1 commit into
nodejs:masterfrom
kunalspathak:quote_issue
Closed

gyp: Revert quote_cmd workaround#1153
kunalspathak wants to merge 1 commit into
nodejs:masterfrom
kunalspathak:quote_issue

Conversation

@kunalspathak

Copy link
Copy Markdown
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

refack commented Mar 21, 2017

Copy link
Copy Markdown
Contributor

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

@refack

refack commented Mar 21, 2017

Copy link
Copy Markdown
Contributor

Looks critical to me! ;)

@joaocgreis joaocgreis left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

refack commented Mar 25, 2017

Copy link
Copy Markdown
Contributor

Added a regression test in #1156

@addaleax

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@refack

refack commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

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

@addaleax

Copy link
Copy Markdown
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
Copy Markdown
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

refack commented Apr 21, 2017

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

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

@refack

refack commented Apr 23, 2017

Copy link
Copy Markdown
Contributor

@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 <reis@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@gibfahn

gibfahn commented Apr 23, 2017

Copy link
Copy Markdown
Member

Landed in 8a76714

@gibfahn gibfahn closed this Apr 23, 2017
@refack

refack commented Apr 23, 2017

Copy link
Copy Markdown
Contributor

🙏

@refack

refack commented Apr 30, 2017

Copy link
Copy Markdown
Contributor

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

rvagg commented Apr 30, 2017

Copy link
Copy Markdown
Member

published @3.6.1

@refack

refack commented May 1, 2017

Copy link
Copy Markdown
Contributor

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