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.

Sign up for GitHub

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

Jump to bottom

src: move more to node_process.cc from node.cc #22422

Closed
wants to merge 1 commit into from
Closed

src: move more to node_process.cc from node.cc #22422

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 20, 2018

Incrementally moving more stuff for the process object over to node_process.cc

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

@nodejs-github-bot
Copy link
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/606/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 20, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 21, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16632/

@jasnell jasnell requested a review from addaleax August 21, 2018 13:36
refack
refack previously approved these changes Aug 21, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Rubber Stamp

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 21, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 21, 2018

Related failures in CI. Will revisit later today.

refack
refack reviewed Aug 21, 2018
View reviewed changes
src/node_internals.h
@@ -948,6 +952,26 @@ void StopProfilerIdleNotifier(const v8::FunctionCallbackInfo<v8::Value>& args);
void Umask(const v8::FunctionCallbackInfo<v8::Value>& args);
void Uptime(const v8::FunctionCallbackInfo<v8::Value>& args);

void EnvDeleter(v8::Local<v8::Name> property,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to have a node_process.h. IIUC these function are only used in node.cc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Eventually everything in node.cc that relates to the process object should be moved over to node_process.cc. Once that is done, we shouldn't need the declarations in node_internals.h any more and definitely won't need a node_process.h. These definitions are transitional only since SetupProcessObject is still in node.cc. I won't move that until after @addaleax's options parser stuff lands.

src/node_internals.h Outdated
@@ -171,6 +171,10 @@ struct sockaddr;

namespace node {

static Mutex process_mutex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these be extern? with the definition only in node_process.cc?

@refack refack dismissed their stale review August 21, 2018 15:55

have comments

@jasnell
Copy link
Member Author

jasnell commented Aug 21, 2018

Updated CI with (hopefully) fixes: https://ci.nodejs.org/job/node-test-pull-request/16641/

addaleax
addaleax approved these changes Aug 21, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM (once @refack’s comment from #22422 (comment) is addressed)

@addaleax addaleax added process Issues and PRs related to the process subsystem. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 21, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

New CI: https://ci.nodejs.org/job/node-test-pull-request/16663/

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 22, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

CI is good.

@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/16688/

jasnell added a commit that referenced this pull request Aug 23, 2018
@jasnell
src: move more to node_process.cc from node.cc
264c99f 
PR-URL: #22422
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Aug 23, 2018

Landed in 264c99f

@jasnell jasnell closed this Aug 23, 2018
addaleax pushed a commit that referenced this pull request Aug 28, 2018
@jasnell @addaleax
src: move more to node_process.cc from node.cc
25fe930 
PR-URL: #22422
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
@jasnell @targos
src: move more to node_process.cc from node.cc
2767eba 
PR-URL: #22422
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@jasnell @targos
src: move more to node_process.cc from node.cc
5f392a1 
PR-URL: #22422
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasnell @nodejs-github-bot @refack @addaleax @cjihrig @devsnek