From 65b13968a279b9815bf00b78500cd72f7cd6a31e Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 9 May 2025 12:24:28 -0700 Subject: [PATCH] Wait to locate system-installed Node until the shell environment is loaded (#30416) Release Notes: - Fixed a race condition that sometimes prevented a system-installed `node` binary from being detected. - Fixed a bug where the `node.path` setting was not respected when invoking npm. --- assets/settings/default.json | 31 ++++++------ crates/eval/src/eval.rs | 4 +- crates/node_runtime/src/node_runtime.rs | 63 ++++++++++++++----------- crates/project/src/project_settings.rs | 2 +- crates/remote_server/src/unix.rs | 4 +- crates/util/src/util.rs | 4 +- crates/zed/src/main.rs | 18 ++++--- 7 files changed, 68 insertions(+), 58 deletions(-) diff --git a/assets/settings/default.json b/assets/settings/default.json index 27a5958fb7..b76a2b9380 100644 --- a/assets/settings/default.json +++ b/assets/settings/default.json @@ -1297,21 +1297,22 @@ "JSONC": ["**/.zed/**/*.json", "**/zed/**/*.json", "**/Zed/**/*.json", "**/.vscode/**/*.json"], "Shell Script": [".env.*"] }, - // By default use a recent system version of node, or install our own. - // You can override this to use a version of node that is not in $PATH with: - // { - // "node": { - // "path": "/path/to/node" - // "npm_path": "/path/to/npm" (defaults to node_path/../npm) - // } - // } - // or to ensure Zed always downloads and installs an isolated version of node: - // { - // "node": { - // "ignore_system_version": true, - // } - // NOTE: changing this setting currently requires restarting Zed. - "node": {}, + // Settings for which version of Node.js and NPM to use when installing + // language servers and Copilot. + // + // Note: changing this setting currently requires restarting Zed. + "node": { + // By default, Zed will look for `node` and `npm` on your `$PATH`, and use the + // existing executables if their version is recent enough. Set this to `true` + // to prevent this, and force Zed to always download and install its own + // version of Node. + "ignore_system_version": false, + // You can also specify alternative paths to Node and NPM. If you specify + // `path`, but not `npm_path`, Zed will assume that `npm` is located at + // `${path}/../npm`. + "path": null, + "npm_path": null + }, // The extensions that Zed should automatically install on startup. // // If you don't want any of these extensions, add this field to your settings diff --git a/crates/eval/src/eval.rs b/crates/eval/src/eval.rs index 7a87103f44..1a73c88563 100644 --- a/crates/eval/src/eval.rs +++ b/crates/eval/src/eval.rs @@ -397,7 +397,7 @@ pub fn init(cx: &mut App) -> Arc { cx.observe_global::(move |cx| { let settings = &ProjectSettings::get_global(cx).node; let options = NodeBinaryOptions { - allow_path_lookup: !settings.ignore_system_version.unwrap_or_default(), + allow_path_lookup: !settings.ignore_system_version, allow_binary_download: true, use_paths: settings.path.as_ref().map(|node_path| { let node_path = PathBuf::from(shellexpand::tilde(node_path).as_ref()); @@ -417,7 +417,7 @@ pub fn init(cx: &mut App) -> Arc { tx.send(Some(options)).log_err(); }) .detach(); - let node_runtime = NodeRuntime::new(client.http_client(), rx); + let node_runtime = NodeRuntime::new(client.http_client(), None, rx); let extension_host_proxy = ExtensionHostProxy::global(cx); diff --git a/crates/node_runtime/src/node_runtime.rs b/crates/node_runtime/src/node_runtime.rs index c2ccba34c4..2e85e5b385 100644 --- a/crates/node_runtime/src/node_runtime.rs +++ b/crates/node_runtime/src/node_runtime.rs @@ -4,19 +4,18 @@ use anyhow::{Context, Result, anyhow, bail}; pub use archive::extract_zip; use async_compression::futures::bufread::GzipDecoder; use async_tar::Archive; -use futures::AsyncReadExt; +use futures::{AsyncReadExt, FutureExt as _, channel::oneshot, future::Shared}; use http_client::{HttpClient, Url}; use semver::Version; use serde::Deserialize; use smol::io::BufReader; use smol::{fs, lock::Mutex}; -use std::env; -use std::ffi::OsString; -use std::io; -use std::process::{Output, Stdio}; use std::{ - env::consts, + env::{self, consts}, + ffi::OsString, + io, path::{Path, PathBuf}, + process::{Output, Stdio}, sync::Arc, }; use util::ResultExt; @@ -38,11 +37,13 @@ struct NodeRuntimeState { instance: Option>, last_options: Option, options: async_watch::Receiver>, + shell_env_loaded: Shared>, } impl NodeRuntime { pub fn new( http: Arc, + shell_env_loaded: Option>, options: async_watch::Receiver>, ) -> Self { NodeRuntime(Arc::new(Mutex::new(NodeRuntimeState { @@ -50,6 +51,7 @@ impl NodeRuntime { instance: None, last_options: None, options, + shell_env_loaded: shell_env_loaded.unwrap_or(oneshot::channel().1).shared(), }))) } @@ -59,6 +61,7 @@ impl NodeRuntime { instance: None, last_options: None, options: async_watch::channel(Some(NodeBinaryOptions::default())).1, + shell_env_loaded: oneshot::channel().1.shared(), }))) } @@ -83,6 +86,7 @@ impl NodeRuntime { } if options.allow_path_lookup { + state.shell_env_loaded.clone().await.ok(); if let Some(instance) = SystemNodeRuntime::detect().await { state.instance = Some(instance.boxed_clone()); return Ok(instance); @@ -277,23 +281,6 @@ impl ManagedNodeRuntime { #[cfg(windows)] const NPM_PATH: &str = "node_modules/npm/bin/npm-cli.js"; - async fn node_environment_path(&self) -> Result { - let node_binary = self.installation_path.join(Self::NODE_PATH); - let mut env_path = vec![ - node_binary - .parent() - .expect("invalid node binary path") - .to_path_buf(), - ]; - - if let Some(existing_path) = std::env::var_os("PATH") { - let mut paths = std::env::split_paths(&existing_path).collect::>(); - env_path.append(&mut paths); - } - - std::env::join_paths(env_path).context("failed to create PATH env variable") - } - async fn install_if_needed(http: &Arc) -> Result> { log::info!("Node runtime install_if_needed"); @@ -381,6 +368,27 @@ impl ManagedNodeRuntime { } } +fn path_with_node_binary_prepended(node_binary: &Path) -> Option { + let existing_path = env::var_os("PATH"); + let node_bin_dir = node_binary.parent().map(|dir| dir.as_os_str()); + match (existing_path, node_bin_dir) { + (Some(existing_path), Some(node_bin_dir)) => { + if let Ok(joined) = env::join_paths( + [PathBuf::from(node_bin_dir)] + .into_iter() + .chain(env::split_paths(&existing_path)), + ) { + Some(joined) + } else { + Some(existing_path) + } + } + (Some(existing_path), None) => Some(existing_path), + (None, Some(node_bin_dir)) => Some(node_bin_dir.to_owned()), + _ => None, + } +} + #[async_trait::async_trait] impl NodeRuntimeTrait for ManagedNodeRuntime { fn boxed_clone(&self) -> Box { @@ -401,7 +409,7 @@ impl NodeRuntimeTrait for ManagedNodeRuntime { let attempt = || async move { let node_binary = self.installation_path.join(Self::NODE_PATH); let npm_file = self.installation_path.join(Self::NPM_PATH); - let env_path = self.node_environment_path().await?; + let env_path = path_with_node_binary_prepended(&node_binary).unwrap_or_default(); if smol::fs::metadata(&node_binary).await.is_err() { return Err(anyhow!("missing node binary file")); @@ -541,9 +549,10 @@ impl NodeRuntimeTrait for SystemNodeRuntime { ) -> anyhow::Result { let node_ca_certs = env::var(NODE_CA_CERTS_ENV_VAR).unwrap_or_else(|_| String::new()); let mut command = util::command::new_smol_command(self.npm.clone()); + let path = path_with_node_binary_prepended(&self.node).unwrap_or_default(); command .env_clear() - .env("PATH", std::env::var_os("PATH").unwrap_or_default()) + .env("PATH", path) .env(NODE_CA_CERTS_ENV_VAR, node_ca_certs) .arg(subcommand) .args(["--cache".into(), self.scratch_dir.join("cache")]) @@ -655,14 +664,14 @@ fn configure_npm_command( #[cfg(windows)] { // SYSTEMROOT is a critical environment variables for Windows. - if let Some(val) = std::env::var("SYSTEMROOT") + if let Some(val) = env::var("SYSTEMROOT") .context("Missing environment variable: SYSTEMROOT!") .log_err() { command.env("SYSTEMROOT", val); } // Without ComSpec, the post-install will always fail. - if let Some(val) = std::env::var("ComSpec") + if let Some(val) = env::var("ComSpec") .context("Missing environment variable: ComSpec!") .log_err() { diff --git a/crates/project/src/project_settings.rs b/crates/project/src/project_settings.rs index c3fb086dc2..c47d7327bc 100644 --- a/crates/project/src/project_settings.rs +++ b/crates/project/src/project_settings.rs @@ -104,7 +104,7 @@ pub struct NodeBinarySettings { pub npm_path: Option, /// If enabled, Zed will download its own copy of Node. #[serde(default)] - pub ignore_system_version: Option, + pub ignore_system_version: bool, } #[derive(Clone, Debug, Default, Serialize, Deserialize, JsonSchema)] diff --git a/crates/remote_server/src/unix.rs b/crates/remote_server/src/unix.rs index d81ae30129..425a36fa03 100644 --- a/crates/remote_server/src/unix.rs +++ b/crates/remote_server/src/unix.rs @@ -468,7 +468,7 @@ pub fn execute_run( ) }; - let node_runtime = NodeRuntime::new(http_client.clone(), node_settings_rx); + let node_runtime = NodeRuntime::new(http_client.clone(), None, node_settings_rx); let mut languages = LanguageRegistry::new(cx.background_executor().clone()); languages.set_language_server_download_dir(paths::languages_dir().clone()); @@ -796,7 +796,7 @@ fn initialize_settings( let settings = &ProjectSettings::get_global(cx).node; log::info!("Got new node settings: {:?}", settings); let options = NodeBinaryOptions { - allow_path_lookup: !settings.ignore_system_version.unwrap_or_default(), + allow_path_lookup: !settings.ignore_system_version, // TODO: Implement this setting allow_binary_download: true, use_paths: settings.path.as_ref().map(|node_path| { diff --git a/crates/util/src/util.rs b/crates/util/src/util.rs index 25bb104a5f..770c86b1db 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -259,7 +259,7 @@ where } #[cfg(unix)] -pub fn load_shell_from_passwd() -> Result<()> { +fn load_shell_from_passwd() -> Result<()> { let buflen = match unsafe { libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX) } { n if n < 0 => 1024, n => n as usize, @@ -309,6 +309,8 @@ pub fn load_shell_from_passwd() -> Result<()> { #[cfg(unix)] pub fn load_login_shell_environment() -> Result<()> { + load_shell_from_passwd().log_err(); + let marker = "ZED_LOGIN_SHELL_START"; let shell = env::var("SHELL").context( "SHELL environment variable is not assigned so we can't source login environment variables", diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index 29a5f04dd8..098c9715f2 100644 --- a/crates/zed/src/main.rs +++ b/crates/zed/src/main.rs @@ -15,7 +15,7 @@ use editor::Editor; use extension::ExtensionHostProxy; use extension_host::ExtensionStore; use fs::{Fs, RealFs}; -use futures::{StreamExt, future}; +use futures::{StreamExt, channel::oneshot, future}; use git::GitHostingProviderRegistry; use gpui::{App, AppContext as _, Application, AsyncApp, UpdateGlobal as _}; @@ -55,9 +55,6 @@ use zed::{ open_paths_with_positions, }; -#[cfg(unix)] -use util::{load_login_shell_environment, load_shell_from_passwd}; - #[cfg(feature = "mimalloc")] #[global_allocator] static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; @@ -303,15 +300,16 @@ fn main() { paths::keymap_file().clone(), ); - #[cfg(unix)] + let (shell_env_loaded_tx, shell_env_loaded_rx) = oneshot::channel(); if !stdout_is_a_pty() { app.background_executor() .spawn(async { - load_shell_from_passwd().log_err(); - load_login_shell_environment().log_err(); + #[cfg(unix)] + util::load_login_shell_environment().log_err(); + shell_env_loaded_tx.send(()).ok(); }) .detach() - }; + } app.on_open_urls({ let open_listener = open_listener.clone(); @@ -386,7 +384,7 @@ fn main() { cx.observe_global::(move |cx| { let settings = &ProjectSettings::get_global(cx).node; let options = NodeBinaryOptions { - allow_path_lookup: !settings.ignore_system_version.unwrap_or_default(), + allow_path_lookup: !settings.ignore_system_version, // TODO: Expose this setting allow_binary_download: true, use_paths: settings.path.as_ref().map(|node_path| { @@ -407,7 +405,7 @@ fn main() { tx.send(Some(options)).log_err(); }) .detach(); - let node_runtime = NodeRuntime::new(client.http_client(), rx); + let node_runtime = NodeRuntime::new(client.http_client(), Some(shell_env_loaded_rx), rx); language::init(cx); language_extension::init(extension_host_proxy.clone(), languages.clone());