FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: reduce one level of spawning in node_gyp by refack · Pull Request #12653 · nodejs/node · GitHub

/ node Public

build: reduce one level of spawning in node_gyp - #12653

Merged
refack merged 2 commits into
nodejs:masterfrom
refack:node-gyp-opt
May 16, 2017
ConversationCommits (2)ChecksFiles changed
Merged

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

Conversation

refack commented Apr 25, 2017

Copy link
Copy Markdown
Contributor

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 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 requested a review from sam-github April 25, 2017 21:37
refack self-assigned this Apr 25, 2017
bnoordhuis requested changes Apr 26, 2017
View reviewed changes
Comment thread tools/gyp_node.py Outdated

bnoordhuis Apr 26, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

refack Apr 26, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Split. or should it be Splat Done.

Comment thread configure Outdated

bnoordhuis Apr 26, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

refack commented Apr 26, 2017

Copy link
Copy Markdown
Contributor Author

@bnoordhuis fixed PTAL

bnoordhuis approved these changes May 15, 2017
View reviewed changes

bnoordhuis 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM modulo comments.

Comment thread tools/gyp_node.py Outdated

bnoordhuis May 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread tools/gyp_node.py Outdated

bnoordhuis May 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Long line?

Comment thread tools/gyp_node.py Outdated

bnoordhuis May 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bnoordhuis commented May 15, 2017

Copy link
Copy Markdown
Member

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

refack commented May 15, 2017

Copy link
Copy Markdown
Contributor Author

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

Ack

refack commented May 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
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>
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 merged commit 8035527 into nodejs:master May 16, 2017

refack commented May 16, 2017

Copy link
Copy Markdown
Contributor Author

landed in 8035527

refack commented May 16, 2017

Copy link
Copy Markdown
Contributor Author

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

refack deleted the node-gyp-opt branch May 16, 2017 19:33
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
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
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 deleted the node-gyp-opt branch May 19, 2017 19:34
refack restored the node-gyp-opt branch May 25, 2017 23:10
jasnell mentioned this pull request May 28, 2017
Closed
refack deleted the node-gyp-opt branch June 10, 2017 21:26
refack removed their assignment Jun 12, 2017
gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks

MylesBorins commented Jun 22, 2017

Copy link
Copy Markdown
Contributor

should this be backported to v6.x?

refack commented Jun 22, 2017

Copy link
Copy Markdown
Contributor Author

should this be backported to v6.x?

I think so.

MylesBorins pushed a commit that referenced this pull request Jun 23, 2017
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
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 mentioned this pull request Jul 18, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
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
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 mentioned this pull request Aug 16, 2017
Merged
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
snyk-bot mentioned this pull request Mar 18, 2020
Open
snyk-bot mentioned this pull request Apr 1, 2020
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
sam-github Awaiting requested review from sam-github
+1 more reviewer
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

Back | FazBrowse Home | New Git URL