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.

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

wasi: add wasi sock_accept stub #46434

Closed
wants to merge 2 commits into from
Closed

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Jan 31, 2023

Refs: nodejs/uvwasi#185

Add stub for sock_accept so that we have stubs
for all of the sock methods in wasi_snapshot_preview1.
Its a bit awkward as the method was added after the
initial definitial of wasi_snapshot-preview1 but I
think it should be semver minor at most to add
the method.

Depends on nodejs/uvwasi#185
being landed in uvwasi first and an updated version
of uvwasi that includes that being pulled into
Node.js

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/wasi

@nodejs-github-bot 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. wasi Issues and PRs related to the WebAssembly System Interface. labels Jan 31, 2023
@mhdawson mhdawson removed the wasi Issues and PRs related to the WebAssembly System Interface. label Jan 31, 2023
@mhdawson mhdawson added the wasi Issues and PRs related to the WebAssembly System Interface. label Jan 31, 2023
@mhdawson mhdawson changed the title Wasi sockets2 wasi: add wasi sock_accept stub Jan 31, 2023
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments.

src/node_wasi.cc Outdated Show resolved Hide resolved
src/node_wasi.cc Outdated Show resolved Hide resolved
src/node_wasi.h Outdated Show resolved Hide resolved
test/wasi/c/sock.c Outdated Show resolved Hide resolved
test/wasi/c/sock.c Outdated Show resolved Hide resolved
test/wasi/c/sock.c Outdated Show resolved Hide resolved
@mhdawson
Copy link
Member Author

Pushed commit to address all comment so far.

@mhdawson
Copy link
Member Author

As mentioned needs an updated version of uvwasi before tests will pass.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
Refs: nodejs/uvwasi#185

Add stub for sock_accept so that we have stubs
for all of the sock methods in wasi_snapshot_preview1.
Its a bit awkward as the method was added after the
initial definitial of wasi_snapshot-preview1 but I
think it should be semver minor at most to add
the method.

Depends on nodejs/uvwasi#185
being landed in uvwasi first and an updated version
of uvwasi that includes that being pulled into
Node.js

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson
Copy link
Member Author

Just added commit to update uvwasi to v0.0.16 so that tests can pass.

@cjihrig a re-LGTM from you might make sense due to that addition.

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

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

The release commit LGTM. Assuming the other commit hasn't changed, the PR LGTM

mhdawson added a commit that referenced this pull request Mar 1, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #46434
Refs: nodejs/uvwasi#185
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
mhdawson added a commit that referenced this pull request Mar 1, 2023
Refs: nodejs/uvwasi#185

Add stub for sock_accept so that we have stubs
for all of the sock methods in wasi_snapshot_preview1.
Its a bit awkward as the method was added after the
initial definitial of wasi_snapshot-preview1 but I
think it should be semver minor at most to add
the method.

Depends on nodejs/uvwasi#185
being landed in uvwasi first and an updated version
of uvwasi that includes that being pulled into
Node.js

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

PR-URL: #46434
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mhdawson
Copy link
Member Author

mhdawson commented Mar 1, 2023

Landed in 8e4fa26...bd04106

@mhdawson mhdawson closed this Mar 1, 2023
targos pushed a commit that referenced this pull request Mar 13, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #46434
Refs: nodejs/uvwasi#185
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Mar 13, 2023
Refs: nodejs/uvwasi#185

Add stub for sock_accept so that we have stubs
for all of the sock methods in wasi_snapshot_preview1.
Its a bit awkward as the method was added after the
initial definitial of wasi_snapshot-preview1 but I
think it should be semver minor at most to add
the method.

Depends on nodejs/uvwasi#185
being landed in uvwasi first and an updated version
of uvwasi that includes that being pulled into
Node.js

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

PR-URL: #46434
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #46434
Refs: nodejs/uvwasi#185
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
Refs: nodejs/uvwasi#185

Add stub for sock_accept so that we have stubs
for all of the sock methods in wasi_snapshot_preview1.
Its a bit awkward as the method was added after the
initial definitial of wasi_snapshot-preview1 but I
think it should be semver minor at most to add
the method.

Depends on nodejs/uvwasi#185
being landed in uvwasi first and an updated version
of uvwasi that includes that being pulled into
Node.js

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

PR-URL: #46434
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danielleadams
Copy link
Member

@mhdawson this didn't land cleanly on v18.x-staging. Do you mind opening a backport PR to v18.x? Thank you

@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Apr 3, 2023
@mhdawson
Copy link
Member Author

mhdawson commented Apr 4, 2023

@danielleadams thanks for the head up. Will add it to my list of things.

mhdawson added a commit to mhdawson/io.js that referenced this pull request Apr 6, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: nodejs#46434
Refs: nodejs/uvwasi#185
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
mhdawson added a commit to mhdawson/io.js that referenced this pull request Apr 6, 2023
Refs: nodejs/uvwasi#185

Add stub for sock_accept so that we have stubs
for all of the sock methods in wasi_snapshot_preview1.
Its a bit awkward as the method was added after the
initial definitial of wasi_snapshot-preview1 but I
think it should be semver minor at most to add
the method.

Depends on nodejs/uvwasi#185
being landed in uvwasi first and an updated version
of uvwasi that includes that being pulled into
Node.js

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

PR-URL: nodejs#46434
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mhdawson
Copy link
Member Author

mhdawson commented Apr 6, 2023

@danielleadams PR for backport - #47455

@mhdawson mhdawson added backported-to-v18.x PRs backported to the v18.x-staging branch. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backport-open-v18.x Indicate that the PR has an open backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backported-to-v18.x PRs backported to the v18.x-staging branch. labels Apr 6, 2023
danielleadams pushed a commit that referenced this pull request May 29, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #46434
Backport-PR-URL: #47455
Refs: nodejs/uvwasi#185
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this pull request May 29, 2023
Refs: nodejs/uvwasi#185

Add stub for sock_accept so that we have stubs
for all of the sock methods in wasi_snapshot_preview1.
Its a bit awkward as the method was added after the
initial definitial of wasi_snapshot-preview1 but I
think it should be semver minor at most to add
the method.

Depends on nodejs/uvwasi#185
being landed in uvwasi first and an updated version
of uvwasi that includes that being pulled into
Node.js

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

PR-URL: #46434
Backport-PR-URL: #47455
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-open-v18.x Indicate that the PR has an open backport. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. wasi Issues and PRs related to the WebAssembly System Interface.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants