Skip to content

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

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
Closed
addaleax wants to merge 1 commit into from
Closed

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

addaleax wants to merge 1 commit into from

Conversation

@addaleax
Copy link
Member

@addaleaxaddaleax commented Jan 24, 2023

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

nodejs-github-bot commented Jan 24, 2023

Review requested:

  • @nodejs/startup

@addaleaxaddaleax mentioned this pull request Jan 24, 2023
Merged
@nodejs-github-botnodejs-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
@addaleaxaddaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 24, 2023
@addaleaxaddaleax 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 possibleJan 24, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Jan 24, 2023

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

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

nodejs-github-bot commented Jan 28, 2023

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

@addaleaxaddaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Jan 30, 2023

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

@addaleax
Copy link
MemberAuthor

addaleax commented Jan 30, 2023

@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)?

@addaleaxaddaleaxforce-pushed the backport-45886-v18.x branch from 335f8dc to 466d7e2CompareFebruary 9, 2023 17:42
@addaleax
Copy link
MemberAuthor

addaleax commented Feb 9, 2023

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

@addaleaxaddaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 9, 2023
@github-actionsgithub-actionsbot 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-actionsbot commented Feb 9, 2023

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

@MylesBorinsMylesBorins added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023
@github-actionsgithub-actionsbot 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

nodejs-github-bot commented Feb 18, 2023

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

@juanarboljuanarbol left a comment

Choose a reason for hiding this comment

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

LGTM

@juanarboljuanarbol 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]>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 24, 2023

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

@juanarbol
Copy link
Member

juanarbol commented Feb 24, 2023

Dropped 0f29b57 to make ncu land clean

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

nodejs-github-bot commented Feb 27, 2023

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

nodejs-github-bot commented Mar 1, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 1, 2023

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 1, 2023

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

juanarbol commented Mar 1, 2023

Landed in 7f0b553

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-actionsgithub-actionsbot mentioned this pull request Mar 6, 2023
Open
20 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juanarboljuanarboljuanarbol approved these changes

Assignees

No one assigned

Labels

author readyPRs 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-ciPRs that need a full CI run.processIssues and PRs related to the process subsystem.request-ci-failedAn error occurred while starting CI via request-ci label, and manual interventon is needed.v18.xIssues 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