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

add stub for sock_accept #185

Merged
merged 2 commits into from
Feb 22, 2023
Merged

add stub for sock_accept #185

merged 2 commits into from
Feb 22, 2023

Conversation

mhdawson
Copy link
Member

add the stub for sock_accept so that we have
stubs for all method in the current version of
snapshot 1. sock_accept was added later after
snapshot 1 was first documented.

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

@mhdawson mhdawson marked this pull request as draft January 30, 2023 19:40
@mhdawson
Copy link
Member Author

Sorry pushed the button a bit early, moved to draft until I build as per uvwasi instructions instead of as dep of Node.js

@mhdawson mhdawson marked this pull request as ready for review January 30, 2023 20:52
@mhdawson mhdawson changed the title add sub for sock_accept add stub for sock_accept Jan 31, 2023
mhdawson added a commit to mhdawson/io.js that referenced this pull request Jan 31, 2023
@mhdawson
wasi: add stub for sock-accept
917512d 
Refs: nodejs/uvwasi#185

Add stub for sock-accept so that we have stubs for all snapshot 1
methods. Since they changed the spec late in the same it is
awkward but I think it is semver minor at most to add the missing
stub.

Depends on the uvwasi nodejs/uvwasi#185
to land first and to have an updated uvwasi version pulled into
Node.js.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
mhdawson added a commit to mhdawson/io.js that referenced this pull request Jan 31, 2023
@mhdawson
wasi: add wasi socki_accept stub
6f3a82a 
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 added a commit to mhdawson/io.js that referenced this pull request Jan 31, 2023
@mhdawson
wasi: add wasi sock_accept stub
e6d345b 
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 mhdawson mentioned this pull request Jan 31, 2023
Closed
mhdawson added a commit to mhdawson/io.js that referenced this pull request Jan 31, 2023
@mhdawson
wasi: add wasi sock_accept stub
16fbfbe 
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>
cjihrig
cjihrig reviewed Jan 31, 2023
View reviewed changes
include/uvwasi.h Outdated
@@ -267,6 +267,10 @@ uvwasi_errno_t uvwasi_sock_send(uvwasi_t* uvwasi,
uvwasi_errno_t uvwasi_sock_shutdown(uvwasi_t* uvwasi,
uvwasi_fd_t sock,
uvwasi_sdflags_t how);
uvwasi_errno_t uvwasi_sock_accept(uvwasi_t* uvwasi,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please put this function in sorted order in the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig pushed a commit to update order

src/uvwasi.c Outdated
Comment on lines 2564 to 2565
/* TODO(mhdawson): Waiting to implement, pending
https://github.com/WebAssembly/WASI/issues/4 */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having a TODO is good, but I don't think this specific text applies in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig updated.

cjihrig
cjihrig approved these changes Jan 31, 2023
View reviewed changes
Copy link
Collaborator

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

The Android build failure in the CI looks new but does not appear related to this change.

@mhdawson
Copy link
Member Author

@cjihrig is there anything I should do to help get this landed? I could push the button, but not sure I should do that even though you've approved?

@cjihrig
Copy link
Collaborator

cjihrig commented Feb 18, 2023

@mhdawson feel free to merge. I'll open an issue for the Android build failure.

@cjihrig cjihrig mentioned this pull request Feb 18, 2023
Closed
@cjihrig
Copy link
Collaborator

cjihrig commented Feb 20, 2023

FYI - you should be able to get a green CI run after #189 lands.

mhdawson and others added 2 commits February 22, 2023 10:53
@mhdawson
add sub for sock_accept
b2b4ec6 
Refs: nodejs#183

add the stub for sock_accept so that we have
stubs for all method in the current version of
snapshot 1. sock_accept was added later after
snapshot 1 was first documented.

Does not complete nodejs#183 but is first step.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson
squash: address comments
9ef5eb3 
Signed-off-by: Michael Dawson <midawson@redhat.com>
@mhdawson
Copy link
Member Author

mhdawson commented Feb 22, 2023 •
edited
Loading

Rebased to pull in #189

@mhdawson
Copy link
Member Author

@cjihrig is it normal for the windows CI to take so long? I see the one after the android fix landed has been running for over 5 hours?

@cjihrig
Copy link
Collaborator

cjihrig commented Feb 22, 2023

It's not normal. I restarted the CI one more time and everything is green now.

@mhdawson
Copy link
Member Author

@cjihrig thanks, landing.

@mhdawson mhdawson merged commit 96f37fe into nodejs:main Feb 22, 2023
mhdawson added a commit to mhdawson/io.js that referenced this pull request Feb 23, 2023
@mhdawson
wasi: add wasi sock_accept stub
5e2de27 
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 added a commit to nodejs/node that referenced this pull request Mar 1, 2023
@mhdawson
deps: update uvwasi to v0.0.16
b185e97 
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 to nodejs/node that referenced this pull request Mar 1, 2023
@mhdawson
wasi: add wasi sock_accept stub
bd04106 
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 to nodejs/node that referenced this pull request Mar 13, 2023
@mhdawson @targos
deps: update uvwasi to v0.0.16
dc4c5f5 
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 to nodejs/node that referenced this pull request Mar 13, 2023
@mhdawson @targos
wasi: add wasi sock_accept stub
0f3b091 
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 to nodejs/node that referenced this pull request Mar 14, 2023
@mhdawson @targos
deps: update uvwasi to v0.0.16
7ab7f97 
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 to nodejs/node that referenced this pull request Mar 14, 2023
@mhdawson @targos
wasi: add wasi sock_accept stub
b2a80d7 
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 added a commit to mhdawson/io.js that referenced this pull request Apr 6, 2023
@mhdawson
deps: update uvwasi to v0.0.16
e588b63 
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
@mhdawson
wasi: add wasi sock_accept stub
74a82ea 
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>
danielleadams pushed a commit to nodejs/node that referenced this pull request May 29, 2023
@mhdawson @danielleadams
deps: update uvwasi to v0.0.16
5f23872 
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 to nodejs/node that referenced this pull request May 29, 2023
@mhdawson @danielleadams
wasi: add wasi sock_accept stub
ddbf518 
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
Reviewers

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mhdawson @cjihrig