From c4d75ea6d5887aee82e67888a1ac21e3bf9ccbdb Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 11 Sep 2025 13:56:06 -0700 Subject: [PATCH] Windows: Fix issues with paths in extensions (#37811) ### Background Zed extensions use WASI to access the file-system. They only have read-write access to one specific folder called their work dir. But extensions do need to be able to *refer* to other arbitrary files on the user's machine. For instance, extensions need to be able to look up existing binaries on the user's `PATH`, and request that Zed invoke them as language servers. Similarly, extensions can create paths to files in the user's project, and use them as arguments in commands that Zed should run. For these reasons, we pass *real* paths back and forth between the host and extensions; we don't try to abstract over the file-system with some virtualization scheme. On Windows, this results in a bit of mismatch, because `wasi-libc` uses *unix-like* path conventions (and thus, so does the Rust standard library when compiling to WASI). ### Change 1 - Fixing `current_dir` In order to keep the extension API minimal, extensions use the standard library function`env::current_dir()` to query the location of their "work" directory. Previously, when initializing extensions, we used the `env::set_current_dir` function to set their work directory, but on Windows, where absolute paths typically begin with a drive letter, like `C:`, the [`wasi-libc` implementation of `chdir`](https://github.com/WebAssembly/wasi-libc/blob/d1793637d8afcdc730408e7b6a19a050c3336ce7/libc-bottom-half/sources/chdir.c#L21) was prepending an extra forward slash to the path, which caused `current_dir()` to return an invalid path. See https://github.com/bytecodealliance/wasmtime/issues/10415 In this PR, I've switched our extension initialization function to *bypass* wasi-libc's `chdir` function, and instead write directly to wasi-libc's private, internal state. This is a bit of a hack, but it causes the `current_dir()` function to do what we want on Windows without any changes to extensions' source code. ### Change 2 - Working around WASI's relative path handling Once `current_dir` was fixed (giving us correct absolute paths on Windows), @kubkon and I discovered that without the spurious leading `/` character, windows absolute paths were no longer accepted by Rust's `std::fs` APIs, because they were now recognized as relative paths, and were being appended to the working directory. We first tried to override the `__wasilibc_find_abspath` function in `wasi-libc` to make it recognize windows absolute paths as being absolute, but that functionality is difficult to override. Eventually @kubkon realized that we could prevent WASI-libc's CWD handling from being linked into the WASM file by overriding the `chdir` function. wasi-libc is designed so that if you don't use their `chdir` function, then all paths will be interpreted as relative to `/`. This makes absolute paths behave correctly. Then, in order to make *relative* paths work again, we simply add a preopen for `.`. Relative paths will match that. ### Next Steps This is a change to `zed-extension-api`, so we do need to update every Zed extension to use the new version, in order for them to work on windows. Release Notes: - N/A --------- Co-authored-by: Jakub Konka --- Cargo.lock | 5 +- crates/extension_api/Cargo.toml | 2 +- crates/extension_api/src/extension_api.rs | 36 ++++++++++- crates/extension_host/src/extension_host.rs | 4 ++ .../src/extension_store_test.rs | 25 +++++--- crates/extension_host/src/wasm_host.rs | 24 ++++--- crates/fs/src/fs.rs | 14 ++++- crates/language_extension/Cargo.toml | 1 + .../src/extension_lsp_adapter.rs | 5 ++ extensions/test-extension/extension.toml | 7 ++- .../test-extension/src/test_extension.rs | 62 +++++++++++++++---- 11 files changed, 147 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a1a747adeb0c795c28ee84a1f2cac88ac5282822..1523063409f6061ab19dad863f5124c95f177627 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9108,6 +9108,7 @@ dependencies = [ "futures 0.3.31", "gpui", "language", + "log", "lsp", "project", "serde", @@ -20675,7 +20676,7 @@ dependencies = [ [[package]] name = "zed_extension_api" -version = "0.6.0" +version = "0.7.0" dependencies = [ "serde", "serde_json", @@ -20722,7 +20723,7 @@ dependencies = [ name = "zed_test_extension" version = "0.1.0" dependencies = [ - "zed_extension_api 0.6.0", + "zed_extension_api 0.7.0", ] [[package]] diff --git a/crates/extension_api/Cargo.toml b/crates/extension_api/Cargo.toml index 001df34e7ab23bc2711c7007f29f43d2b92970c0..318a0024bf4d9bae76af888b6668d7c21f37f804 100644 --- a/crates/extension_api/Cargo.toml +++ b/crates/extension_api/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "zed_extension_api" -version = "0.6.0" +version = "0.7.0" description = "APIs for creating Zed extensions in Rust" repository = "https://github.com/zed-industries/zed" documentation = "https://docs.rs/zed_extension_api" diff --git a/crates/extension_api/src/extension_api.rs b/crates/extension_api/src/extension_api.rs index 72327179ee08550994854d8b95a190ac94d84cea..723e5442098f1a66b78b86fa7ed980a18944778b 100644 --- a/crates/extension_api/src/extension_api.rs +++ b/crates/extension_api/src/extension_api.rs @@ -267,9 +267,43 @@ pub trait Extension: Send + Sync { #[macro_export] macro_rules! register_extension { ($extension_type:ty) => { + #[cfg(target_os = "wasi")] + mod wasi_ext { + unsafe extern "C" { + static mut errno: i32; + pub static mut __wasilibc_cwd: *mut std::ffi::c_char; + } + + pub fn init_cwd() { + unsafe { + // Ensure that our chdir function is linked, instead of the + // one from wasi-libc in the chdir.o translation unit. Otherwise + // we risk linking in `__wasilibc_find_relpath_alloc` which + // is a weak symbol and is being used by + // `__wasilibc_find_relpath`, which we do not want on + // Windows. + chdir(std::ptr::null()); + + __wasilibc_cwd = std::ffi::CString::new(std::env::var("PWD").unwrap()) + .unwrap() + .into_raw() + .cast(); + } + } + + #[unsafe(no_mangle)] + pub unsafe extern "C" fn chdir(raw_path: *const std::ffi::c_char) -> i32 { + // Forbid extensions from changing CWD and so return an appropriate error code. + errno = 58; // NOTSUP + return -1; + } + } + #[unsafe(export_name = "init-extension")] pub extern "C" fn __init_extension() { - std::env::set_current_dir(std::env::var("PWD").unwrap()).unwrap(); + #[cfg(target_os = "wasi")] + wasi_ext::init_cwd(); + zed_extension_api::register_extension(|| { Box::new(<$extension_type as zed_extension_api::Extension>::new()) }); diff --git a/crates/extension_host/src/extension_host.rs b/crates/extension_host/src/extension_host.rs index 0b7dd008707f4df62c1c036b1377dd04446ff8d2..e6c0c128f90c0138e3fe574390231ced882ff959 100644 --- a/crates/extension_host/src/extension_host.rs +++ b/crates/extension_host/src/extension_host.rs @@ -587,6 +587,10 @@ impl ExtensionStore { /// This can be used to make certain functionality provided by extensions /// available out-of-the-box. pub fn auto_install_extensions(&mut self, cx: &mut Context) { + if cfg!(test) { + return; + } + let extension_settings = ExtensionSettings::get_global(cx); let extensions_to_install = extension_settings diff --git a/crates/extension_host/src/extension_store_test.rs b/crates/extension_host/src/extension_store_test.rs index 347a610439c98ae020a7ebf190dd9e1a603df5a1..855077bcf87c58fb8e751d6477921d7e8bba8ad9 100644 --- a/crates/extension_host/src/extension_store_test.rs +++ b/crates/extension_host/src/extension_store_test.rs @@ -531,7 +531,6 @@ async fn test_extension_store(cx: &mut TestAppContext) { // `let fake_server = fake_servers.next().await.unwrap();`. // Reenable this test when we figure out why. #[gpui::test] -#[cfg_attr(target_os = "windows", ignore)] async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) { init_test(cx); cx.executor().allow_parking(); @@ -546,7 +545,7 @@ async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) { let test_extension_dir = root_dir.join("extensions").join(test_extension_id); let fs = Arc::new(RealFs::new(None, cx.executor())); - let extensions_dir = TempTree::new(json!({ + let extensions_tree = TempTree::new(json!({ "installed": {}, "work": {} })); @@ -554,7 +553,7 @@ async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) { "test.gleam": "" })); - let extensions_dir = extensions_dir.path().canonicalize().unwrap(); + let extensions_dir = extensions_tree.path().canonicalize().unwrap(); let project_dir = project_dir.path().canonicalize().unwrap(); let project = Project::test(fs.clone(), [project_dir.as_path()], cx).await; @@ -618,6 +617,10 @@ async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) { { "name": format!("gleam-{version}-aarch64-unknown-linux-musl.tar.gz"), "browser_download_url": asset_download_uri + }, + { + "name": format!("gleam-{version}-x86_64-pc-windows-msvc.tar.gz"), + "browser_download_url": asset_download_uri } ] } @@ -714,13 +717,17 @@ async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) { .await .unwrap(); - // todo(windows) - // This test hangs here on Windows. let fake_server = fake_servers.next().await.unwrap(); - let expected_server_path = - extensions_dir.join(format!("work/{test_extension_id}/gleam-v1.2.3/gleam")); + let work_dir = extensions_dir.join(format!("work/{test_extension_id}")); + let expected_server_path = work_dir.join("gleam-v1.2.3/gleam"); let expected_binary_contents = language_server_version.lock().binary_contents.clone(); + // check that IO operations in extension work correctly + assert!(work_dir.join("dir-created-with-rel-path").exists()); + assert!(work_dir.join("dir-created-with-abs-path").exists()); + assert!(work_dir.join("file-created-with-abs-path").exists()); + assert!(work_dir.join("file-created-with-rel-path").exists()); + assert_eq!(fake_server.binary.path, expected_server_path); assert_eq!(fake_server.binary.arguments, [OsString::from("lsp")]); assert_eq!( @@ -826,7 +833,9 @@ async fn test_extension_store_with_test_extension(cx: &mut TestAppContext) { // Reload the extension, clearing its cache. // Start a new instance of the language server. extension_store - .update(cx, |store, cx| store.reload(Some("gleam".into()), cx)) + .update(cx, |store, cx| { + store.reload(Some("test-extension".into()), cx) + }) .await; cx.executor().run_until_parked(); project.update(cx, |project, cx| { diff --git a/crates/extension_host/src/wasm_host.rs b/crates/extension_host/src/wasm_host.rs index c5bc21fc1c44659b845b7616aa1714a0872f90f3..3ff8fcf7a408a78decca2b36eff9ade89f0a4072 100644 --- a/crates/extension_host/src/wasm_host.rs +++ b/crates/extension_host/src/wasm_host.rs @@ -37,6 +37,7 @@ use std::{ sync::Arc, }; use task::{DebugScenario, SpawnInTerminal, TaskTemplate, ZedDebugConfig}; +use util::paths::SanitizedPath; use wasmtime::{ CacheStore, Engine, Store, component::{Component, ResourceTable}, @@ -607,7 +608,6 @@ impl WasmHost { let component = Component::from_binary(&this.engine, &wasm_bytes) .context("failed to compile wasm component")?; - let mut store = wasmtime::Store::new( &this.engine, WasmState { @@ -666,19 +666,17 @@ impl WasmHost { let file_perms = wasi::FilePerms::all(); let dir_perms = wasi::DirPerms::all(); + let path = SanitizedPath::new(&extension_work_dir); + + let mut ctx = wasi::WasiCtxBuilder::new(); + ctx.inherit_stdio() + .env("PWD", path.to_string()) + .env("RUST_BACKTRACE", "full"); + + ctx.preopened_dir(&path, ".", dir_perms, file_perms)?; + ctx.preopened_dir(&path, path.to_string(), dir_perms, file_perms)?; - Ok(wasi::WasiCtxBuilder::new() - .inherit_stdio() - .preopened_dir(&extension_work_dir, ".", dir_perms, file_perms)? - .preopened_dir( - &extension_work_dir, - extension_work_dir.to_string_lossy(), - dir_perms, - file_perms, - )? - .env("PWD", extension_work_dir.to_string_lossy()) - .env("RUST_BACKTRACE", "full") - .build()) + Ok(ctx.build()) } pub fn writeable_path_from_extension(&self, id: &Arc, path: &Path) -> Result { diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 98c8dc9054984c49732bec57a9604a14ceb5ee72..846d1f75d8e64263dd043cf916bcb3d23b951976 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -343,7 +343,19 @@ impl Fs for RealFs { #[cfg(windows)] if smol::fs::metadata(&target).await?.is_dir() { - smol::fs::windows::symlink_dir(target, path).await? + let status = smol::process::Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .args([path, target.as_path()]) + .status() + .await?; + + if !status.success() { + return Err(anyhow::anyhow!( + "Failed to create junction from {:?} to {:?}", + path, + target + )); + } } else { smol::fs::windows::symlink_file(target, path).await? } diff --git a/crates/language_extension/Cargo.toml b/crates/language_extension/Cargo.toml index cc73b1f92396f7264bdde6650c257682f1adb6c9..565563611e7065dd2aa20fec64f73f81e51f0d48 100644 --- a/crates/language_extension/Cargo.toml +++ b/crates/language_extension/Cargo.toml @@ -20,6 +20,7 @@ futures.workspace = true fs.workspace = true gpui.workspace = true language.workspace = true +log.workspace = true lsp.workspace = true project.workspace = true serde.workspace = true diff --git a/crates/language_extension/src/extension_lsp_adapter.rs b/crates/language_extension/src/extension_lsp_adapter.rs index 318f5e62d8d586c3cfa0cbc261ac1850641f611e..2c7cc9b7b1c73abf8daaceeefd53963c8ddf735b 100644 --- a/crates/language_extension/src/extension_lsp_adapter.rs +++ b/crates/language_extension/src/extension_lsp_adapter.rs @@ -124,6 +124,11 @@ impl ExtensionLanguageServerProxy for LanguageServerRegistryProxy { language_server_id: LanguageServerName, status: BinaryStatus, ) { + log::debug!( + "updating binary status for {} to {:?}", + language_server_id, + status + ); self.language_registry .update_lsp_binary_status(language_server_id, status); } diff --git a/extensions/test-extension/extension.toml b/extensions/test-extension/extension.toml index 0ba9eeaadf63984950302f9032bdf0e6f3f6f25e..0cb5afac7f7031c7f2eb9db2fbbcef9d2e107b68 100644 --- a/extensions/test-extension/extension.toml +++ b/extensions/test-extension/extension.toml @@ -17,4 +17,9 @@ commit = "8432ffe32ccd360534837256747beb5b1c82fca1" [[capabilities]] kind = "process:exec" command = "echo" -args = ["hello!"] +args = ["hello from a child process!"] + +[[capabilities]] +kind = "process:exec" +command = "cmd" +args = ["/C", "echo", "hello from a child process!"] diff --git a/extensions/test-extension/src/test_extension.rs b/extensions/test-extension/src/test_extension.rs index ee0b1b36a1b14b899363b86c8d24db120efaf09b..8c7737ba7faaaa64204cb04c81c16c42d8d46f37 100644 --- a/extensions/test-extension/src/test_extension.rs +++ b/extensions/test-extension/src/test_extension.rs @@ -14,9 +14,37 @@ impl TestExtension { language_server_id: &LanguageServerId, _worktree: &zed::Worktree, ) -> Result { - let echo_output = Command::new("echo").arg("hello!").output()?; + let (platform, arch) = zed::current_platform(); - println!("{}", String::from_utf8_lossy(&echo_output.stdout)); + let current_dir = std::env::current_dir().unwrap(); + println!("current_dir: {}", current_dir.display()); + + fs::create_dir_all(current_dir.join("dir-created-with-abs-path")).unwrap(); + fs::create_dir_all("./dir-created-with-rel-path").unwrap(); + fs::write("file-created-with-rel-path", b"contents 1").unwrap(); + fs::write( + current_dir.join("file-created-with-abs-path"), + b"contents 2", + ) + .unwrap(); + assert_eq!( + fs::read("file-created-with-rel-path").unwrap(), + b"contents 1" + ); + assert_eq!( + fs::read("file-created-with-abs-path").unwrap(), + b"contents 2" + ); + + let command = match platform { + zed::Os::Linux | zed::Os::Mac => Command::new("echo"), + zed::Os::Windows => Command::new("cmd").args(["/C", "echo"]), + }; + let output = command.arg("hello from a child process!").output()?; + println!( + "command output: {}", + String::from_utf8_lossy(&output.stdout).trim() + ); if let Some(path) = &self.cached_binary_path && fs::metadata(path).is_ok_and(|stat| stat.is_file()) @@ -36,9 +64,18 @@ impl TestExtension { }, )?; - let (platform, arch) = zed::current_platform(); + let ext = "tar.gz"; + let download_type = zed::DownloadedFileType::GzipTar; + + // Do this if you want to actually run this extension - + // the actual asset is a .zip. But the integration test is simpler + // if every platform uses .tar.gz. + // + // ext = "zip"; + // download_type = zed::DownloadedFileType::Zip; + let asset_name = format!( - "gleam-{version}-{arch}-{os}.tar.gz", + "gleam-{version}-{arch}-{os}.{ext}", version = release.version, arch = match arch { zed::Architecture::Aarch64 => "aarch64", @@ -67,18 +104,21 @@ impl TestExtension { &zed::LanguageServerInstallationStatus::Downloading, ); - zed::download_file( - &asset.download_url, - &version_dir, - zed::DownloadedFileType::GzipTar, - ) - .map_err(|e| format!("failed to download file: {e}"))?; + zed::download_file(&asset.download_url, &version_dir, download_type) + .map_err(|e| format!("failed to download file: {e}"))?; + + zed::set_language_server_installation_status( + language_server_id, + &zed::LanguageServerInstallationStatus::None, + ); let entries = fs::read_dir(".").map_err(|e| format!("failed to list working directory {e}"))?; for entry in entries { let entry = entry.map_err(|e| format!("failed to load directory entry {e}"))?; - if entry.file_name().to_str() != Some(&version_dir) { + let filename = entry.file_name(); + let filename = filename.to_str().unwrap(); + if filename.starts_with("gleam-") && filename != version_dir { fs::remove_dir_all(entry.path()).ok(); } }