Skip to content

build: reduce one level of spawning in node_gyp#12653

Merged
refack merged 2 commits intonodejs:masterfrom
refack:node-gyp-opt
May 16, 2017
Merged

build: reduce one level of spawning in node_gyp#12653
refack merged 2 commits intonodejs:masterfrom
refack:node-gyp-opt

Conversation

@refack
Copy link
Contributor

@refack refack commented Apr 25, 2017

configure will now call node_gyp as a module instead of forking
makes it easier to debug

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

build

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Apr 25, 2017
@refack refack requested a review from sam-github April 25, 2017 21:37
@refack refack self-assigned this Apr 25, 2017
bnoordhuis
bnoordhuis requested changes Apr 26, 2017
View reviewed changes
tools/gyp_node.py Outdated
Copy link
Member

@bnoordhuis bnoordhuis Apr 26, 2017

Choose a reason for hiding this comment

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

Don't make unrelated changes in a single commit, split it out into two commits.

Copy link
Contributor Author

@refack refack Apr 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Split. or should it be Splat Done.

configure Outdated
Copy link
Member

@bnoordhuis bnoordhuis Apr 26, 2017

Choose a reason for hiding this comment

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

Superfluous. errorlevel is always zero because run_gyp() exits on error.

@refack
Copy link
Contributor Author

refack commented Apr 26, 2017

@bnoordhuis fixed PTAL

bnoordhuis
bnoordhuis approved these changes May 15, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.

tools/gyp_node.py Outdated
Copy link
Member

@bnoordhuis bnoordhuis May 15, 2017

Choose a reason for hiding this comment

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

While you're here, can you drop the extraneous space before the =?

tools/gyp_node.py Outdated
Copy link
Member

@bnoordhuis bnoordhuis May 15, 2017

Choose a reason for hiding this comment

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

Long line?

tools/gyp_node.py Outdated
Copy link
Member

@bnoordhuis bnoordhuis May 15, 2017

Choose a reason for hiding this comment

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

You can just run_gyp(sys.argv[1:]) here, it's already a list.

@bnoordhuis
Copy link
Member

bnoordhuis commented May 15, 2017

By the way, the second commit log should be brought up to par with the style guide.

@refack
Copy link
Contributor Author

refack commented May 15, 2017

By the way, the second commit log should be brought up to par with the style guide.

Ack

@refack
Copy link
Contributor Author

refack commented May 15, 2017 •
edited
Loading

Addressed all comments and rebased.
I'll leave this here another day...
/cc @nodejs/build
CI: https://ci.nodejs.org/job/node-test-commit/9896/
(CI fails are false negatives)

refack added 2 commits May 16, 2017 15:26
@refack
build: reduce one level of spawning in node_gyp
4aff056 
`configure` will now call `node_gyp` as a module instead of forking
makes it easier to debug

PR-URL: nodejs#12653
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@refack
build: simplify if in setting of arg_paths
8035527 
PR-URL: nodejs#12653
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@refack refack merged commit 8035527 into nodejs:master May 16, 2017
@refack
Copy link
Contributor Author

refack commented May 16, 2017

landed in 8035527

@refack
Copy link
Contributor Author

refack commented May 16, 2017

Post land double check master: https://ci.nodejs.org/job/node-test-commit/9928/

@refack refack deleted the node-gyp-opt branch May 16, 2017 19:33
@refack refack restored the node-gyp-opt branch May 16, 2017 21:49
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@refack
build: reduce one level of spawning in node_gyp
1a79c77 
`configure` will now call `node_gyp` as a module instead of forking
makes it easier to debug

PR-URL: nodejs#12653
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@refack
build: simplify if in setting of arg_paths
9d81fc9 
PR-URL: nodejs#12653
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@refack refack deleted the node-gyp-opt branch May 19, 2017 19:34
@refack refack restored the node-gyp-opt branch May 25, 2017 23:10
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
@refack refack deleted the node-gyp-opt branch June 10, 2017 21:26
@refack refack removed their assignment Jun 12, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Jun 22, 2017

should this be backported to v6.x?

@refack
Copy link
Contributor Author

refack commented Jun 22, 2017

should this be backported to v6.x?

I think so.

MylesBorins pushed a commit that referenced this pull request Jun 23, 2017
@refack @MylesBorins
build: simplify if in setting of arg_paths
923b920 
PR-URL: #12653
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@refack @MylesBorins
build: simplify if in setting of arg_paths
7bda962 
PR-URL: #12653
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
@refack @MylesBorins
build: reduce one level of spawning in node_gyp
9dde328 
`configure` will now call `node_gyp` as a module instead of forking
makes it easier to debug

PR-URL: #12653
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@refack @MylesBorins
build: reduce one level of spawning in node_gyp
efc0f64 
`configure` will now call `node_gyp` as a module instead of forking
makes it easier to debug

PR-URL: #12653
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
@snyk-bot snyk-bot mentioned this pull request Mar 18, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@sam-github sam-github Awaiting requested review from sam-github

+1 more reviewer

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@refack @bnoordhuis @MylesBorins @gibfahn @nodejs-github-bot