-
Notifications
You must be signed in to change notification settings - Fork 4
Detect WSL #53
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
Detect WSL #53
Conversation
|
Note: if we want to do this, we'll also need to change the acp_proxy to run commands through WSL. |
correct. |
tidewave-core/src/server.rs
Outdated
| ("cmd.exe", vec!["/s", "/c", cmd]) | ||
| if env.get("WSL_DISTRO_NAME").is_some() { | ||
| // WSL case: use --cd flag and construct env string | ||
| // Build env assignments string: VAR1=value1 VAR2=value2 ... command |
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.
Also, Claude says there is this for env vars: set WSLENV=VAR1:VAR2:VAR3
We can pass all except path. May be less brittle
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.
I did use WSLENV first, but it did mess with the PATH as I did not include it. Since we know that the environment variables we have are from Linux, I thought it makes more sense to only set them inside as well.
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.
I wanted to avoid the escaping but either way is fine by me then!
|
If we are having cold feet, my suggestion is to pass is_wsl to all operations. This way the client has full control over when to wrap commands. So if it turns out this is broken, we can stop passing the flag on the client, without releasing a new version of the app. Given we need the is_wsl check on the client anyway, that sounds like a sensible route to me. |
|
We cannot do the path translation in the project, since we don't know if the App / CLI is actually running inside or outside of WSL.
Sure, we can add the flag, but overall skipping the wrapping would not help much, since disabling it would just mean that you cannot really use Tidewave any more, because in 99% of the cases, things will be broken. So if it turns out this is broken, I think we would rather go with a warning / error page for WSL as well and prevent a user from using Tidewave in a mismatched case. |
|
Agreed on all takes. Let's go with the flag on all operations, so at least the detection logic is in a single place instead of multiple, and ship this!!! |
Since we also need to detect WSL for reads and writes and we don't send the env to those, I added a
is_wslflag that we set on the client. Overall, running throughwsl.exefeels quite a bit slower and I'm not sure if the approach is stable enough. It feels quite brittle tbh.