FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

wasi: make returnOnExit true by default by mhdawson · Pull Request #47390 · nodejs/node · GitHub

/ node Public

wasi: make returnOnExit true by default - #47390

Closed
mhdawson wants to merge 7 commits into
nodejs:mainfrom
mhdawson:uvwasi-exit-default
ConversationCommits (7)ChecksFiles changed
Closed

wasi: make returnOnExit true by default#47390
mhdawson wants to merge 7 commits into
nodejs:mainfrom
mhdawson:uvwasi-exit-default

Conversation

mhdawson commented Apr 3, 2023

Copy link
Copy Markdown
Member

Refs: #46923

nodejs-github-bot commented Apr 3, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/wasi

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 commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

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 added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v19.x labels Apr 3, 2023

richardlau commented Apr 3, 2023

Copy link
Copy Markdown
Member

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

mhdawson mentioned this pull request Apr 3, 2023
Closed
mhdawson force-pushed the uvwasi-exit-default branch from 78475e5 to d8ce188 Compare April 3, 2023 22:03
mhdawson changed the title doc: make returnOnExit true by default wasi: make returnOnExit true by default Apr 3, 2023

mhdawson commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

@richardlau fixed.

VoltrexKeyva reviewed Apr 4, 2023
View reviewed changes
Comment thread doc/api/wasi.md Outdated
Comment thread doc/api/wasi.md Outdated
Comment thread lib/wasi.js Outdated
Comment thread lib/wasi.js Outdated
cjihrig approved these changes Apr 4, 2023
View reviewed changes
Comment thread test/wasi/test-wasi.js Outdated
Comment thread test/wasi/test-wasi.js Outdated
Comment on lines 99 to 100

cjihrig Apr 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mhdawson Apr 4, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mhdawson Apr 4, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

pushed commit to do that.

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 5, 2023
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 commented Apr 5, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 5, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 6, 2023

Copy link
Copy Markdown
Collaborator

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

mhdawson commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

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 commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

Rebased, will start new CI

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2023
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 commented Apr 6, 2023

Copy link
Copy Markdown
Collaborator

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

wasi: make returnOnExit true by default
5583c30 
Refs: nodejs#46923

Signed-off-by: Michael Dawson <mdawson@devrus.com>
mhdawson and others added 6 commits April 6, 2023 19:44
Update doc/api/wasi.md
35eb47b 
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Update doc/api/wasi.md
ba8fc08 
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Update test/wasi/test-wasi.js
76470ca 
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
Update lib/wasi.js
99d8a2f 
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Update lib/wasi.js
1437def 
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
squash: address comments
773abe5 
Signed-off-by: Michael Dawson <mdawson@devrus.com>

mhdawson commented Apr 6, 2023

Copy link
Copy Markdown
Member Author

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

mhdawson force-pushed the uvwasi-exit-default branch from e4fb68c to 773abe5 Compare April 7, 2023 00:08
mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2023
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 commented Apr 7, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 11, 2023

Copy link
Copy Markdown
Collaborator

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

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

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #47390
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson commented Apr 11, 2023

Copy link
Copy Markdown
Member Author

Landed in 56ccd59

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

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #47390
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request May 2, 2023
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 mentioned this pull request May 2, 2023
Merged
targos added a commit that referenced this pull request May 3, 2023
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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
guybedford guybedford approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
richardlau richardlau approved these changes
+1 more reviewer
VoltrexKeyva VoltrexKeyva left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Back | FazBrowse Home | New Git URL