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

[v18.x backport] src: use CreateEnvironment instead of inlining its code where possible #46330

Closed
wants to merge 1 commit into from
Closed

[v18.x backport] src: use CreateEnvironment instead of inlining its code where possible #46330

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Only difference between the original PR and this is ExitCode::kBootstrapFailure vs. BOOTSTRAP_ERROR.

We had a number of places in which we created an Environment instance by performing each step in CreateEnvironment manually. Instead, just call the function itself.

PR-URL: #45886
Reviewed-By: Colin Ihrig [email protected]
Reviewed-By: James M Snell [email protected]
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Joyee Cheung [email protected]

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@addaleax addaleax mentioned this pull request Jan 24, 2023
Merged
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Jan 24, 2023
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2023
@addaleax addaleax changed the title [v18.x backkport] src: use CreateEnvironment instead of inlining its code where possible [v18.x backport] src: use CreateEnvironment instead of inlining its code where possible Jan 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jan 25, 2023
Open
12 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member Author

@juanarbol Are the CI failures here specific to this PR? There’s a lot of fs.watch failures so I assume they’re related to test changes having been backported but not code changes (specifically, 17ae2ab)?

@addaleax addaleax force-pushed the backport-45886-v18.x branch from 335f8dc to 466d7e2 Compare February 9, 2023 17:42
@addaleax
Copy link
Member Author

addaleax commented Feb 9, 2023

(did a clean rebase to be able to try CI again)

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 9, 2023
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Feb 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

Failed to start CI
- Validating Jenkins credentials
✖  Jenkins credentials invalid
https://github.com/nodejs/node/actions/runs/4136947306

@MylesBorins MylesBorins added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

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

juanarbol
juanarbol approved these changes Feb 24, 2023
View reviewed changes
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

LGTM

@juanarbol juanarbol added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 24, 2023
@addaleax @juanarbol
src: use CreateEnvironment instead of inlining its code where possible
481bbb4 
We had a number of places in which we created an `Environment` instance
by performing each step in `CreateEnvironment` manually. Instead,
just call the function itself.

PR-URL: nodejs#45886
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@juanarbol juanarbol force-pushed the backport-45886-v18.x branch from 466d7e2 to 481bbb4 Compare February 24, 2023 00:40
@nodejs-github-bot
Copy link
Collaborator

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

@juanarbol
Copy link
Member

Dropped 0f29b57 to make ncu land clean

This was referenced Feb 25, 2023
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Feb 28, 2023
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

juanarbol pushed a commit that referenced this pull request Mar 1, 2023
@addaleax @juanarbol
src: use CreateEnvironment instead of inlining its code where possible
7f0b553 
We had a number of places in which we created an `Environment` instance
by performing each step in `CreateEnvironment` manually. Instead,
just call the function itself.

PR-URL: #45886
Backport-PR-URL: #46330
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@juanarbol
Copy link
Member

Landed in 7f0b553

@juanarbol juanarbol closed this Mar 1, 2023
This was referenced Mar 2, 2023
Open
Open
Open
Open
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
@addaleax @juanarbol
src: use CreateEnvironment instead of inlining its code where possible
8adaa13 
We had a number of places in which we created an `Environment` instance
by performing each step in `CreateEnvironment` manually. Instead,
just call the function itself.

PR-URL: #45886
Backport-PR-URL: #46330
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 6, 2023
Open
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanarbol juanarbol juanarbol 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++. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@addaleax @nodejs-github-bot @juanarbol @MylesBorins