wasi: add support for version when creating WASI by mhdawson · Pull Request #46469 · nodejs/node · GitHub

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

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

Refs: nodejs#46254 - add version to options when creating WASI object - add convenience function to return importObject Signed-off-by: Michael Dawson <mdawson@devrus.com>

Review requested:

@nodejs/wasi

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

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

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.

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'

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).

will do

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 <cjihrig@gmail.com>

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

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

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 <mdawson@devrus.com>

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

Failed to start CI

- Validating Jenkins credentials
✖  Jenkins credentials invalid

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

LGTM. Thanks for doing this.

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

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

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 <mdawson@devrus.com> PR-URL: #46469 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Landed in 2d35f66

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 <mdawson@devrus.com> PR-URL: #46469 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <mdawson@devrus.com>

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 <cjihrig@gmail.com>

Update doc/api/wasi.md

954a984

Co-authored-by: Colin Ihrig <cjihrig@gmail.com>

Update doc/api/wasi.md

848373c

Co-authored-by: Colin Ihrig <cjihrig@gmail.com>

Update doc/api/wasi.md

388dc3f

Co-authored-by: Colin Ihrig <cjihrig@gmail.com>

squash: address comments

3cab1f5

squash: address comments

61dca0b

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

squash: address comments

9f07256

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

squash: address comments

fc34053

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

squash: address comments

ad25e59

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

squash: remove test code related to unstable

9107570

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

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 label Mar 1, 2023

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

               import { argv, env } from 'node:process';
               const wasi = new WASI({
+                version: 'wasi_snapshot_preview1',

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

wasi: add support for version when creating WASI by mhdawson · Pull Request #46469 · nodejs/node · GitHub

Uh oh!

Uh oh!

wasi: add support for version when creating WASI - #46469

wasi: add support for version when creating WASI#46469mhdawson wants to merge 12 commits intonodejs:mainnodejs/node:mainfrom mhdawson:add-wasi-version-argmhdawson/io.js:add-wasi-version-argCopy head branch name to clipboard

Conversation

mhdawson commented Feb 1, 2023

Uh oh!

nodejs-github-bot commented Feb 1, 2023

Uh oh!

devsnek commented Feb 1, 2023

Uh oh!

mhdawson commented Feb 1, 2023

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybedford Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cjihrig commented Feb 3, 2023

Uh oh!

mhdawson commented Feb 6, 2023

Uh oh!

guybedford commented Feb 7, 2023

Uh oh!

mhdawson commented Feb 7, 2023

Uh oh!

github-actions Bot commented Feb 7, 2023

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

nodejs-github-bot commented Feb 21, 2023

Uh oh!

mhdawson commented Feb 22, 2023

Uh oh!

mhdawson commented Feb 22, 2023

Uh oh!

juanarbol commented Mar 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

wasi: add support for version when creating WASI#46469
mhdawson wants to merge 12 commits into
nodejs:mainfrom
mhdawson:add-wasi-version-arg

guybedford Feb 2, 2023 •
edited
Loading