Skip to content

Conversation

@SteffenDE
Copy link
Contributor

Since we also need to detect WSL for reads and writes and we don't send the env to those, I added a is_wsl flag that we set on the client. Overall, running through wsl.exe feels quite a bit slower and I'm not sure if the approach is stable enough. It feels quite brittle tbh.

@SteffenDE
Copy link
Contributor Author

Note: if we want to do this, we'll also need to change the acp_proxy to run commands through WSL.

@josevalim
Copy link
Contributor

Note: if we want to do this, we'll also need to change the acp_proxy to run commands through WSL.

correct.

("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
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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!

@josevalim
Copy link
Contributor

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.

@SteffenDE
Copy link
Contributor Author

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.

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.

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.

@josevalim
Copy link
Contributor

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!!!

@SteffenDE SteffenDE marked this pull request as ready for review November 19, 2025 09:58
@SteffenDE SteffenDE merged commit fbdff7b into main Nov 19, 2025
1 check passed
@SteffenDE SteffenDE deleted the sd-wsl branch November 19, 2025 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants