Leading slash with env::current_dir() in Windows #20559
Comments
|
I have looked around and found one peculiarity: What if we try |
Nothing changes with v23 and windows instead of mingw. |
|
Thank you, good to know. One other thing I was pointed at is zed/crates/extension_host/src/wasm_host.rs Lines 313 to 315 in 55cd99c and this: zed/crates/extension_host/src/wasm_host.rs Lines 161 to 183 in 55cd99c could we debug and check what kind of paths dwell there? Or I can check this tomorrow when I'm back to my Win machine. |
|
To answer my question, seems that we're doing fine here: for a patch diff --git a/crates/extension_host/src/wasm_host.rs b/crates/extension_host/src/wasm_host.rs
index bd66a31a27..e28245d037 100644
--- a/crates/extension_host/src/wasm_host.rs
+++ b/crates/extension_host/src/wasm_host.rs
@@ -255,7 +255,7 @@ impl WasmHost {
Ok(WasmExtension {
manifest: manifest.clone(),
- work_dir: this.work_dir.clone().into(),
+ work_dir: dbg!(this.work_dir.clone()).into(),
tx,
zed_api_version,
})
@@ -265,7 +265,7 @@ impl WasmHost {
async fn build_wasi_ctx(&self, manifest: &Arc<ExtensionManifest>) -> Result<wasi::WasiCtx> {
let extension_work_dir = self.work_dir.join(manifest.id.as_ref());
self.fs
- .create_dir(&extension_work_dir)
+ .create_dir(dbg!(&extension_work_dir))
.await
.context("failed to create extension work dir")?;I still find it very suspicious that / -prefixed paths look exactly how my MINGW terminal does this:
Given that using the upgraded version of the SDK did not help and it was mingw-only version before, I lean to the fact that this is a bug in wasi. The next step we'd better do then, is creating a minimal examle to repro this, and submitting this to their repo. |
Would you like to test the windows v23 SDK yourself? In case I have done something wrong with it. Before we even file an issue to them. |
|
I did retest it by changing all SDK versions in the code and recompiling the html plugin in dev mode with it, so not sure we are missing anything. |
JosephTLyons
added
windows
language
|
The working directory has been changed using the PWD environment variable here, but this variable cannot be accessed under Windows, which may be related to this issue. zed/crates/extension_api/src/extension_api.rs Line 169 in 6cba467 |
|
It's not that, apparently, if I change the extension as diff --git a/crates/extension_api/src/extension_api.rs b/crates/extension_api/src/extension_api.rs
index 4bb1229538..7d7f17b1b3 100644
--- a/crates/extension_api/src/extension_api.rs
+++ b/crates/extension_api/src/extension_api.rs
@@ -166,6 +166,8 @@ macro_rules! register_extension {
($extension_type:ty) => {
#[export_name = "init-extension"]
pub extern "C" fn __init_extension() {
+ let a = std::env::var("PWD");
+ eprintln!("??????????????????? {:?}", a);
std::env::set_current_dir(std::env::var("PWD").unwrap()).unwrap();
zed_extension_api::register_extension(|| {
Box::new(<$extension_type as zed_extension_api::Extension>::new())
diff --git a/extensions/html/Cargo.toml b/extensions/html/Cargo.toml
index e8f8a741bd..e9ee29f203 100644
--- a/extensions/html/Cargo.toml
+++ b/extensions/html/Cargo.toml
@@ -13,4 +13,4 @@ path = "src/html.rs"
crate-type = ["cdylib"]
[dependencies]
-zed_extension_api = "0.1.0"
+zed_extension_api = { path = "../../crates/extension_api" }and install the html extension as a dev one, I see |
I think the issue lies with You can test this with your current modifications by making the following changes to your code: macro_rules! register_extension {
($extension_type:ty) => {
#[export_name = "init-extension"]
pub extern "C" fn __init_extension() {
let a = std::env::var("PWD");
eprintln!("PWD: {:?}", a);
std::env::set_current_dir(std::env::var("PWD").unwrap()).unwrap();
eprintln!("Get: {:?}", std::env::current_dir()); // Add this line
zed_extension_api::register_extension(|| {
Box::new(<$extension_type as zed_extension_api::Extension>::new())Which prints something like: PWD: Ok("C:\\Users\\awpc\\AppData\\Local\\Zed\\extensions\\work\\html")
Get: Ok("/C:\\Users\\awpc\\AppData\\Local\\Zed\\extensions\\work\\html")And under WASI, |
|
Is wasmtime's problem. We need turn problem to upstream |
Not an issue.wasmtime provides a sandbox environment that is similar to the Unix environment.That is why the first slash exists. |
|
I believe the only thing we can do is to remove the leading slash from the string in the Windows environment. :( |
I'm working on it. |
|
#22600 was closed because the fix wasn't ideal, but perfect shouldn't be the enemy of functional. So I created a Github workflow for windows builds that include @someone120's patch: lilnasy/zed-windows-builds. It's been working well for me for Astro and Svelte files.
Thanks a lot for your investigation, @someone120! I can finally use Zed on Windows.❤ |
|
What is the status on this? Not working LSPs is the main blocker for me to try out Zed, @JunkuiZhang did you come up with a more solid solution to this than @someone120 's PR? I really think that the argument from @lilnasy
should take priority here |
|
Perhaps we could modify the std of the plugin to make |
|
I have created a minimized reproduction of the behavior of We shall see what comes of it. |
I think modifying wasmtine is not a good idea. Essentially, wasmtime is a Unix-like environment, and we shouldn't force it to adapt to the Windows environment. |
|
This is probably the biggest blocker for people trying Zed on Windows at present. I'm new to Zed, Neovim being my daily, but beyond this issue most of the key Zed features seem to be working pretty well in Windows. Most other jank I've discovered is repeatable in Linux and presumably MacOS builds as well. I concur with @someone120; it appears that the fix at #14905 is the most viable cross platform solution, even though a pain point requiring zed extension api to be updated. A more hacky fix like #22600 can provide backwards compatibility for the old api which would phase out over time anyway. Going the above route allows Zed to work in a more or less (afaik) feature complete way in Windows now. Doing so sooner allows enthusiasts and early adopters to start daily driving Zed and making Windows contributions sooner, widening its appeal. Is there any deal breaker with this approach? |
This PR adds a workaround for the leading `/` on Windows paths (zed-industries/zed#20559).
|
Prior to my opening of bytecodealliance/wasmtime#10415 no one had yet done the due diligence on what the root cause of the issue is (and whose problem it actually is). Without this we cannot actually begin to craft a well-engineered solution. And indeed, it seems that eventually the goal is for Wasmtime to be able to handle this case:
We can then use this to inform our solution, from the standpoint that it is a temporary workaround until (ideally) Wasmtime is able to take care of the translation for us. In zed-extensions/astro#5 I have put together an approach on how to fix the issue in user space (the extension code). I don't have a Windows machine, but I have published v0.1.5 of the Astro extension (since that is the one the original issue was reported against). Could folks on Windows please install that version and report back whether it is working as intended? |
Hi there @maxdeviant, just checked this on my Windows laptop and yes, the language server in the new version works! |
Great! So now the solution to this issue will be for any extensions that are not working correctly on Windows to add this module verbatim to their code: /// Extensions to the Zed extension API that have not yet stabilized.
mod zed_ext {
/// Sanitizes the given path to remove the leading `/` on Windows.
///
/// On macOS and Linux this is a no-op.
///
/// This is a workaround for https://github.com/bytecodealliance/wasmtime/issues/10415.
pub fn sanitize_windows_path(path: std::path::PathBuf) -> std::path::PathBuf {
use zed_extension_api::{current_platform, Os};
let (os, _arch) = current_platform();
match os {
Os::Mac | Os::Linux => path,
Os::Windows => path
.to_string_lossy()
.to_string()
.trim_start_matches('/')
.into(),
}
}
}And then use it to sanitize any problematic paths (generally zed_ext::sanitize_windows_path(env::current_dir().unwrap())At some point in the future we may opt to ship this function as part of the extension API, but for now we can just inline it into each extension, as this will allow extensions to update without needing a new version of the extension API. |
This is based on zed-extensions/astro#5 which addresses zed-industries/zed#20559 so that svelte users on windows can also use the extension. Credits to @maxdeviant for the original astro fix --------- Co-authored-by: Marshall Bowers <[email protected]>
Check for existing issues
Describe the bug / provide steps to reproduce it
The issue is related to LSPs not spinning up due to a message like this one:
or this one:
The problem is related to the WASM runtime in which the extensions run. The
env::current_dir()is abstracted away from the base platform in which Zed is running. You can check this withenv::constants::OSwhich returns an empty string.env::current_dir()returns this path:"/C:\\Users\\alvga\\AppData\\Local\\Zed\\extensions\\work\\astro_1/node_modules/@astrojs/language-server/bin/nodeServer.js". You will see a leading forward slash which then is passed to thezed::Commandand interprets it as some disk, and ultimately having the first presented log.This may not only impact extensions but maybe other crates as well.
Environment
Zed: v0.162.0 (Zed Dev 4ca1837)
OS: Windows 10.0.22631
Memory: 47.9 GiB
Architecture: x86_64
GPU: NVIDIA GeForce RTX 3060 || NVIDIA || 565.90
If applicable, add mockups / screenshots to help explain present your vision of the feature
No response
If applicable, attach your Zed.log file to this issue.
No response
The text was updated successfully, but these errors were encountered: