-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Bridge command generation to powershell when on Windows #2319
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
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
b694b04
to
22e29cc
Compare
I have read the CLA Document and I hereby sign the CLA |
recheck |
I have read the CLA Document and I hereby sign the CLA |
recheck |
e56de43
to
9a957d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, overall this seems great! I do worry a bit about the escaping in the one place I called out. Could we at least get more unit tests to try to flag edge cases like newlines and other types of whitespace that might make parsing funny?
let first = command.first().map(String::as_str); | ||
if first != Some(ps.exe.as_str()) { | ||
// TODO (CODEX_2900): Handle escaping newlines. | ||
if command.iter().any(|a| a.contains('\n') || a.contains('\r')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit dicey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, flagged CODEX-2900 for follow-up
return Some(command); | ||
} | ||
|
||
let joined = shlex::try_join(command.iter().map(|s| s.as_str())).ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is shlex's quoting rules appropriate for Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not the best, flagged in CODEX-2900 for follow-up
use tokio::process::Command; | ||
|
||
// Prefer PowerShell 7+ (`pwsh`) if available, otherwise fall back to Windows PowerShell. | ||
let has_pwsh = Command::new("pwsh") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider tokio::join!
to run these in parallel since this is on the startup path via Session::new()
?
9a957d1
to
84cee01
Compare
What? Why? How?
Testing