wasi: add support for version when creating WASI #46469

mhdawson · 2023-02-01T22:45:36Z

add version to options when creating WASI object
add convenience function to return importObject

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

Refs: nodejs#46254 - add version to options when creating WASI object - add convenience function to return importObject Signed-off-by: Michael Dawson <[email protected]>

nodejs-github-bot · 2023-02-01T22:45:41Z

Review requested:

@nodejs/wasi

devsnek · 2023-02-01T23:05:46Z

why do we need to explicitly pass a version? each version already has its own import namespace, so they can just coexist right?

mhdawson · 2023-02-01T23:08:31Z

@devsnek you can read some of the discussion suggestion for a version in #46254

guybedford

Thanks for working on this @mhdawson. Looks good to me and the importObject seems useful, although I was hoping we could discuss some more API changes before removing the flag as well.

guybedford · 2023-02-02T18:55:02Z

doc/api/wasi.md

@@ -16,21 +16,18 @@ import { WASI } from 'wasi';
 import { argv, env } from 'node:process';

 const wasi = new WASI({
+  version: 'wasi_snapshot_preview1',


It might be worth considering making this just preview1. The current expectation for WASI itself is that preview1 is effectively being moved to be considered stable at this point, even if that hasn't been properly announced yet.

~~At the very least, snapshot-preview1 may be a bit briefer.~~

Apparently, the shapshot part isn't expected to change, so that preview1 may well be sufficient.

@cjihrig I'd defer to you on how to best name the versions. I just used exactly what was documented as that makes it obvious what it maps to.

@guybedford probably has better access to the folks that are determining the names of the versions. I'd be in favor of at least dropping the "wasi_" prefix though.

Ok, will update to 'preview1'

As I started doing that I realised that the other thing is it means that the mapping between the version requested and what goes into the import object is not as clear. In terms of the discussion about documenting { wasi_unstable: wasi.wasiImport } adding it as a version that could be requested would make less sense if we don't have a 1 to 1 mapping between the version requested and the name of the field in the import object.

I'll wait until next week to give a bit more time for you two to comment on that aspect. If we still think usigin preview1 is better I'll document the { wasi_unstable: wasi.wasiImport } some other way.

Talked with @guybedford seems like for preview2 the binding between the version and the imports is not direct anyway so my reasoning for keeping them in line would no longer make sense. Updating to 'preview1'

doc/api/wasi.md

cjihrig · 2023-02-03T15:11:04Z

doc/api/wasi.md

-// Some WASI binaries require:
-//   const importObject = { wasi_unstable: wasi.wasiImport };
-const importObject = { wasi_snapshot_preview1: wasi.wasiImport };
-


We appear to be losing this information, which could be important to some users. I think we either need to keep this somewhere in the docs, or support passing in "wasi_unstable" as a version which would return the same API as wasi_snapshot_preview1, but with a different version string.

I'm happy to either add it back to the doc or add to "wasi_unstable" to what can be passed in as a version depending on what people think is best.

It seems like a pretty easy thing to support since it would be an alias for preview1, so I'd suggest supporting it and removing this note from the docs (but we would need to document that "wasi_unstable" is a valid version).

lib/wasi.js

cjihrig · 2023-02-03T15:28:04Z

although I was hoping we could discuss some more API changes before removing the flag as well.

@guybedford what else did you have in mind? Maybe open another issue. I'd hate for us to remove the flag and have something slip through the cracks.

Co-authored-by: Colin Ihrig <[email protected]>

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

mhdawson · 2023-02-06T23:39:08Z

@cjihrig, @guybedford I think I've addressed all the comments so far.

The one outstanding thing is that the tests are not run for 'unstable' since they were compiled against 'wasi_snapshot_preview1'. I think we would need a new set of the compiled wasm test files. Does that need to be handled in this PR or is it ok since there seems to have been no prior testing of unstable anyway?

guybedford · 2023-02-07T16:58:35Z

The unstable import interface is very old at this point so I think it is fine to treat that as a legacy path that we can support but that we don't need to explicitly test.

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

mhdawson · 2023-02-07T22:53:06Z

Pushed commit to remove disabled testing related to unstable version. Should be ready to go at this point.

github-actions · 2023-02-07T22:56:33Z

Failed to start CI

- Validating Jenkins credentials
✖  Jenkins credentials invalid

https://github.com/nodejs/node/actions/runs/4118977794

cjihrig

LGTM. Thanks for doing this.

nodejs-github-bot · 2023-02-17T18:36:22Z

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

nodejs-github-bot · 2023-02-21T23:22:45Z

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

mhdawson · 2023-02-22T14:45:59Z

Looks like status from CI did not post correct. CI is complete and passed, going to land.

Refs: #46254 - add version to options when creating WASI object - add convenience function to return importObject Signed-off-by: Michael Dawson <[email protected]> PR-URL: #46469 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

mhdawson · 2023-02-22T14:47:18Z

Landed in 2d35f66

juanarbol · 2023-03-01T20:33:49Z

Would you mind backporting this to v18.x?

Refs: #46254 - add version to options when creating WASI object - add convenience function to return importObject Signed-off-by: Michael Dawson <[email protected]> PR-URL: #46469 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47086

Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47087

wasi: add support for version when creating WASI

97ef606

Refs: nodejs#46254 - add version to options when creating WASI object - add convenience function to return importObject Signed-off-by: Michael Dawson <[email protected]>

mhdawson added the wasi Issues and PRs related to the WebAssembly System Interface. label Feb 1, 2023

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. labels Feb 1, 2023

richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 1, 2023

squash: add pr-url for doc section

53c7fa1

guybedford approved these changes Feb 2, 2023

View reviewed changes

guybedford reviewed Feb 2, 2023

View reviewed changes

jasnell approved these changes Feb 3, 2023

View reviewed changes

cjihrig reviewed Feb 3, 2023

View reviewed changes

mhdawson and others added 9 commits February 3, 2023 14:42

Update lib/wasi.js

1397b38

Co-authored-by: Colin Ihrig <[email protected]>

Update doc/api/wasi.md

954a984

Co-authored-by: Colin Ihrig <[email protected]>

Update doc/api/wasi.md

848373c

Co-authored-by: Colin Ihrig <[email protected]>

Update doc/api/wasi.md

388dc3f

Co-authored-by: Colin Ihrig <[email protected]>

squash: address comments

3cab1f5

squash: address comments

61dca0b

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

squash: address comments

9f07256

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

squash: address comments

fc34053

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

squash: address comments

ad25e59

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

squash: remove test code related to unstable

9107570

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

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 7, 2023

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 7, 2023

cjihrig approved these changes Feb 8, 2023

View reviewed changes

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023

mhdawson mentioned this pull request Feb 17, 2023

uvwasi team working tasks nodejs/uvwasi#187

Closed

This was referenced Feb 18, 2023

CI Reliability 2023-02-18 nodejs/reliability#499

Open

CI Reliability 2023-02-19 nodejs/reliability#500

Open

mhdawson closed this Feb 22, 2023

toyobayashi added a commit to toyobayashi/wasm-util that referenced this pull request Feb 26, 2023

add support for version when creating WASI (nodejs/node#46469)

2311766

juanarbol added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Mar 1, 2023

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasi: add support for version when creating WASI #46469

wasi: add support for version when creating WASI #46469

mhdawson commented Feb 1, 2023

nodejs-github-bot commented Feb 1, 2023

devsnek commented Feb 1, 2023

mhdawson commented Feb 1, 2023

guybedford left a comment

guybedford Feb 2, 2023

guybedford Feb 2, 2023 •

edited

Loading

mhdawson Feb 3, 2023

cjihrig Feb 3, 2023

mhdawson Feb 3, 2023

mhdawson Feb 3, 2023

mhdawson Feb 6, 2023

cjihrig Feb 3, 2023

mhdawson Feb 3, 2023

cjihrig Feb 3, 2023

mhdawson Feb 3, 2023

cjihrig commented Feb 3, 2023

mhdawson commented Feb 6, 2023

guybedford commented Feb 7, 2023

mhdawson commented Feb 7, 2023

github-actions bot commented Feb 7, 2023

cjihrig left a comment

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Feb 21, 2023

mhdawson commented Feb 22, 2023

mhdawson commented Feb 22, 2023

juanarbol commented Mar 1, 2023

wasi: add support for version when creating WASI #46469

wasi: add support for version when creating WASI #46469

Conversation

mhdawson commented Feb 1, 2023

nodejs-github-bot commented Feb 1, 2023

devsnek commented Feb 1, 2023

mhdawson commented Feb 1, 2023

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Feb 3, 2023

mhdawson commented Feb 6, 2023

guybedford commented Feb 7, 2023

mhdawson commented Feb 7, 2023

github-actions bot commented Feb 7, 2023

cjihrig left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Feb 21, 2023

mhdawson commented Feb 22, 2023

mhdawson commented Feb 22, 2023

juanarbol commented Mar 1, 2023

guybedford Feb 2, 2023 •

edited

Loading