diff --git a/src/cli/global/remove.rs b/src/cli/global/remove.rs index d44ba1bf7..c71cc3a4f 100644 --- a/src/cli/global/remove.rs +++ b/src/cli/global/remove.rs @@ -1,7 +1,12 @@ -use clap::Parser; -use clap_verbosity_flag::Verbosity; - +use crate::cli::global::revert_environment_after_error; use crate::cli::has_specs::HasSpecs; +use crate::global::{EnvironmentName, ExposedName, Project, StateChanges}; +use clap::Parser; +use itertools::Itertools; +use miette::Context; +use pixi_config::{Config, ConfigCli}; +use rattler_conda_types::MatchSpec; +use std::str::FromStr; /// Removes a package previously installed into a globally accessible location via `pixi global install`. #[derive(Parser, Debug)] @@ -11,8 +16,12 @@ pub struct Args { #[arg(num_args = 1..)] packages: Vec, - #[command(flatten)] - verbose: Verbosity, + /// Specifies the environment that the dependencies need to be removed from. + #[clap(short, long, required = true)] + environment: EnvironmentName, + + #[clap(flatten)] + config: ConfigCli, } impl HasSpecs for Args { @@ -21,6 +30,80 @@ impl HasSpecs for Args { } } -pub async fn execute(_args: Args) -> miette::Result<()> { - todo!() +pub async fn execute(args: Args) -> miette::Result<()> { + let config = Config::with_cli_config(&args.config); + let project_original = Project::discover_or_create() + .await? + .with_cli_config(config.clone()); + + if project_original.environment(&args.environment).is_none() { + miette::bail!("Environment {} doesn't exist. You can create a new environment with `pixi global install`.", &args.environment); + } + + async fn apply_changes( + env_name: &EnvironmentName, + specs: &[MatchSpec], + project: &mut Project, + ) -> miette::Result { + // Remove specs from the manifest + for spec in specs { + project.manifest.remove_dependency(env_name, spec)?; + } + + // Figure out which package the exposed binaries belong to + let prefix = project.environment_prefix(env_name).await?; + + for spec in specs { + if let Some(name) = spec.clone().name { + // If the package is not existent, don't try to remove executables + if let Ok(record) = prefix.find_designated_package(&name).await { + prefix + .find_executables(&[record]) + .into_iter() + .filter_map(|(name, _path)| ExposedName::from_str(name.as_str()).ok()) + .for_each(|exposed_name| { + project + .manifest + .remove_exposed_name(env_name, &exposed_name) + .ok(); + }); + } + } + } + + // Sync environment + let state_changes = project.sync_environment(env_name).await?; + + project.manifest.save().await?; + Ok(state_changes) + } + + let mut project = project_original.clone(); + let specs = args + .specs()? + .into_iter() + .map(|(_, specs)| specs) + .collect_vec(); + + match apply_changes(&args.environment, specs.as_slice(), &mut project) + .await + .wrap_err(format!( + "Couldn't remove packages from {}", + &args.environment + )) { + Ok(state_changes) => { + state_changes.report(); + } + Err(err) => { + revert_environment_after_error(&args.environment, &project_original) + .await + .wrap_err(format!( + "Could not remove {:?}. Reverting also failed.", + args.packages + ))?; + return Err(err); + } + } + + Ok(()) } diff --git a/src/global/common.rs b/src/global/common.rs index 5016313b9..62196a24b 100644 --- a/src/global/common.rs +++ b/src/global/common.rs @@ -1,11 +1,14 @@ -use super::{EnvironmentName, ExposedName}; +use super::{extract_executable_from_script, EnvironmentName, ExposedName, Mapping}; use fancy_display::FancyDisplay; use fs_err as fs; use fs_err::tokio as tokio_fs; +use indexmap::IndexSet; use is_executable::IsExecutable; +use itertools::Itertools; use miette::{Context, IntoDiagnostic}; use pixi_config::home_path; use pixi_manifest::PrioritizedChannel; +use pixi_utils::executable_from_path; use rattler_conda_types::{Channel, ChannelConfig, NamedChannelOrUrl, PackageRecord, PrefixRecord}; use std::collections::HashMap; use std::ffi::OsStr; @@ -406,6 +409,67 @@ pub(crate) fn channel_url_to_prioritized_channel( .into()) } +/// Figures out what the status is of the exposed binaries of the environment. +/// +/// Returns a tuple of the exposed binaries to remove and the exposed binaries to add. +pub(crate) async fn get_expose_scripts_sync_status( + bin_dir: &BinDir, + env_dir: &EnvDir, + exposed: &IndexSet, +) -> miette::Result<(IndexSet, IndexSet)> { + // Get all paths to the binaries from the scripts in the bin directory. + let locally_exposed = bin_dir.files().await?; + let executable_paths = futures::future::join_all(locally_exposed.iter().map(|path| { + let path = path.clone(); + async move { + extract_executable_from_script(&path) + .await + .ok() + .map(|exec| (path, exec)) + } + })) + .await + .into_iter() + .flatten() + .collect_vec(); + + // Filter out all binaries that are related to the environment + let related_exposed = executable_paths + .into_iter() + .filter(|(_, exec)| exec.starts_with(env_dir.path())) + .map(|(path, _)| path) + .collect_vec(); + + // Get all related expose scripts not required by the environment manifest + let to_remove = related_exposed + .iter() + .filter(|path| { + !exposed + .iter() + .any(|mapping| executable_from_path(path) == mapping.exposed_name().to_string()) + }) + .cloned() + .collect::>(); + + // Get all required exposed binaries that are not yet exposed + let to_add = exposed + .iter() + .filter_map(|mapping| { + if related_exposed + .iter() + .map(|path| executable_from_path(path)) + .any(|exec| exec == mapping.exposed_name().to_string()) + { + None + } else { + Some(mapping.exposed_name().clone()) + } + }) + .collect::>(); + + Ok((to_remove, to_add)) +} + #[cfg(test)] mod tests { use super::*; @@ -507,4 +571,89 @@ mod tests { assert_eq!(executable_script_path, path.join(exposed_name.to_string())); } } + + #[tokio::test] + async fn test_get_expose_scripts_sync_status() { + let tmp_home_dir = tempfile::tempdir().unwrap(); + let tmp_home_dir_path = tmp_home_dir.path().to_path_buf(); + let env_root = EnvRoot::new(tmp_home_dir_path.clone()).unwrap(); + let env_name = EnvironmentName::from_str("test").unwrap(); + let env_dir = EnvDir::from_env_root(env_root, &env_name).await.unwrap(); + let bin_dir = BinDir::new(tmp_home_dir_path.clone()).unwrap(); + + // Test empty + let exposed = IndexSet::new(); + let (to_remove, to_add) = get_expose_scripts_sync_status(&bin_dir, &env_dir, &exposed) + .await + .unwrap(); + assert!(to_remove.is_empty()); + assert!(to_add.is_empty()); + + // Test with exposed + let mut exposed = IndexSet::new(); + exposed.insert(Mapping::new( + ExposedName::from_str("test").unwrap(), + "test".to_string(), + )); + let (to_remove, to_add) = get_expose_scripts_sync_status(&bin_dir, &env_dir, &exposed) + .await + .unwrap(); + assert!(to_remove.is_empty()); + assert_eq!(to_add.len(), 1); + + // Add a script to the bin directory + let script_path = if cfg!(windows) { + bin_dir.path().join("test.bat") + } else { + bin_dir.path().join("test") + }; + + #[cfg(windows)] + { + let script = format!( + r#" + @"{}" %* + "#, + env_dir + .path() + .join("bin") + .join("test.exe") + .to_string_lossy() + ); + tokio_fs::write(&script_path, script).await.unwrap(); + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + let script = format!( + r#"#!/bin/sh + "{}" "$@" + "#, + env_dir.path().join("bin").join("test").to_string_lossy() + ); + tokio_fs::write(&script_path, script).await.unwrap(); + // Set the file permissions to make it executable + let metadata = tokio_fs::metadata(&script_path).await.unwrap(); + let mut permissions = metadata.permissions(); + permissions.set_mode(0o755); // rwxr-xr-x + tokio_fs::set_permissions(&script_path, permissions) + .await + .unwrap(); + }; + + let (to_remove, to_add) = get_expose_scripts_sync_status(&bin_dir, &env_dir, &exposed) + .await + .unwrap(); + assert!(to_remove.is_empty()); + assert!(to_add.is_empty()); + + // Test to_remove + let (to_remove, to_add) = + get_expose_scripts_sync_status(&bin_dir, &env_dir, &IndexSet::new()) + .await + .unwrap(); + assert_eq!(to_remove.len(), 1); + assert!(to_add.is_empty()); + } } diff --git a/src/global/project/environment.rs b/src/global/project/environment.rs index 7e6d6015a..98687e5c9 100644 --- a/src/global/project/environment.rs +++ b/src/global/project/environment.rs @@ -1,5 +1,5 @@ use crate::global::install::local_environment_matches_spec; -use crate::global::{extract_executable_from_script, BinDir, EnvDir, ExposedName, Mapping}; +use crate::global::EnvDir; use crate::prefix::Prefix; use console::StyledObject; use fancy_display::FancyDisplay; @@ -7,11 +7,9 @@ use indexmap::IndexSet; use itertools::Itertools; use miette::Diagnostic; use pixi_consts::consts; -use pixi_utils::executable_from_path; use rattler_conda_types::{MatchSpec, Platform}; use regex::Regex; use serde::{self, Deserialize, Deserializer, Serialize}; -use std::path::PathBuf; use std::{fmt, str::FromStr}; use thiserror::Error; @@ -81,67 +79,6 @@ pub struct ParseEnvironmentNameError { pub attempted_parse: String, } -/// Figures out what the status is of the exposed binaries of the environment. -/// -/// Returns a tuple of the exposed binaries to remove and the exposed binaries to add. -pub(crate) async fn get_expose_scripts_sync_status( - bin_dir: &BinDir, - env_dir: &EnvDir, - exposed: &IndexSet, -) -> miette::Result<(IndexSet, IndexSet)> { - // Get all paths to the binaries from the scripts in the bin directory. - let locally_exposed = bin_dir.files().await?; - let executable_paths = futures::future::join_all(locally_exposed.iter().map(|path| { - let path = path.clone(); - async move { - extract_executable_from_script(&path) - .await - .ok() - .map(|exec| (path, exec)) - } - })) - .await - .into_iter() - .flatten() - .collect_vec(); - - // Filter out all binaries that are related to the environment - let related_exposed = executable_paths - .into_iter() - .filter(|(_, exec)| exec.starts_with(env_dir.path())) - .map(|(path, _)| path) - .collect_vec(); - - // Get all related expose scripts not required by the environment manifest - let to_remove = related_exposed - .iter() - .filter(|path| { - !exposed - .iter() - .any(|mapping| executable_from_path(path) == mapping.exposed_name().to_string()) - }) - .cloned() - .collect::>(); - - // Get all required exposed binaries that are not yet exposed - let to_add = exposed - .iter() - .filter_map(|mapping| { - if related_exposed - .iter() - .map(|path| executable_from_path(path)) - .any(|exec| exec == mapping.exposed_name().to_string()) - { - None - } else { - Some(mapping.exposed_name().clone()) - } - }) - .collect::>(); - - Ok((to_remove, to_add)) -} - /// Checks if the manifest is in sync with the locally installed environment and binaries. /// Returns `true` if the environment is in sync, `false` otherwise. pub(crate) async fn environment_specs_in_sync( @@ -171,6 +108,7 @@ mod tests { use crate::global::EnvRoot; use fs_err::tokio as tokio_fs; use rattler_conda_types::ParseStrictness; + use std::path::PathBuf; #[tokio::test] async fn test_environment_specs_in_sync() { @@ -206,89 +144,4 @@ mod tests { .unwrap(); assert!(result); } - - #[tokio::test] - async fn test_get_expose_scripts_sync_status() { - let tmp_home_dir = tempfile::tempdir().unwrap(); - let tmp_home_dir_path = tmp_home_dir.path().to_path_buf(); - let env_root = EnvRoot::new(tmp_home_dir_path.clone()).unwrap(); - let env_name = EnvironmentName::from_str("test").unwrap(); - let env_dir = EnvDir::from_env_root(env_root, &env_name).await.unwrap(); - let bin_dir = BinDir::new(tmp_home_dir_path.clone()).unwrap(); - - // Test empty - let exposed = IndexSet::new(); - let (to_remove, to_add) = get_expose_scripts_sync_status(&bin_dir, &env_dir, &exposed) - .await - .unwrap(); - assert!(to_remove.is_empty()); - assert!(to_add.is_empty()); - - // Test with exposed - let mut exposed = IndexSet::new(); - exposed.insert(Mapping::new( - ExposedName::from_str("test").unwrap(), - "test".to_string(), - )); - let (to_remove, to_add) = get_expose_scripts_sync_status(&bin_dir, &env_dir, &exposed) - .await - .unwrap(); - assert!(to_remove.is_empty()); - assert_eq!(to_add.len(), 1); - - // Add a script to the bin directory - let script_path = if cfg!(windows) { - bin_dir.path().join("test.bat") - } else { - bin_dir.path().join("test") - }; - - #[cfg(windows)] - { - let script = format!( - r#" - @"{}" %* - "#, - env_dir - .path() - .join("bin") - .join("test.exe") - .to_string_lossy() - ); - tokio_fs::write(&script_path, script).await.unwrap(); - } - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - - let script = format!( - r#"#!/bin/sh - "{}" "$@" - "#, - env_dir.path().join("bin").join("test").to_string_lossy() - ); - tokio_fs::write(&script_path, script).await.unwrap(); - // Set the file permissions to make it executable - let metadata = tokio_fs::metadata(&script_path).await.unwrap(); - let mut permissions = metadata.permissions(); - permissions.set_mode(0o755); // rwxr-xr-x - tokio_fs::set_permissions(&script_path, permissions) - .await - .unwrap(); - }; - - let (to_remove, to_add) = get_expose_scripts_sync_status(&bin_dir, &env_dir, &exposed) - .await - .unwrap(); - assert!(to_remove.is_empty()); - assert!(to_add.is_empty()); - - // Test to_remove - let (to_remove, to_add) = - get_expose_scripts_sync_status(&bin_dir, &env_dir, &IndexSet::new()) - .await - .unwrap(); - assert_eq!(to_remove.len(), 1); - assert!(to_add.is_empty()); - } } diff --git a/src/global/project/manifest.rs b/src/global/project/manifest.rs index e10351feb..887e92996 100644 --- a/src/global/project/manifest.rs +++ b/src/global/project/manifest.rs @@ -81,7 +81,7 @@ impl Manifest { // Update self.parsed if self.parsed.envs.get(env_name).is_some() { - miette::bail!("Environment {env_name} already exists."); + miette::bail!("Environment {} already exists.", env_name.fancy_display()); } self.parsed.envs.insert( env_name.clone(), @@ -96,7 +96,10 @@ impl Manifest { channels_array.push(channel.as_str()); } - tracing::debug!("Added environment {} to toml document", env_name); + tracing::debug!( + "Added environment {} to toml document", + env_name.fancy_display() + ); Ok(()) } @@ -135,15 +138,13 @@ impl Manifest { }; let spec = PixiSpec::from_nameless_matchspec(spec, channel_config); - if !self.parsed.envs.contains_key(env_name) { - miette::bail!("Environment {} doesn't exist", env_name.fancy_display()); - } - // Update self.parsed self.parsed .envs .get_mut(env_name) - .ok_or_else(|| miette::miette!("This should be impossible"))? + .ok_or_else(|| { + miette::miette!("Environment {} doesn't exist.", env_name.fancy_display()) + })? .dependencies .insert(name.clone(), spec.clone()); @@ -158,7 +159,46 @@ impl Manifest { "Added dependency {}={} to toml document for environment {}", name.as_normalized(), spec.to_toml_value().to_string(), - env_name + env_name.fancy_display() + ); + Ok(()) + } + + /// Removes a dependency from the manifest + pub fn remove_dependency( + &mut self, + env_name: &EnvironmentName, + spec: &MatchSpec, + ) -> miette::Result<()> { + // Determine the name of the package to add + let (Some(name), _spec) = spec.clone().into_nameless() else { + miette::bail!("pixi does not support wildcard dependencies") + }; + + // Update self.parsed + self.parsed + .envs + .get_mut(env_name) + .ok_or_else(|| { + miette::miette!("Environment {} doesn't exist.", env_name.fancy_display()) + })? + .dependencies + .swap_remove(&name) + .ok_or(miette::miette!( + "Dependency {} not found in {}", + console::style(name.as_normalized()).green(), + env_name.fancy_display() + ))?; + + // Update self.document + self.document + .get_or_insert_nested_table(&format!("envs.{env_name}.dependencies"))? + .remove(name.as_normalized()); + + tracing::debug!( + "Removed dependency {} to toml document for environment {}", + console::style(name.as_normalized()).green(), + env_name.fancy_display() ); Ok(()) } @@ -178,7 +218,9 @@ impl Manifest { self.parsed .envs .get_mut(env_name) - .ok_or_else(|| miette::miette!("Can't find {} yet", env_name))? + .ok_or_else(|| { + miette::miette!("Can't find environment {} yet", env_name.fancy_display()) + })? .platform .replace(platform); @@ -878,4 +920,41 @@ mod tests { channels.into_iter().map(From::from).collect(); assert_eq!(actual_channels, expected_channels); } + + #[test] + fn test_remove_dependency() { + let env_name = EnvironmentName::from_str("test-env").unwrap(); + let match_spec = MatchSpec::from_str("pytest", ParseStrictness::Strict).unwrap(); + + let mut manifest = Manifest::from_str( + Path::new("global.toml"), + r#" +[envs.test-env] +channels = ["test-channel"] +dependencies = { "python" = "*", pytest = "*"} +"#, + ) + .unwrap(); + + // Remove dependency + manifest.remove_dependency(&env_name, &match_spec).unwrap(); + + // Check document + assert!(!manifest + .document + .to_string() + .contains(match_spec.name.clone().unwrap().as_normalized())); + + // Check parsed + let actual_value = manifest + .parsed + .envs + .get(&env_name) + .unwrap() + .dependencies + .get(&match_spec.name.unwrap()); + assert!(actual_value.is_none()); + + assert_snapshot!(manifest.document.to_string()); + } } diff --git a/src/global/project/mod.rs b/src/global/project/mod.rs index 14bfbe8d5..8db027646 100644 --- a/src/global/project/mod.rs +++ b/src/global/project/mod.rs @@ -1,11 +1,11 @@ use super::{extract_executable_from_script, BinDir, EnvRoot, StateChange, StateChanges}; -use crate::global::common::{channel_url_to_prioritized_channel, find_package_records}; +use crate::global::common::{ + channel_url_to_prioritized_channel, find_package_records, get_expose_scripts_sync_status, +}; use crate::global::install::{ create_activation_script, create_executable_scripts, script_exec_mapping, }; -use crate::global::project::environment::{ - environment_specs_in_sync, get_expose_scripts_sync_status, -}; +use crate::global::project::environment::environment_specs_in_sync; use crate::repodata::Repodata; use crate::rlimit::try_increase_rlimit_to_sensible; use crate::{ @@ -396,13 +396,17 @@ impl Project { self.manifest.parsed.envs.get(name) } + /// Returns the EnvDir with the environment name. + pub(crate) async fn environment_dir(&self, name: &EnvironmentName) -> miette::Result { + EnvDir::from_env_root(self.env_root.clone(), name).await + } + /// Returns the prefix of the environment with the given name. pub(crate) async fn environment_prefix( &self, env_name: &EnvironmentName, ) -> miette::Result { - let env_dir = EnvDir::from_env_root(self.env_root.clone(), env_name).await?; - Ok(Prefix::new(env_dir.path())) + Ok(Prefix::new(self.environment_dir(env_name).await?.path())) } /// Create an authenticated reqwest client for this project diff --git a/src/global/project/snapshots/pixi__global__project__manifest__tests__remove_dependency.snap b/src/global/project/snapshots/pixi__global__project__manifest__tests__remove_dependency.snap new file mode 100644 index 000000000..dc030881b --- /dev/null +++ b/src/global/project/snapshots/pixi__global__project__manifest__tests__remove_dependency.snap @@ -0,0 +1,7 @@ +--- +source: src/global/project/manifest.rs +expression: manifest.document.to_string() +--- +[envs.test-env] +channels = ["test-channel"] +dependencies = { "python" = "*"} diff --git a/tests/integration/test_global.py b/tests/integration/test_global.py index fed838326..45dd66119 100644 --- a/tests/integration/test_global.py +++ b/tests/integration/test_global.py @@ -1018,3 +1018,49 @@ def test_add(pixi: Path, tmp_path: Path, dummy_channel_1: str) -> None: # Make sure it now exposes the binary dummy_b = tmp_path / "bin" / exec_extension("dummy-b") assert dummy_b.is_file() + + +def test_remove_dependency(pixi: Path, tmp_path: Path, dummy_channel_1: str) -> None: + env = {"PIXI_HOME": str(tmp_path)} + + verify_cli_command( + [ + pixi, + "global", + "install", + "--channel", + dummy_channel_1, + "--environment", + "my-env", + "dummy-a", + "dummy-b", + ], + env=env, + ) + dummy_a = tmp_path / "bin" / exec_extension("dummy-a") + dummy_b = tmp_path / "bin" / exec_extension("dummy-b") + assert dummy_a.is_file() + assert dummy_b.is_file() + + # Remove dummy-a + verify_cli_command( + [pixi, "global", "remove", "--environment", "my-env", "dummy-a"], + env=env, + ) + assert not dummy_a.is_file() + + # Remove non-existing package + verify_cli_command( + [pixi, "global", "remove", "--environment", "my-env", "dummy-a"], + ExitCode.FAILURE, + env=env, + stderr_contains=["Dependency", "dummy-a", "not", "my-env"], + ) + + # Remove package from non-existing environment + verify_cli_command( + [pixi, "global", "remove", "--environment", "dummy-a", "dummy-a"], + ExitCode.FAILURE, + env=env, + stderr_contains="Environment dummy-a doesn't exist", + )