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

Input handling is based solely on TTY detection #5724

Closed
alexeagle opened this issue Sep 2, 2022 · 8 comments
Closed

Input handling is based solely on TTY detection #5724

alexeagle opened this issue Sep 2, 2022 · 8 comments

Comments

@alexeagle
Copy link
Contributor

alexeagle commented Sep 2, 2022

Describe the bug

#5017 mentions the problem, but jumps into a big refactoring and has stalled. I'll try to distill the problem.

This bit of code

fn collect_stdin_input() -> Option<String> {
if atty::is(atty::Stream::Stdin) {
return None;
}
Some(
io::stdin()
.lock()
.lines()
.map(|line| line.expect("Not able to read stdin"))
.collect::<Vec<String>>()
.join("\n"),
)
}

returns input from stdin purely based on a syscall to Rust's TTY detection.

Then this is handled non-conditionally where it returns:

let stdin_input = collect_stdin_input();
if stdin_input.is_some() && !self.files.is_empty() {
anyhow::bail!("Cannot specify inputs from stdin and files at the same time");
}

Which means that if swc does not detect a TTY, you are forced to pass input on stdin.
This is especially bad when you want to provide a directory of files as the inputs. There's no way to do this with the pure rust "swcx" binary.

(Bazel runs actions in a hermetic environment, or sometimes remotely on a farm of workers, and there is no TTY present)

Expected behavior

There ought to be some flag or environment variable the pure rust CLI can use to determine when to read input from stdin.
To make it simple and non-breaking, I'm not expecting a behavior change by default, just some affordance for tooling like Bazel to choose the codepath where inputs (in particular, a folder) can be passed as an argument to swc.

Version

1.2.246

@alexeagle alexeagle added the C-bug label Sep 2, 2022
@kdy1 kdy1 added this to the Planned milestone Sep 3, 2022
@kwonoj kwonoj added the A: cli label Sep 3, 2022
@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2022

This is inherited behavior from legacy @swc/cli. Not saying I am in favor of this or something, just saying this was introduced to ensure feature parity to existing cli. We might declare breaking changes and make things more explicit, or expanding option as suggested.

@alexeagle
Copy link
Contributor Author

This isn't what happens with the @swc/cli wrapper though. We currently use that under Bazel and always pass files/directories in the command line arguments. It either does detect a TTY or follows some other codepath?

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2022

Yes, there are some behavioral differences due to how rust detects stdin. Just saying why the current code works in this way. In any case, I agree we'd like to change current behavior.

@alexeagle
Copy link
Contributor Author

I'm trying to decide if we should

  1. write a custom wrapper CLI around swcx that reads a directory argument, and calls through to swcx with each file found as stdin. FWIW we had to do this for Terser https://github.com/aspect-build/rules_terser/blob/main/terser/private/run_terser.js but it's extra maintenance burden, diverges from the upstream tool, and we wouldn't want a JS dependency here.
  2. drop directory support from our bazel rules_swc
  3. wait for a fix here that lets us override the tty detection
  4. find some hack that actually does attach a tiny pseudoterminal to stdin so Rust is fooled into thinking there's a TTY, like https://unix.stackexchange.com/questions/157458/make-program-in-a-pipe-think-it-has-tty

What do you think? Do you expect it will take a while to agree on what change is desired here?

@kwonoj
Copy link
Member

kwonoj commented Sep 3, 2022

I think upstream will eventually pick 3, but I can't promise when, unfortunately. For the design I wouldn't expect we'll take too long though. Probably I'll accept some breaking changes, or even taking out stdin support as stdin usecase was quite thin anyway.

@alexeagle
Copy link
Contributor Author

Thanks OJ - FWIW I was able to use socat with a crazy command to wrap swcx, essentially
socat - SYSTEM:"path/to/swcx compile my/input/dir --out-dir bazel-out",pty under Bazel to hook a tty to the swcx process, and then it does accept folders like the node CLI did :)

So perhaps I'll just include a hermetic compilation for socat and make that part of our swc toolchain in the case where we minify a directory, in case it takes a long time to resolve this.

@alexeagle
Copy link
Contributor Author

I think this was fixed in #6714

@kdy1 kdy1 modified the milestones: Planned, v1.3.26 Jan 11, 2023
@swc-bot
Copy link
Collaborator

swc-bot commented Feb 10, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

4 participants