Skip to content

Conversation

phated
Copy link
Contributor

@phated phated commented Dec 15, 2022

When uvwasi is used on Windows, it might be used in non-"ConsoleMode" and the uv_guess_handle function can return two different values for FILE_TYPE_CHAR because of that difference, see https://github.com/libuv/libuv/blob/v1.x/src/win/handle.c#L42-L47

This function can also return UV_FILE in the case of FILE_TYPE_DISK but I assume that a disk file will properly stat and there isn't a good way to figure out which one is being returned.

This fixes #157 (took me about a month of debugging)

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.

Thanks for the patch. This LGTM with one small change. I think you'll also have to rebase this after #180 gets approved and merged to take care of the CI.

@phated
Copy link
Contributor Author

phated commented Dec 15, 2022

Thanks for the suggestion! I've merged it and I'll rebase once #180 lands.

@cjihrig
Copy link
Collaborator

cjihrig commented Dec 16, 2022

It should be OK to rebase this now.

@phated
Copy link
Contributor Author

phated commented Dec 16, 2022

Thanks @cjihrig! This has been rebased now. Cheers 🍻

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. Thanks for the patch. Barring any surprises, this should be able to go out in the next Node 19 patch release.

@cjihrig cjihrig merged commit 6485e16 into nodejs:main Dec 16, 2022
@phated
Copy link
Contributor Author

phated commented Dec 16, 2022

Excellent! We're actually using this through wasm-micro-runtime that uses uvwasi as an experimental WASI backend. And I'm writing bindings for OCaml, which is where I encountered this bug.

@cjihrig
Copy link
Collaborator

cjihrig commented Dec 16, 2022

Oh that's cool. In that case, you won't have to wait for a Node release (which would probably be next year). I'll get a uvwasi release out today.

@cjihrig
Copy link
Collaborator

cjihrig commented Dec 16, 2022

This is published in v0.0.14.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows application without console has not stdin、stdout、stderr
2 participants