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

feat: redirect process stdio to file #2554

Merged
merged 9 commits into from Jun 21, 2019

Conversation

2 participants
@bartlomieju
Copy link
Contributor

commented Jun 20, 2019

Closes #2013

I got prototype solution to redirect process stdio to files - based on passing rid of open files.

bartlomieju added some commits Jun 20, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Proposed API for run:

export interface RunOptions {
  args: string[];
  cwd?: string;
  env?: { [key: string]: string };
  stdout?: ProcessStdio | File | number;
  stderr?: ProcessStdio | File | number;
  stdin?: ProcessStdio | File | number;
}

There is one quirk I'm not yet sure how to solve: if you specify stdout as file that has been opened in read mode there is no error, instead you just get empty file.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Looks like my implementation follows one in Rust cookbook.

I'm still not sure how to ensure file is opened for write... Also there might be issue with buffering of the output - take a look at //js/process_test.ts:runRedirectStdoutStderr - the order of messages is random, on Mac it was error, output, on Windows CI it was output, error

Calling for advice: @ry, @piscisaureus

Show resolved Hide resolved js/process_test.ts
const text = decoder.decode(fileContents);

assertStrContains(text, "error");
assertStrContains(text, "output");

This comment has been minimized.

Copy link
@ry

ry Jun 21, 2019

Collaborator

Is the order of these strings not deterministic? I guess not...

assertEquals(status.code, 0);
p.close();
file.close();
}

This comment has been minimized.

Copy link
@ry

ry Jun 21, 2019

Collaborator

Very nice. This works on windows too?

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 21, 2019

Author Contributor

CI tells it does 😅

Show resolved Hide resolved tests/subdir/redirected_stdio.ts Outdated
Show resolved Hide resolved js/process.ts Outdated
c.stderr(subprocess_stdio_map(inner.stderr()));
let stdin_rid = inner.stdin_rid();
if stdin_rid > 0 {
c.stdin(resources::get_file(stdin_rid)?);

This comment has been minimized.

Copy link
@ry

ry Jun 21, 2019

Collaborator

At least in unix, the stdio can be associated with many types of fds including pipes and sockets, not just files. Probably it doesn't work like that on Windows.

Some commentary here regarding this would be useful. Maybe a TODO to make this work with sockets. It seems like if you give a socket rid in your patch, it will result in a panic("bad rid")

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jun 21, 2019

Author Contributor

Added a TODO

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

I'm still not sure how to ensure file is opened for write...

I don't think there is way. You would just attempt it and maybe the syscall fails.

Also there might be issue with buffering of the output - take a look at //js/process_test.ts:runRedirectStdoutStderr - the order of messages is random, on Mac it was error, output, on Windows CI it was output, error

This is expected since we're reading from the subprocess's stdout/stderr asynchronously / in parallel.

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Nice fix!

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

I'm still not sure how to ensure file is opened for write...

I don't think there is way. You would just attempt it and maybe the syscall fails.

I doesn't fail... everything runs correctly, except there's nothing in file.

Also there might be issue with buffering of the output - take a look at //js/process_test.ts:runRedirectStdoutStderr - the order of messages is random, on Mac it was error, output, on Windows CI it was output, error

This is expected since we're reading from the subprocess's stdout/stderr asynchronously / in parallel.

Makes sense.

Nice fix!

Thanks!

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

I doesn't fail... everything runs correctly, except there's nothing in file.

Sounds like a problem for another day. Maybe the error from the syscall isn't being propagated correctly - or maybe that is the correct posix semantics. idk.

@ry
Copy link
Collaborator

left a comment

Looks good - can you just add some text to the jsdoc for run() describing that it can be used with File rids?

@ry

ry approved these changes Jun 21, 2019

Copy link
Collaborator

left a comment

LGTM

@ry ry merged commit 642eaf9 into denoland:master Jun 21, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bartlomieju bartlomieju deleted the bartlomieju:feat-redirect_process_to_file branch Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.