oak-team/oak-gpui
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Simplify environment loading code (#40531)

Browse Source 
This is a refactoring PR to simplify our environment loading code by:

- Getting rid of `EnvironmentErrorMessage` in favor of using
`anyhow::Result` everywhere, with a separate `mpsc` channel to
communicate statuses that will be shown in the activity indicator
- Inlining some functions that were only called once to reduce
indirection
- Removing the separate `direnv` module

Release Notes:

- N/A
This commit is contained in:
Cole Miller
2025-10-23 03:57:33 +00:00
committed by GitHub
parent bf63ff2b91
commit 18daa9a839
6 changed files with 144 additions and 248 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/activity_indicator/src/activity_indicator.rs
+4 -5
View File
@@ -11,8 +11,7 @@ use language::{
LanguageServerStatusUpdate, ServerHealth,
};
use project::{
EnvironmentErrorMessage, LanguageServerProgress, LspStoreEvent, Project,
ProjectEnvironmentEvent,
LanguageServerProgress, LspStoreEvent, Project, ProjectEnvironmentEvent,
git_store::{GitStoreEvent, Repository},
};
use smallvec::SmallVec;
@@ -327,20 +326,20 @@ impl ActivityIndicator {
.flatten()
}
fn pending_environment_error<'a>(&'a self, cx: &'a App) -> Option<&'a EnvironmentErrorMessage> {
fn pending_environment_error<'a>(&'a self, cx: &'a App) -> Option<&'a String> {
self.project.read(cx).peek_environment_error(cx)
}
fn content_to_render(&mut self, cx: &mut Context<Self>) -> Option<Content> {
// Show if any direnv calls failed
if let Some(error) = self.pending_environment_error(cx) {
if let Some(message) = self.pending_environment_error(cx) {
return Some(Content {
icon: Some(
Icon::new(IconName::Warning)
.size(IconSize::Small)
.into_any_element(),
),
message: error.0.clone(),
message: message.clone(),
on_click: Some(Arc::new(move |this, window, cx| {
this.project.update(cx, |project, cx| {
project.pop_environment_error(cx);
crates/project/src/direnv.rs
-82
View File
@@ -1,82 +0,0 @@
use crate::environment::EnvironmentErrorMessage;
use std::process::ExitStatus;
use {collections::HashMap, std::path::Path, util::ResultExt};
#[derive(Clone)]
pub enum DirenvError {
NotFound,
FailedRun,
NonZeroExit(ExitStatus, Vec<u8>),
InvalidJson,
}
impl From<DirenvError> for Option<EnvironmentErrorMessage> {
fn from(value: DirenvError) -> Self {
match value {
DirenvError::NotFound => None,
DirenvError::FailedRun | DirenvError::NonZeroExit(_, _) => {
Some(EnvironmentErrorMessage(String::from(
"Failed to run direnv. See logs for more info",
)))
}
DirenvError::InvalidJson => Some(EnvironmentErrorMessage(String::from(
"Direnv returned invalid json. See logs for more info",
))),
}
}
}
pub async fn load_direnv_environment(
env: &HashMap<String, String>,
dir: &Path,
) -> Result<HashMap<String, Option<String>>, DirenvError> {
let Ok(direnv_path) = which::which("direnv") else {
return Err(DirenvError::NotFound);
};
let args = &["export", "json"];
let Some(direnv_output) = smol::process::Command::new(&direnv_path)
.args(args)
.envs(env)
.env("TERM", "dumb")
.current_dir(dir)
.output()
.await
.log_err()
else {
return Err(DirenvError::FailedRun);
};
if !direnv_output.status.success() {
log::error!(
"Loading direnv environment failed ({}), stderr: {}",
direnv_output.status,
String::from_utf8_lossy(&direnv_output.stderr)
);
return Err(DirenvError::NonZeroExit(
direnv_output.status,
direnv_output.stderr,
));
}
let output = String::from_utf8_lossy(&direnv_output.stdout);
if output.is_empty() {
// direnv outputs nothing when it has no changes to apply to environment variables
return Ok(HashMap::default());
}
match serde_json::from_str(&output) {
Ok(env) => Ok(env),
Err(err) => {
log::error!(
"json parse error {}, while parsing output of `{} {}`:\n{}",
err,
direnv_path.display(),
args.join(" "),
output
);
Err(DirenvError::InvalidJson)
}
}
}
crates/project/src/environment.rs
+129 -149
View File
@@ -1,4 +1,5 @@
use futures::{FutureExt, future::Shared};
use anyhow::{Context as _, bail};
use futures::{FutureExt, StreamExt as _, channel::mpsc, future::Shared};
use language::Buffer;
use remote::RemoteClient;
use rpc::proto::{self, REMOTE_SERVER_PROJECT_ID};
@@ -20,7 +21,9 @@ pub struct ProjectEnvironment {
cli_environment: Option<HashMap<String, String>>,
local_environments: HashMap<(Shell, Arc<Path>), Shared<Task<Option<HashMap<String, String>>>>>,
remote_environments: HashMap<(Shell, Arc<Path>), Shared<Task<Option<HashMap<String, String>>>>>,
environment_error_messages: VecDeque<EnvironmentErrorMessage>,
environment_error_messages: VecDeque<String>,
environment_error_messages_tx: mpsc::UnboundedSender<String>,
_tasks: Vec<Task<()>>,
}
pub enum ProjectEnvironmentEvent {
@@ -30,12 +33,24 @@ pub enum ProjectEnvironmentEvent {
impl EventEmitter<ProjectEnvironmentEvent> for ProjectEnvironment {}
impl ProjectEnvironment {
pub fn new(cli_environment: Option<HashMap<String, String>>) -> Self {
pub fn new(cli_environment: Option<HashMap<String, String>>, cx: &mut Context<Self>) -> Self {
let (tx, mut rx) = mpsc::unbounded();
let task = cx.spawn(async move |this, cx| {
while let Some(message) = rx.next().await {
this.update(cx, |this, cx| {
this.environment_error_messages.push_back(message);
cx.emit(ProjectEnvironmentEvent::ErrorsUpdated);
})
.ok();
}
});
Self {
cli_environment,
local_environments: Default::default(),
remote_environments: Default::default(),
environment_error_messages: Default::default(),
environment_error_messages_tx: tx,
_tasks: vec![task],
}
}
@@ -128,7 +143,37 @@ impl ProjectEnvironment {
self.local_environments
.entry((shell.clone(), abs_path.clone()))
.or_insert_with(|| {
get_local_directory_environment_impl(shell, abs_path.clone(), cx).shared()
let load_direnv = ProjectSettings::get_global(cx).load_direnv.clone();
let shell = shell.clone();
let tx = self.environment_error_messages_tx.clone();
cx.spawn(async move |_, cx| {
let mut shell_env = cx
.background_spawn(load_directory_shell_environment(
shell,
abs_path.clone(),
load_direnv,
tx,
))
.await
.log_err();
if let Some(shell_env) = shell_env.as_mut() {
let path = shell_env
.get("PATH")
.map(|path| path.as_str())
.unwrap_or_default();
log::info!(
"using project environment variables shell launched in {:?}. PATH={:?}",
abs_path,
path
);
set_origin_marker(shell_env, EnvironmentOrigin::WorktreeShell);
}
shell_env
})
.shared()
})
.clone()
}
@@ -165,11 +210,11 @@ impl ProjectEnvironment {
.clone()
}
pub fn peek_environment_error(&self) -> Option<&EnvironmentErrorMessage> {
pub fn peek_environment_error(&self) -> Option<&String> {
self.environment_error_messages.front()
}
pub fn pop_environment_error(&mut self) -> Option<EnvironmentErrorMessage> {
pub fn pop_environment_error(&mut self) -> Option<String> {
self.environment_error_messages.pop_front()
}
}
@@ -194,125 +239,72 @@ impl From<EnvironmentOrigin> for String {
}
}
#[derive(Debug)]
pub struct EnvironmentErrorMessage(pub String);
impl std::fmt::Display for EnvironmentErrorMessage {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.0)
}
}
impl EnvironmentErrorMessage {
#[allow(dead_code)]
fn from_str(s: &str) -> Self {
Self(String::from(s))
}
}
async fn load_directory_shell_environment(
shell: &Shell,
abs_path: &Path,
load_direnv: &DirenvSettings,
) -> (
Option<HashMap<String, String>>,
Option<EnvironmentErrorMessage>,
) {
match smol::fs::metadata(abs_path).await {
Ok(meta) => {
let dir = if meta.is_dir() {
abs_path
} else if let Some(parent) = abs_path.parent() {
parent
} else {
return (
None,
Some(EnvironmentErrorMessage(format!(
"Failed to load shell environment in {}: not a directory",
abs_path.display()
))),
);
};
shell: Shell,
abs_path: Arc<Path>,
load_direnv: DirenvSettings,
tx: mpsc::UnboundedSender<String>,
) -> anyhow::Result<HashMap<String, String>> {
let meta = smol::fs::metadata(&abs_path).await.with_context(|| {
tx.unbounded_send(format!("Failed to open {}", abs_path.display()))
.ok();
format!("stat {abs_path:?}")
})?;
load_shell_environment(shell, dir, load_direnv).await
}
Err(err) => (
None,
Some(EnvironmentErrorMessage(format!(
"Failed to load shell environment in {}: {}",
abs_path.display(),
err
))),
),
}
}
let dir = if meta.is_dir() {
abs_path.clone()
} else {
abs_path
.parent()
.with_context(|| {
tx.unbounded_send(format!("Failed to open {}", abs_path.display()))
.ok();
format!("getting parent of {abs_path:?}")
})?
.into()
};
async fn load_shell_environment(
shell: &Shell,
dir: &Path,
load_direnv: &DirenvSettings,
) -> (
Option<HashMap<String, String>>,
Option<EnvironmentErrorMessage>,
) {
use crate::direnv::load_direnv_environment;
use util::shell_env;
if cfg!(any(test, feature = "test-support")) {
let fake_env = [("ZED_FAKE_TEST_ENV".into(), "true".into())]
.into_iter()
.collect();
(Some(fake_env), None)
} else if cfg!(target_os = "windows") {
if cfg!(target_os = "windows") {
// Note: direnv is not available on Windows, so we skip direnv processing
// and just return the shell environment
let (shell, args) = shell.program_and_args();
let mut envs = match shell_env::capture(shell, args, dir).await {
Ok(envs) => envs,
Err(err) => {
util::log_err(&err);
return (
None,
Some(EnvironmentErrorMessage(format!(
"Failed to load environment variables: {}",
err
))),
);
}
};
let mut envs = util::shell_env::capture(shell.clone(), args, abs_path)
.await
.with_context(|| {
tx.unbounded_send("Failed to load environment variables".into())
.ok();
format!("capturing shell environment with {shell:?}")
})?;
if let Some(path) = envs.remove("Path") {
// windows env vars are case-insensitive, so normalize the path var
// so we can just assume `PATH` in other places
envs.insert("PATH".into(), path);
}
// Note: direnv is not available on Windows, so we skip direnv processing
// and just return the shell environment
(Some(envs), None)
Ok(envs)
} else {
let dir_ = dir.to_owned();
let (shell, args) = shell.program_and_args();
let mut envs = match shell_env::capture(shell, args, &dir_).await {
Ok(envs) => envs,
Err(err) => {
util::log_err(&err);
return (
None,
Some(EnvironmentErrorMessage::from_str(
"Failed to load environment variables. See log for details",
)),
);
}
};
let mut envs = util::shell_env::capture(shell.clone(), args, abs_path)
.await
.with_context(|| {
tx.unbounded_send("Failed to load environment variables".into())
.ok();
format!("capturing shell environment with {shell:?}")
})?;
// If the user selects `Direct` for direnv, it would set an environment
// variable that later uses to know that it should not run the hook.
// We would include in `.envs` call so it is okay to run the hook
// even if direnv direct mode is enabled.
let (direnv_environment, direnv_error) = match load_direnv {
DirenvSettings::ShellHook => (None, None),
DirenvSettings::Direct => match load_direnv_environment(&envs, dir).await {
Ok(env) => (Some(env), None),
Err(err) => (None, err.into()),
},
let direnv_environment = match load_direnv {
DirenvSettings::ShellHook => None,
DirenvSettings::Direct => load_direnv_environment(&envs, &dir)
.await
.with_context(|| {
tx.unbounded_send("Failed to load direnv environment".into())
.ok();
"load direnv environment"
})
.log_err(),
};
if let Some(direnv_environment) = direnv_environment {
for (key, value) in direnv_environment {
@@ -324,51 +316,39 @@ async fn load_shell_environment(
}
}
(Some(envs), direnv_error)
Ok(envs)
}
}
fn get_local_directory_environment_impl(
shell: &Shell,
abs_path: Arc<Path>,
cx: &Context<ProjectEnvironment>,
) -> Task<Option<HashMap<String, String>>> {
let load_direnv = ProjectSettings::get_global(cx).load_direnv.clone();
async fn load_direnv_environment(
env: &HashMap<String, String>,
dir: &Path,
) -> anyhow::Result<HashMap<String, Option<String>>> {
let direnv_path = which::which("direnv").context("finding direnv binary")?;
let shell = shell.clone();
cx.spawn(async move |this, cx| {
let (mut shell_env, error_message) = cx
.background_spawn({
let abs_path = abs_path.clone();
async move {
load_directory_shell_environment(&shell, &abs_path, &load_direnv).await
}
})
.await;
let args = &["export", "json"];
let direnv_output = smol::process::Command::new(&direnv_path)
.args(args)
.envs(env)
.env("TERM", "dumb")
.current_dir(dir)
.output()
.await
.context("running direnv")?;
if let Some(shell_env) = shell_env.as_mut() {
let path = shell_env
.get("PATH")
.map(|path| path.as_str())
.unwrap_or_default();
log::info!(
"using project environment variables shell launched in {:?}. PATH={:?}",
abs_path,
path
);
if !direnv_output.status.success() {
bail!(
"Loading direnv environment failed ({}), stderr: {}",
direnv_output.status,
String::from_utf8_lossy(&direnv_output.stderr)
);
}
set_origin_marker(shell_env, EnvironmentOrigin::WorktreeShell);
}
let output = String::from_utf8_lossy(&direnv_output.stdout);
if output.is_empty() {
// direnv outputs nothing when it has no changes to apply to environment variables
return Ok(HashMap::default());
}
if let Some(error) = error_message {
this.update(cx, |this, cx| {
log::error!("{error}");
this.environment_error_messages.push_back(error);
cx.emit(ProjectEnvironmentEvent::ErrorsUpdated)
})
.log_err();
}
shell_env
})
serde_json::from_str(&output).context("parsing direnv json")
}
crates/project/src/project.rs
+5 -9
View File
@@ -23,11 +23,10 @@ pub mod worktree_store;
#[cfg(test)]
mod project_tests;
mod direnv;
mod environment;
use buffer_diff::BufferDiff;
use context_server_store::ContextServerStore;
pub use environment::{EnvironmentErrorMessage, ProjectEnvironmentEvent};
pub use environment::ProjectEnvironmentEvent;
use git::repository::get_git_committer;
use git_store::{Repository, RepositoryId};
pub mod search_history;
@@ -1071,7 +1070,7 @@ impl Project {
let context_server_store =
cx.new(|cx| ContextServerStore::new(worktree_store.clone(), weak_self, cx));
let environment = cx.new(|_| ProjectEnvironment::new(env));
let environment = cx.new(|cx| ProjectEnvironment::new(env, cx));
let manifest_tree = ManifestTree::new(worktree_store.clone(), cx);
let toolchain_store = cx.new(|cx| {
ToolchainStore::local(
@@ -1306,7 +1305,7 @@ impl Project {
cx.subscribe(&settings_observer, Self::on_settings_observer_event)
.detach();
let environment = cx.new(|_| ProjectEnvironment::new(None));
let environment = cx.new(|cx| ProjectEnvironment::new(None, cx));
let lsp_store = cx.new(|cx| {
LspStore::new_remote(
@@ -1519,7 +1518,7 @@ impl Project {
ImageStore::remote(worktree_store.clone(), client.clone().into(), remote_id, cx)
})?;
let environment = cx.new(|_| ProjectEnvironment::new(None))?;
let environment = cx.new(|cx| ProjectEnvironment::new(None, cx))?;
let breakpoint_store =
cx.new(|_| BreakpointStore::remote(remote_id, client.clone().into()))?;
@@ -1951,10 +1950,7 @@ impl Project {
}
#[inline]
pub fn peek_environment_error<'a>(
&'a self,
cx: &'a App,
) -> Option<&'a EnvironmentErrorMessage> {
pub fn peek_environment_error<'a>(&'a self, cx: &'a App) -> Option<&'a String> {
self.environment.read(cx).peek_environment_error()
}
crates/remote_server/src/headless_project.rs
+1 -1
View File
@@ -94,7 +94,7 @@ impl HeadlessProject {
store
});
let environment = cx.new(|_| ProjectEnvironment::new(None));
let environment = cx.new(|cx| ProjectEnvironment::new(None, cx));
let manifest_tree = ManifestTree::new(worktree_store.clone(), cx);
let toolchain_store = cx.new(|cx| {
ToolchainStore::local(
crates/util/src/util.rs
+5 -2
View File
@@ -353,7 +353,10 @@ pub async fn load_login_shell_environment() -> Result<()> {
// into shell's `cd` command (and hooks) to manipulate env.
// We do this so that we get the env a user would have when spawning a shell
// in home directory.
for (name, value) in shell_env::capture(get_system_shell(), &[], paths::home_dir()).await? {
for (name, value) in shell_env::capture(get_system_shell(), &[], paths::home_dir())
.await
.with_context(|| format!("capturing environment with {:?}", get_system_shell()))?
{
unsafe { env::set_var(&name, &value) };
}
@@ -627,7 +630,7 @@ where
}
pub fn log_err<E: std::fmt::Debug>(error: &E) {
log_error_with_caller(*Location::caller(), error, log::Level::Warn);
log_error_with_caller(*Location::caller(), error, log::Level::Error);
}
pub trait TryFutureExt {