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

C-API: add functions to overwrite stdin / stdout / stderr handlers #3032

Closed
wants to merge 29 commits into from
Closed

C-API: add functions to overwrite stdin / stdout / stderr handlers #3032

wants to merge 29 commits into from

Conversation

epilys
Copy link
Contributor

@epilys epilys commented Jul 21, 2022 •
edited by fschutt
Loading

Fixes #2334

Review

  • Add functional test for this (important!)
  • Add a short description of the change to the CHANGELOG.md file

@epilys epilys self-assigned this Jul 21, 2022
@fschutt fschutt force-pushed the capi-wasi-overwrite-stdin branch 5 times, most recently from 26db0dd to 298d224 Compare July 26, 2022 13:26
@fschutt fschutt changed the title c-api: add functions to overwrite stdin and to write to it [WIP] C-API: add functions to overwrite stdin / stdout / stderr handlers Jul 26, 2022
@fschutt fschutt force-pushed the capi-wasi-overwrite-stdin branch from 298d224 to 226de59 Compare July 26, 2022 13:29
@fschutt fschutt marked this pull request as ready for review July 26, 2022 13:31
@fschutt fschutt force-pushed the capi-wasi-overwrite-stdin branch 2 times, most recently from bab041e to cc54f16 Compare August 1, 2022 13:26
@epilys epilys requested a review from syrusakbary as a code owner August 1, 2022 15:29
@fschutt fschutt mentioned this pull request Aug 2, 2022
Closed
@fschutt fschutt force-pushed the capi-wasi-overwrite-stdin branch 4 times, most recently from 02d678f to 7cbfe50 Compare August 5, 2022 10:16
@fschutt
Copy link
Contributor

fschutt commented Aug 5, 2022

@epilys as you requested, I separated the fixes from clippy and make lint into a new PR, which can be merged after this PR has been merged. The other tests should work, so this PR can be merged now.

patches.zip

@fschutt fschutt force-pushed the capi-wasi-overwrite-stdin branch from 7cbfe50 to 1778874 Compare August 5, 2022 10:30
@fschutt fschutt force-pushed the capi-wasi-overwrite-stdin branch from 1778874 to f1a3350 Compare August 5, 2022 10:35
@epilys
Copy link
Contributor Author

epilys commented Aug 5, 2022

@syrusakbary this needs a manual merge as well

syrusakbary
syrusakbary requested changes Aug 5, 2022
View reviewed changes
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

I think we may need a meeting to discuss this further. I see as an issue that we are operating just in terms of specific structs (STDIN, STDOUT, STDERR), rather than in terms of pipes.

I'd like the API to be pipe based, as I think it will be more resilient also for other kind of filesystem things. Let's discuss

@fschutt
Copy link
Contributor

fschutt commented Aug 5, 2022

You can store pipe objects inside of the structs, the current approach is much more flexible than locking the approach to pipes. I could add code to add a wasi_new_console_out_pipe() to use pipes, if you want, but I wouldn't change the overall API.

@guregu guregu mentioned this pull request Aug 8, 2022
Closed
@fschutt fschutt requested a review from syrusakbary August 19, 2022 10:40
@epilys epilys closed this Aug 24, 2022
@fschutt fschutt mentioned this pull request Aug 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syrusakbary syrusakbary syrusakbary requested changes

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

Successfully merging this pull request may close these issues.

[c-api] add more functions for stdin/stderr/stdout control
3 participants
@epilys @fschutt @syrusakbary