From 85fdcef564426db5582b68a99d31cd6433696d75 Mon Sep 17 00:00:00 2001 From: Stanislav Alekseev <43210583+WeetHet@users.noreply.github.com> Date: Thu, 21 Mar 2024 19:40:33 +0200 Subject: [PATCH] Do not enable venv in terminal for bash-like oneshot task invocations (#8444) Release Notes: - Work around #8334 by only activating venv in the terminal not in tasks (see #8440 for a proper solution) - To use venv modify your tasks in the following way: ```json { "label": "Python main.py", "command": "sh", "args": ["-c", "source .venv/bin/activate && python3 main.py"] } ``` --------- Co-authored-by: Kirill Bulatov --- Cargo.lock | 13 +++- crates/project/src/terminals.rs | 121 +++++++++++++++++++++++--------- crates/terminal/Cargo.toml | 2 +- crates/terminal/src/terminal.rs | 22 +++--- 4 files changed, 109 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bd12b6ba6c..04bfc860bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -87,10 +87,11 @@ dependencies = [ [[package]] name = "alacritty_terminal" -version = "0.22.1-dev" -source = "git+https://github.com/alacritty/alacritty?rev=992011a4cd9a35f197acc0a0bd430d89a0d01013#992011a4cd9a35f197acc0a0bd430d89a0d01013" +version = "0.23.0-rc1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bc2c16faa5425a10be102dda76f73d76049b44746e18ddeefc44d78bbe76cbce" dependencies = [ - "base64 0.21.4", + "base64 0.22.0", "bitflags 2.4.2", "home", "libc", @@ -1292,6 +1293,12 @@ version = "0.21.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9ba43ea6f343b788c8764558649e08df62f86c6ef251fdaeb1ffd010a9ae50a2" +[[package]] +name = "base64" +version = "0.22.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9475866fec1451be56a3c2400fd081ff546538961565ccb5b7142cbd22bc7a51" + [[package]] name = "base64-simd" version = "0.8.0" diff --git a/crates/project/src/terminals.rs b/crates/project/src/terminals.rs index e9d09a0ff1..6f6511481c 100644 --- a/crates/project/src/terminals.rs +++ b/crates/project/src/terminals.rs @@ -1,4 +1,5 @@ use crate::Project; +use collections::HashMap; use gpui::{AnyWindowHandle, Context, Entity, Model, ModelContext, WeakModel}; use settings::Settings; use smol::channel::bounded; @@ -7,6 +8,7 @@ use terminal::{ terminal_settings::{self, Shell, TerminalSettings, VenvSettingsContent}, SpawnTask, TaskState, Terminal, TerminalBuilder, }; +use util::ResultExt; // #[cfg(target_os = "macos")] // use std::os::unix::ffi::OsStrExt; @@ -28,12 +30,27 @@ impl Project { "creating terminals as a guest is not supported yet" ); + let is_terminal = spawn_task.is_none(); let settings = TerminalSettings::get_global(cx); let python_settings = settings.detect_venv.clone(); let (completion_tx, completion_rx) = bounded(1); + let mut env = settings.env.clone(); + // Alacritty uses parent project's working directory when no working directory is provided + // https://github.com/alacritty/alacritty/blob/fd1a3cc79192d1d03839f0fd8c72e1f8d0fce42e/extra/man/alacritty.5.scd?plain=1#L47-L52 + + let current_directory = std::env::current_dir().ok(); + let venv_base_directory = working_directory + .as_deref() + .or(current_directory.as_deref()) + .unwrap_or_else(|| Path::new("")); // if everything fails, use relative path + let (spawn_task, shell) = if let Some(spawn_task) = spawn_task { env.extend(spawn_task.env); + // Activate minimal Python virtual environment + if let Some(python_settings) = &python_settings.as_option() { + self.set_python_venv_path_for_tasks(python_settings, venv_base_directory, &mut env); + } ( Some(TaskState { id: spawn_task.id, @@ -82,16 +99,20 @@ impl Project { }) .detach(); - if let Some(python_settings) = &python_settings.as_option() { - let activate_command = Project::get_activate_command(python_settings); - let activate_script_path = - self.find_activate_script_path(python_settings, working_directory); - self.activate_python_virtual_environment( - activate_command, - activate_script_path, - &terminal_handle, - cx, - ); + // if the terminal is not a task, activate full Python virtual environment + if is_terminal { + if let Some(python_settings) = &python_settings.as_option() { + if let Some(activate_script_path) = + self.find_activate_script_path(python_settings, venv_base_directory) + { + self.activate_python_virtual_environment( + Project::get_activate_command(python_settings), + activate_script_path, + &terminal_handle, + cx, + ); + } + } } terminal_handle }); @@ -102,12 +123,8 @@ impl Project { pub fn find_activate_script_path( &mut self, settings: &VenvSettingsContent, - working_directory: Option, + venv_base_directory: &Path, ) -> Option { - // When we are unable to resolve the working directory, the terminal builder - // defaults to '/'. We should probably encode this directly somewhere, but for - // now, let's just hard code it here. - let working_directory = working_directory.unwrap_or_else(|| Path::new("/").to_path_buf()); let activate_script_name = match settings.activate_script { terminal_settings::ActivateScript::Default => "activate", terminal_settings::ActivateScript::Csh => "activate.csh", @@ -115,17 +132,53 @@ impl Project { terminal_settings::ActivateScript::Nushell => "activate.nu", }; - for virtual_environment_name in settings.directories { - let mut path = working_directory.join(virtual_environment_name); - path.push("bin/"); - path.push(activate_script_name); + settings + .directories + .into_iter() + .find_map(|virtual_environment_name| { + let path = venv_base_directory + .join(virtual_environment_name) + .join("bin") + .join(activate_script_name); + path.exists().then_some(path) + }) + } - if path.exists() { - return Some(path); + pub fn set_python_venv_path_for_tasks( + &mut self, + settings: &VenvSettingsContent, + venv_base_directory: &Path, + env: &mut HashMap, + ) { + let activate_path = settings + .directories + .into_iter() + .find_map(|virtual_environment_name| { + let path = venv_base_directory.join(virtual_environment_name); + path.exists().then_some(path) + }); + + if let Some(path) = activate_path { + // Some tools use VIRTUAL_ENV to detect the virtual environment + env.insert( + "VIRTUAL_ENV".to_string(), + path.to_string_lossy().to_string(), + ); + + let path_bin = path.join("bin"); + // We need to set the PATH to include the virtual environment's bin directory + if let Some(paths) = std::env::var_os("PATH") { + let paths = std::iter::once(path_bin).chain(std::env::split_paths(&paths)); + if let Some(new_path) = std::env::join_paths(paths).log_err() { + env.insert("PATH".to_string(), new_path.to_string_lossy().to_string()); + } + } else { + env.insert( + "PATH".to_string(), + path.join("bin").to_string_lossy().to_string(), + ); } } - - None } fn get_activate_command(settings: &VenvSettingsContent) -> &'static str { @@ -138,22 +191,20 @@ impl Project { fn activate_python_virtual_environment( &mut self, activate_command: &'static str, - activate_script: Option, + activate_script: PathBuf, terminal_handle: &Model, cx: &mut ModelContext, ) { - if let Some(activate_script) = activate_script { - // Paths are not strings so we need to jump through some hoops to format the command without `format!` - let mut command = Vec::from(activate_command.as_bytes()); - command.push(b' '); - // Wrapping path in double quotes to catch spaces in folder name - command.extend_from_slice(b"\""); - command.extend_from_slice(activate_script.as_os_str().as_encoded_bytes()); - command.extend_from_slice(b"\""); - command.push(b'\n'); + // Paths are not strings so we need to jump through some hoops to format the command without `format!` + let mut command = Vec::from(activate_command.as_bytes()); + command.push(b' '); + // Wrapping path in double quotes to catch spaces in folder name + command.extend_from_slice(b"\""); + command.extend_from_slice(activate_script.as_os_str().as_encoded_bytes()); + command.extend_from_slice(b"\""); + command.push(b'\n'); - terminal_handle.update(cx, |this, _| this.input_bytes(command)); - } + terminal_handle.update(cx, |this, _| this.input_bytes(command)); } pub fn local_terminal_handles(&self) -> &Vec> { diff --git a/crates/terminal/Cargo.toml b/crates/terminal/Cargo.toml index 0bba4f3458..f47ee2db2e 100644 --- a/crates/terminal/Cargo.toml +++ b/crates/terminal/Cargo.toml @@ -15,7 +15,7 @@ doctest = false [dependencies] # TODO: when new version of this crate is released, change it -alacritty_terminal = { git = "https://github.com/alacritty/alacritty", rev = "992011a4cd9a35f197acc0a0bd430d89a0d01013" } +alacritty_terminal = "0.23.0-rc1" anyhow.workspace = true collections.workspace = true dirs = "4.0.0" diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs index b39c6eb53e..736202fcb0 100644 --- a/crates/terminal/src/terminal.rs +++ b/crates/terminal/src/terminal.rs @@ -310,13 +310,19 @@ impl TerminalBuilder { working_directory: Option, task: Option, shell: Shell, - env: HashMap, + mut env: HashMap, blink_settings: Option, alternate_scroll: AlternateScroll, max_scroll_history_lines: Option, window: AnyWindowHandle, completion_tx: Sender<()>, ) -> Result { + // TODO: Properly set the current locale, + env.entry("LC_ALL".to_string()) + .or_insert_with(|| "en_US.UTF-8".to_string()); + + env.insert("ZED_TERM".to_string(), "true".to_string()); + let pty_options = { let alac_shell = match shell.clone() { Shell::System => None, @@ -332,20 +338,13 @@ impl TerminalBuilder { shell: alac_shell, working_directory: working_directory.clone(), hold: false, + env: env.into_iter().collect(), } }; - // First, setup Alacritty's env + // Setup Alacritty's env setup_env(); - // Then setup configured environment variables - for (key, value) in env { - std::env::set_var(key, value); - } - //TODO: Properly set the current locale, - std::env::set_var("LC_ALL", "en_US.UTF-8"); - std::env::set_var("ZED_TERM", "true"); - let scrolling_history = if task.is_some() { // Tasks like `cargo build --all` may produce a lot of output, ergo allow maximum scrolling. // After the task finishes, we do not allow appending to that terminal, so small tasks output should not @@ -650,6 +649,9 @@ impl Terminal { self.events .push_back(InternalEvent::ColorRequest(*idx, fun_ptr.clone())); } + AlacTermEvent::ChildExit(_) => { + // TODO: Handle child exit + } } }