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

wasi: make returnOnExit true by default #47390

Closed
wants to merge 7 commits into from
Closed

wasi: make returnOnExit true by default #47390

wants to merge 7 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Apr 3, 2023

Refs: #46923

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/wasi

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. wasi Issues and PRs related to the WebAssembly System Interface. labels Apr 3, 2023
@mhdawson
Copy link
Member Author

mhdawson commented Apr 3, 2023

We agreed in #46923 that even though this is breaking, since wasi is still experimental we will mark it as SemVer minor but not backport to LTS versions earlier that 20.x

@mhdawson mhdawson added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v19.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Apr 3, 2023
@richardlau
Copy link
Member

This shouldn't be titled with "doc" subsystem -- it's an actual functional change.

@mhdawson mhdawson mentioned this pull request Apr 3, 2023
Closed
@mhdawson mhdawson changed the title doc: make returnOnExit true by default wasi: make returnOnExit true by default Apr 3, 2023
@mhdawson
Copy link
Member Author

mhdawson commented Apr 3, 2023

@richardlau fixed.

VoltrexKeyva
VoltrexKeyva reviewed Apr 4, 2023
View reviewed changes
doc/api/wasi.md Outdated Show resolved Hide resolved
doc/api/wasi.md Outdated Show resolved Hide resolved
lib/wasi.js Outdated Show resolved Hide resolved
lib/wasi.js Outdated Show resolved Hide resolved
cjihrig
cjihrig approved these changes Apr 4, 2023
View reviewed changes
test/wasi/test-wasi.js Outdated Show resolved Hide resolved
test/wasi/test-wasi.js Outdated
Comment on lines 99 to 100
if (options.returnOnExit === false)
opts.env.RETURN_ON_EXIT = 'false';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be setting this to 'true' as well if it is specified? It might be enough to do this since the environment variables will be strings in the child process:

if ('returnOnExit' in options) {
  opts.env.RETURN_ON_EXIT = options.returnOnExit;
}

Note - if for whatever reason that doesn't work, we can do String(options.returnOnExit) on the right hand side of the assignment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not do that originally since true is the default, but it would be more complete. Will update.

Copy link
Member Author

Choose a reason for hiding this comment

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

pushed commit to do that.

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@mhdawson
Copy link
Member Author

mhdawson commented Apr 6, 2023

Please don't land this until after #47391 lands as we want 47391 to make it into Node.js 20 and I'm not sure if there will be conflicts betwen the 2. If there are I'd prefer to get 47391 landed and then rebase this PR

@mhdawson
Copy link
Member Author

mhdawson commented Apr 6, 2023

Rebased, will start new CI

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

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

mhdawson and others added 6 commits April 6, 2023 19:44
@mhdawson @VoltrexKeyva
Update doc/api/wasi.md
35eb47b 
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
@mhdawson @VoltrexKeyva
Update doc/api/wasi.md
ba8fc08 
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
@mhdawson @cjihrig
Update test/wasi/test-wasi.js
76470ca 
Co-authored-by: Colin Ihrig <[email protected]>
@mhdawson @VoltrexKeyva
Update lib/wasi.js
99d8a2f 
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
@mhdawson @VoltrexKeyva
Update lib/wasi.js
1437def 
Co-authored-by: Mohammed Keyvanzadeh <[email protected]>
@mhdawson
squash: address comments
773abe5 
Signed-off-by: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member Author

mhdawson commented Apr 6, 2023

guessing using the UI did not work, will fix then force push

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

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

@nodejs-github-bot
Copy link
Collaborator

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

mhdawson added a commit that referenced this pull request Apr 11, 2023
@mhdawson
wasi: make returnOnExit true by default
56ccd59 
Refs: #46923

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #47390
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mhdawson
Copy link
Member Author

Landed in 56ccd59

@mhdawson mhdawson closed this Apr 11, 2023
targos pushed a commit that referenced this pull request May 2, 2023
@mhdawson @targos
wasi: make returnOnExit true by default
75c1d1b 
Refs: #46923

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #47390
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request May 2, 2023
@targos
2023-05-02, Version 20.1.0 (Current)
4f64397 
Notable changes:

crypto:
  * update root certificates to NSS 3.89 (Node.js GitHub Bot) #47659
dns:
  * (SEMVER-MINOR) expose getDefaultResultOrder (btea) #46973
doc:
  * add KhafraDev to collaborators (Matthew Aitken) #47510
fs:
  * add recursive option to readdir and opendir (Ethan Arrowood) #41439
  * (SEMVER-MINOR) add support for mode flag to specify the copy behavior (Tetsuharu Ohzeki) #47084
stream:
  * (SEMVER-MINOR) preserve object mode in compose (Raz Luvaton) #47413
test_runner:
  * (SEMVER-MINOR) support combining coverage reports (Colin Ihrig) #47686
wasi:
  * (SEMVER-MINOR) make returnOnExit true by default (Michael Dawson) #47390

PR-URL: TODO
targos added a commit that referenced this pull request May 2, 2023
@targos
2023-05-03, Version 20.1.0 (Current)
1eca04c 
Notable changes:

assert:
  * deprecate `CallTracker` (Moshe Atlow) #47740
crypto:
  * update root certificates to NSS 3.89 (Node.js GitHub Bot) #47659
dns:
  * (SEMVER-MINOR) expose `getDefaultResultOrder` (btea) #46973
doc:
  * add KhafraDev to collaborators (Matthew Aitken) #47510
fs:
  * (SEMVER-MINOR) add `recursive` option to `readdir` and `opendir` (Ethan Arrowood) #41439
  * (SEMVER-MINOR) add support for `mode` flag to specify the copy behavior of the `cp` methods (Tetsuharu Ohzeki) #47084
http:
  * (SEMVER-MINOR) add `highWaterMark` option `http.createServer` (HinataKah0) #47405
stream:
  * (SEMVER-MINOR) preserve object mode in `compose` (Raz Luvaton) #47413
test_runner:
  * (SEMVER-MINOR) add `testNamePatterns` to `run` API (Chemi Atlow) #47648
  * (SEMVER-MINOR) execute `before` hook on test (Chemi Atlow) #47586
  * (SEMVER-MINOR) support combining coverage reports (Colin Ihrig) #47686
wasi:
  * (SEMVER-MINOR) make `returnOnExit` true by default (Michael Dawson) #47390

PR-URL: #47820
@targos targos mentioned this pull request May 2, 2023
Merged
targos added a commit that referenced this pull request May 3, 2023
@targos
2023-05-03, Version 20.1.0 (Current)
501dad8 
Notable changes:

assert:
  * deprecate `CallTracker` (Moshe Atlow) #47740
crypto:
  * update root certificates to NSS 3.89 (Node.js GitHub Bot) #47659
dns:
  * (SEMVER-MINOR) expose `getDefaultResultOrder` (btea) #46973
doc:
  * add KhafraDev to collaborators (Matthew Aitken) #47510
fs:
  * (SEMVER-MINOR) add `recursive` option to `readdir` and `opendir` (Ethan Arrowood) #41439
  * (SEMVER-MINOR) add support for `mode` flag to specify the copy behavior of the `cp` methods (Tetsuharu Ohzeki) #47084
http:
  * (SEMVER-MINOR) add `highWaterMark` option `http.createServer` (HinataKah0) #47405
stream:
  * (SEMVER-MINOR) preserve object mode in `compose` (Raz Luvaton) #47413
test_runner:
  * (SEMVER-MINOR) add `testNamePatterns` to `run` API (Chemi Atlow) #47648
  * (SEMVER-MINOR) execute `before` hook on test (Chemi Atlow) #47586
  * (SEMVER-MINOR) support combining coverage reports (Colin Ihrig) #47686
wasi:
  * (SEMVER-MINOR) make `returnOnExit` true by default (Michael Dawson) #47390

PR-URL: #47820
targos added a commit that referenced this pull request May 3, 2023
@targos
2023-05-03, Version 20.1.0 (Current)
68ef4a6 
Notable changes:

assert:
  * deprecate `CallTracker` (Moshe Atlow) #47740
crypto:
  * update root certificates to NSS 3.89 (Node.js GitHub Bot) #47659
dns:
  * (SEMVER-MINOR) expose `getDefaultResultOrder` (btea) #46973
doc:
  * add KhafraDev to collaborators (Matthew Aitken) #47510
fs:
  * (SEMVER-MINOR) add `recursive` option to `readdir` and `opendir` (Ethan Arrowood) #41439
  * (SEMVER-MINOR) add support for `mode` flag to specify the copy behavior of the `cp` methods (Tetsuharu Ohzeki) #47084
http:
  * (SEMVER-MINOR) add `highWaterMark` option `http.createServer` (HinataKah0) #47405
stream:
  * (SEMVER-MINOR) preserve object mode in `compose` (Raz Luvaton) #47413
test_runner:
  * (SEMVER-MINOR) add `testNamePatterns` to `run` API (Chemi Atlow) #47628
  * (SEMVER-MINOR) execute `before` hook on test (Chemi Atlow) #47586
  * (SEMVER-MINOR) support combining coverage reports (Colin Ihrig) #47686
wasi:
  * (SEMVER-MINOR) make `returnOnExit` true by default (Michael Dawson) #47390

PR-URL: #47820
targos added a commit that referenced this pull request May 3, 2023
@targos
2023-05-03, Version 20.1.0 (Current)
c24a61b 
Notable changes:

assert:
  * deprecate `CallTracker` (Moshe Atlow) #47740
crypto:
  * update root certificates to NSS 3.89 (Node.js GitHub Bot) #47659
dns:
  * (SEMVER-MINOR) expose `getDefaultResultOrder` (btea) #46973
doc:
  * add KhafraDev to collaborators (Matthew Aitken) #47510
fs:
  * (SEMVER-MINOR) add `recursive` option to `readdir` and `opendir` (Ethan Arrowood) #41439
  * (SEMVER-MINOR) add support for `mode` flag to specify the copy behavior of the `cp` methods (Tetsuharu Ohzeki) #47084
http:
  * (SEMVER-MINOR) add `highWaterMark` option `http.createServer` (HinataKah0) #47405
stream:
  * (SEMVER-MINOR) preserve object mode in `compose` (Raz Luvaton) #47413
test_runner:
  * (SEMVER-MINOR) add `testNamePatterns` to `run` API (Chemi Atlow) #47628
  * (SEMVER-MINOR) execute `before` hook on test (Chemi Atlow) #47586
  * (SEMVER-MINOR) support combining coverage reports (Colin Ihrig) #47686
wasi:
  * (SEMVER-MINOR) make `returnOnExit` true by default (Michael Dawson) #47390

PR-URL: #47820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva left review comments

@richardlau richardlau richardlau approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@guybedford guybedford guybedford approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. wasi Issues and PRs related to the WebAssembly System Interface.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mhdawson @nodejs-github-bot @richardlau @jasnell @guybedford @cjihrig @tniessen @VoltrexKeyva