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

Make stdin a psuedoterminal when isatty is set #1265

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion share/wake/lib/system/job.wake
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,10 @@ export def makeJSONRunner (plan: JSONRunnerPlan): Runner =
Fail f = Pair (Fail (makeError f)) ""
Pass inFile =
def outFile = "{build}/{prefix}.out.json"
def cmd = script, "-I", "-p", inFile, "-o", outFile, extraArgs
def always_args = "-I", "-p", inFile, "-o", outFile, extraArgs
def optional_arg =
if isatty then "-i", Nil else Nil
def cmd = script, optional_arg ++ always_args

def proxy =
RunnerInput label cmd Nil (extraEnv ++ environment) "." "" Nil prefix (estimate record) isatty
Expand Down
10 changes: 9 additions & 1 deletion src/runtime/job.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -829,9 +829,17 @@ static void launch(JobTable *jobtable) {
jobtable->imp->pipes[stdout_stream[0]] = entry;
jobtable->imp->pipes[stderr_stream[0]] = entry;
clock_gettime(CLOCK_REALTIME, &entry->job->start);
std::string stdin_file_str = "/dev/null";
int stdin_stream[2];
Copy link
Collaborator

@V-FEXrt V-FEXrt May 24, 2023

Choose a reason for hiding this comment

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

I'm not going to block, but I really think this should be explicit via a tuple data in wakelang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean exactly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mentioned in chat but something like

data StdInOption =
  DevNull
  Interactive
  File (filename)


def job = 
  makeExecPlan ......
  | setPlanStdinOption DevNull
  | runJob

if (!task.stdin_file.empty()) {
stdin_file_str = task.stdin_file;
} else if (task.is_atty) {
create_psuedoterminal(stdin_stream);
stdin_file_str = "#" + std::to_string(stdin_stream[1]);
}
std::stringstream prelude;
prelude << find_execpath() << "/../lib/wake/shim-wake" << '\0'
<< (task.stdin_file.empty() ? "/dev/null" : task.stdin_file.c_str()) << '\0'
<< stdin_file_str << '\0'
<< std::to_string(stdout_stream[1]) << '\0' << std::to_string(stderr_stream[1]) << '\0'
<< task.dir << '\0';
std::string shim = prelude.str() + task.cmdline;
Expand Down
15 changes: 11 additions & 4 deletions tools/shim-wake/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,17 @@ int main(int argc, char **argv) {
return 127;
}

stdin_fd = open(argv[1], O_RDONLY);
if (stdin_fd == -1) {
fprintf(stderr, "open: %s: %s\n", argv[1], strerror(errno));
return 127;
// Because we want to be able to read in both file descirptors and
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: descirptors

// files for stdin, we use a '#' to denote a file descriptor.
const char* stdin_file = argv[1];
if (stdin_file[0] == '#') {
stdin_fd = atoi(stdin_file + 1);
} else {
stdin_fd = open(stdin_file, O_RDONLY);
if (stdin_fd == -1) {
fprintf(stderr, "open: %s: %s\n", argv[1], strerror(errno));
return 127;
Comment on lines +157 to +159
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this if after the if/else in case atoi fails for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

atoi does not tell you if it fails so we can't catch the error :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just meant something like

if (stdin_file[0] == '#') {
    stdin_fd = atoi(stdin_file + 1);
  } else {
    stdin_fd = open(stdin_file, O_RDONLY);
}

if (stdin_fd == -1) {
      fprintf(stderr, "open: %s: %s\n", argv[1], strerror(errno));
      return 127;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that's incorrect for the atoi case. The dup/dup2 below will fail if something is wrong with atoi....maybe the reality is that we should not being using atoi because its a terrible interface lol.

}
}

stdout_fd = atoi(argv[2]);
Expand Down