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: add node_process.cc #21105

Closed
wants to merge 2 commits into from
Closed

src: add node_process.cc #21105

wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 3, 2018

Begin moving process object function definitions out of
node.cc ... continuing the process of making node.cc
smaller and easier to maintain.

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 3, 2018
@jasnell jasnell requested a review from addaleax June 3, 2018 00:35
@jasnell jasnell added the process Issues and PRs related to the process subsystem. label Jun 3, 2018
@jasnell
Copy link
Member Author

jasnell commented Jun 3, 2018

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

devsnek
devsnek approved these changes Jun 3, 2018
View reviewed changes
Copy link
Member

@devsnek devsnek 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 lgtm

refack
refack approved these changes Jun 3, 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.

👍 👍

@addaleax
Copy link
Member

addaleax commented Jun 3, 2018

LGTM, but this will need a non-trivial rebase against #20876 and quick backports against other release lines ¯\_(ツ)_/¯

@jasnell
Copy link
Member Author

jasnell commented Jun 3, 2018

@addaleax ... when are you expecting to get #20876 landed? I can hold off on landing this one until after that one goes.

@addaleax
Copy link
Member

addaleax commented Jun 3, 2018

@jasnell It’s labelled tsc-agenda, so I don’t think we can do it before the meeting, but by the end of the week should be doable. (The macOS CI failure over there is real but I’m optimistic about taking care of that.)

@jasnell
Copy link
Member Author

jasnell commented Jun 3, 2018

Awesome. 🎉

trivikr
trivikr approved these changes Jun 3, 2018
View reviewed changes
Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

RSLGTM

@addaleax
Copy link
Member

addaleax commented Jun 7, 2018

@jasnell This is ready for rebasing now :)

@jasnell
Copy link
Member Author

jasnell commented Jun 7, 2018

Awesome. Will try to rebase tomorrow

@jasnell
src: add node_process.cc
c87f004 
Begin moving `process` object function definitions out of
`node.cc` ... continuing the process of making `node.cc`
smaller and easier to maintain.
@jasnell
Copy link
Member Author

jasnell commented Jun 10, 2018

@addaleax ... rebased! PTAL

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

@jasnell
Copy link
Member Author

jasnell commented Jun 10, 2018

Some related failures in CI... trying again: https://ci.nodejs.org/job/node-test-pull-request/15377/

@jasnell
Copy link
Member Author

jasnell commented Jun 11, 2018

One flaky failure in Linux... re-running https://ci.nodejs.org/job/node-test-commit-linux/19468/

@Trott
Copy link
Member

Trott commented Jun 11, 2018

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

@Trott
Copy link
Member

Trott commented Jun 11, 2018

And again: https://ci.nodejs.org/job/node-test-pull-request/15387/

(Related: Consider approving #21251.)

jasnell added a commit that referenced this pull request Jun 14, 2018
@jasnell
src: add node_process.cc
baadc7a 
Begin moving `process` object function definitions out of
`node.cc` ... continuing the process of making `node.cc`
smaller and easier to maintain.

PR-URL: #21105
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jun 14, 2018

Landed in baadc7a

@jasnell jasnell closed this Jun 14, 2018
@jasnell
Copy link
Member Author

jasnell commented Jun 14, 2018

(note... landed this initially without metadata, caught it, then did a quick force push with the metadata... how? you ask, I forgot to node land --amend after rebasing out a squash commit)

@joyeecheung
Copy link
Member

@jasnell Have you tried adding -x "node land --amend" to the rebase command? git node should suggest that now, it will add an exec node land --amend line for each commit in the interactive rebase session.

Maybe nodejs/node-core-utils#160 could help with that as well

@targos
Copy link
Member

targos commented Jun 15, 2018

@jasnell would you be able to backport this to v10.x-staging asap? It probably has to be done from scratch instead of a cherry-pick to avoid sneaking in semver-major changes.

@addaleax
Copy link
Member

@targos Are you still planning on merging the Worker changes into v10.x in the near future? I think they might have a tiny conflict with one of the patches in the security release – but a big one with this PR

(i.e. I’d recommend doing this after the Worker PR – if there are nontrivial merge conflicts, I’m happy to help)

@targos
Copy link
Member

targos commented Jun 15, 2018

I've already merged everything from master apart from this PR, including de Worker changes

@jasnell
Copy link
Member Author

jasnell commented Jun 15, 2018

Will do a backport PR early next week.

This was referenced Jun 19, 2018
Closed
Closed
@targos
Copy link
Member

targos commented Jun 22, 2018

Ping @jasnell

@targos targos mentioned this pull request Jun 26, 2018
Closed
2 tasks
@targos
Copy link
Member

targos commented Jul 3, 2018

Ping. Does anyone want to pick this up?

@targos
Copy link
Member

targos commented Jul 10, 2018

Ping. Commits that depend on this change are accumulating.

@jasnell
Copy link
Member Author

jasnell commented Jul 10, 2018

I'll be able to get back on backporting this week.

@targos
Copy link
Member

targos commented Jul 10, 2018

Super. Thank you!

jasnell added a commit to jasnell/node that referenced this pull request Jul 13, 2018
@jasnell
src: add node_process.cc
f367b4d 
Backport of nodejs#21105
@jasnell jasnell mentioned this pull request Jul 13, 2018
Closed
2 tasks
jasnell added a commit to jasnell/node that referenced this pull request Jul 13, 2018
@jasnell
src: add node_process.cc
c819fe3 
Begin moving `process` object function definitions out of
`node.cc` ... continuing the process of making `node.cc`
smaller and easier to maintain.

PR-URL: nodejs#21105
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@jasnell jasnell mentioned this pull request Jul 13, 2018
Closed
2 tasks
targos pushed a commit that referenced this pull request Jul 14, 2018
@jasnell @targos
src: add node_process.cc
a68b7dd 
Begin moving `process` object function definitions out of
`node.cc` ... continuing the process of making `node.cc`
smaller and easier to maintain.

PR-URL: #21105
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>

Backport-PR-URL: #21799
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@targos targos mentioned this pull request Jul 17, 2018
Merged
This was referenced Jul 18, 2018
Closed
Closed
Closed
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Aug 1, 2018
@jasnell @MylesBorins
src: add node_process.cc
c127df4 
Begin moving `process` object function definitions out of
`node.cc` ... continuing the process of making `node.cc`
smaller and easier to maintain.

Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Backport-PR-URL: #21798
PR-URL: #21105
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@jasnell @rvagg
src: add node_process.cc
0321afe 
Begin moving `process` object function definitions out of
`node.cc` ... continuing the process of making `node.cc`
smaller and easier to maintain.

Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Backport-PR-URL: #21798
PR-URL: #21105
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@devsnek devsnek devsnek approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@evanlucas evanlucas evanlucas approved these changes

@refack refack refack approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. 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.