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

build: reduce one level of spawning in node_gyp #12653

Merged
merged 2 commits into from
May 16, 2017

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
args.append(os.path.join(arg_path, 'node.gyp'))
common_fn = os.path.join(arg_path, 'common.gypi')
options_fn = os.path.join(arg_path, 'config.gypi')
options_fips_fn = os.path.join(arg_path, 'config_fips.gypi')
Copy link
Member

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

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
@@ -1400,4 +1401,5 @@ gyp_args += args
if warn.warned:
warn('warnings were emitted in the configure phase')

sys.exit(subprocess.call(gyp_args))
errorlevel = run_gyp(gyp_args)
sys.exit(errorlevel)
Copy link
Member

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

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.

options_fips_fn = os.path.join(os.path.abspath(node_root), 'config_fips.gypi')
arg_path = node_root if sys.platform == 'win32' else os.path.abspath(node_root)
args.append(os.path.join(arg_path, 'node.gyp'))
common_fn = os.path.join(arg_path, 'common.gypi')
Copy link
Member

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 =?

common_fn = os.path.join(os.path.abspath(node_root), 'common.gypi')
options_fn = os.path.join(os.path.abspath(node_root), 'config.gypi')
options_fips_fn = os.path.join(os.path.abspath(node_root), 'config_fips.gypi')
arg_path = node_root if sys.platform == 'win32' else os.path.abspath(node_root)
Copy link
Member

Choose a reason for hiding this comment

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

Long line?



if __name__ == '__main__':
args = sys.argv[1:]
gyp_args = list(args)
run_gyp(gyp_args)
Copy link
Member

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

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

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)

`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>
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
`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
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
@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
3 tasks
@MylesBorins
Copy link
Contributor

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
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
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
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
`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
`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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants