From ebbeb29fe018ce4dd13709db1cce2de9d9593f16 Mon Sep 17 00:00:00 2001 From: Olivier Lacroix Date: Tue, 21 May 2024 09:04:22 +0100 Subject: [PATCH] feat: Align `remove` arguments with `add` (#1406) --- src/cli/add.rs | 310 ++++++++++++++---------------- src/cli/global/common.rs | 19 +- src/cli/global/install.rs | 3 +- src/cli/global/remove.rs | 3 +- src/cli/global/upgrade.rs | 5 +- src/cli/has_specs.rs | 37 ++++ src/cli/init.rs | 34 ++-- src/cli/mod.rs | 1 + src/cli/remove.rs | 162 ++++------------ src/project/manifest/mod.rs | 217 +++++++++++---------- src/project/manifest/pyproject.rs | 6 +- tests/add_tests.rs | 1 + tests/common/builders.rs | 114 +++++------ tests/common/mod.rs | 43 +---- tests/install_tests.rs | 2 +- tests/solve_group_tests.rs | 1 + 16 files changed, 430 insertions(+), 528 deletions(-) create mode 100644 src/cli/has_specs.rs diff --git a/src/cli/add.rs b/src/cli/add.rs index 946e0886c..25ba7800e 100644 --- a/src/cli/add.rs +++ b/src/cli/add.rs @@ -5,14 +5,15 @@ use crate::{ FeatureName, }; use clap::Parser; +use indexmap::IndexMap; use itertools::{Either, Itertools}; use crate::project::grouped_environment::GroupedEnvironment; use miette::{IntoDiagnostic, WrapErr}; use rattler_conda_types::{ version_spec::{LogicalOperator, RangeOperator}, - Channel, MatchSpec, NamelessMatchSpec, PackageName, ParseStrictness, Platform, Version, - VersionBumpType, VersionSpec, + Channel, MatchSpec, NamelessMatchSpec, PackageName, Platform, Version, VersionBumpType, + VersionSpec, }; use rattler_repodata_gateway::{Gateway, RepoData}; use rattler_solve::{resolvo, ChannelPriority, RepoDataIter, SolverImpl}; @@ -22,43 +23,57 @@ use std::{ path::PathBuf, }; +use super::has_specs::HasSpecs; + /// Adds dependencies to the project +/// +/// The dependencies should be defined as MatchSpec for conda package, or a PyPI requirement +/// for the --pypi dependencies. If no specific version is provided, the latest version +/// compatible with your project will be chosen automatically or a * will be used. +/// +/// Example usage: +/// +/// - `pixi add python=3.9`: This will select the latest minor version that complies with 3.9.*, i.e., +/// python version 3.9.0, 3.9.1, 3.9.2, etc. +/// - `pixi add python`: In absence of a specified version, the latest version will be chosen. +/// For instance, this could resolve to python version 3.11.3.* at the time of writing. +/// +/// Adding multiple dependencies at once is also supported: +/// - `pixi add python pytest`: This will add both `python` and `pytest` to the project's dependencies. +/// +/// The `--platform` and `--build/--host` flags make the dependency target specific. +/// - `pixi add python --platform linux-64 --platform osx-arm64`: Will add the latest version of python for linux-64 and osx-arm64 platforms. +/// - `pixi add python --build`: Will add the latest version of python for as a build dependency. +/// +/// Mixing `--platform` and `--build`/`--host` flags is supported +/// +/// The `--pypi` option will add the package as a pypi dependency. This can not be mixed with the conda dependencies +/// - `pixi add --pypi boto3` +/// - `pixi add --pypi "boto3==version" +/// +/// If the project manifest is a `pyproject.toml`, adding a pypi dependency will add it to the native pyproject `project.dependencies` array +/// or to the native `project.optional-dependencies` table if a feature is specified: +/// - `pixi add --pypi boto3` will add `boto3` to the `project.dependencies` array +/// - `pixi add --pypi boto3 --feature aws` will add `boto3` to the `project.dependencies.aws` array +/// These dependencies will then be read by pixi as if they had been added to the pixi `pypi-dependencies` tables of the default or of a named feature. +/// #[derive(Parser, Debug, Default)] -#[clap(arg_required_else_help = true)] +#[clap(arg_required_else_help = true, verbatim_doc_comment)] pub struct Args { - /// Specify the dependencies you wish to add to the project. - /// - /// The dependencies should be defined as MatchSpec for conda package, or a PyPI requirement - /// for the --pypi dependencies. If no specific version is provided, the latest version - /// compatible with your project will be chosen automatically or a * will be used. - /// - /// Example usage: - /// - /// - `pixi add python=3.9`: This will select the latest minor version that complies with 3.9.*, i.e., - /// python version 3.9.0, 3.9.1, 3.9.2, etc. - /// - `pixi add python`: In absence of a specified version, the latest version will be chosen. - /// For instance, this could resolve to python version 3.11.3.* at the time of writing. - /// - /// Adding multiple dependencies at once is also supported: - /// - `pixi add python pytest`: This will add both `python` and `pytest` to the project's dependencies. - /// - /// The `--platform` and `--build/--host` flags make the dependency target specific. - /// - `pixi add python --platform linux-64 --platform osx-arm64`: Will add the latest version of python for linux-64 and osx-arm64 platforms. - /// - `pixi add python --build`: Will add the latest version of python for as a build dependency. - /// - /// Mixing `--platform` and `--build`/`--host` flags is supported - /// - /// The `--pypi` option will add the package as a pypi dependency. This can not be mixed with the conda dependencies - /// - `pixi add --pypi boto3` - /// - `pixi add --pypi "boto3==version" - /// - /// If the project manifest is a `pyproject.toml`, adding a pypi dependency will add it to the native pyproject `project.dependencies` array - /// or to the native `project.optional-dependencies` table if a feature is specified: - /// - `pixi add --pypi boto3` will add `boto3` to the `project.dependencies` array - /// - `pixi add --pypi boto3 --feature aws` will add `boto3` to the `project.dependencies.aws` array - /// These dependencies will then be read by pixi as if they had been added to the pixi `pypi-dependencies` tables of the default or of a named feature. - /// - #[arg(required = true, verbatim_doc_comment)] + #[clap(flatten)] + pub dependency_config: DependencyConfig, + + #[clap(flatten)] + pub config: ConfigCli, + + /// Whether the pypi requirement should be editable + #[arg(long, requires = "pypi")] + pub editable: bool, +} +#[derive(Parser, Debug, Default)] +pub struct DependencyConfig { + /// The dependencies as names, conda MatchSpecs or PyPi requirements + #[arg(required = true)] pub specs: Vec, /// The path to 'pixi.toml' or 'pyproject.toml' @@ -66,11 +81,11 @@ pub struct Args { pub manifest_path: Option, /// The specified dependencies are host dependencies. Conflicts with `build` and `pypi` - #[arg(long, conflicts_with = "build")] + #[arg(long, conflicts_with_all = ["build", "pypi"])] pub host: bool, /// The specified dependencies are build dependencies. Conflicts with `host` and `pypi` - #[arg(long, conflicts_with = "host")] + #[arg(long, conflicts_with_all = ["host", "pypi"])] pub build: bool, /// The specified dependencies are pypi dependencies. Conflicts with `host` and `build` @@ -81,45 +96,85 @@ pub struct Args { #[clap(long, conflicts_with = "no_install")] pub no_lockfile_update: bool, - /// Don't install the package to the environment, only add the package to the lock-file. + /// Don't modify the environment, only modify the lock-file. #[arg(long)] pub no_install: bool, - /// The platform(s) for which the dependency should be added + /// The platform(s) for which the dependency should be modified #[arg(long, short)] pub platform: Vec, - /// The feature for which the dependency should be added + /// The feature for which the dependency should be modified #[arg(long, short)] pub feature: Option, - - #[clap(flatten)] - pub config: ConfigCli, - - /// Whether the pypi requirement should be editable - #[arg(long, requires = "pypi")] - pub editable: bool, } -impl DependencyType { - pub fn from_args(args: &Args) -> Self { - if args.pypi { - Self::PypiDependency - } else if args.host { +impl DependencyConfig { + pub fn dependency_type(&self) -> DependencyType { + if self.pypi { + DependencyType::PypiDependency + } else if self.host { DependencyType::CondaDependency(SpecType::Host) - } else if args.build { + } else if self.build { DependencyType::CondaDependency(SpecType::Build) } else { DependencyType::CondaDependency(SpecType::Run) } } + pub fn lock_file_usage(&self) -> LockFileUsage { + if self.no_lockfile_update { + LockFileUsage::Frozen + } else { + LockFileUsage::Update + } + } + pub fn feature_name(&self) -> FeatureName { + self.feature + .clone() + .map_or(FeatureName::Default, FeatureName::Named) + } + pub fn display_success(&self, operation: &str) { + for package in self.specs.clone() { + eprintln!( + "{}{operation} {}", + console::style(console::Emoji("✔ ", "")).green(), + console::style(package).bold(), + ); + } + + // Print if it is something different from host and dep + let dependency_type = self.dependency_type(); + if !matches!( + dependency_type, + DependencyType::CondaDependency(SpecType::Run) + ) { + eprintln!( + "{operation} these as {}.", + console::style(dependency_type.name()).bold() + ); + } + + // Print something if we've modified for platforms + if !self.platform.is_empty() { + eprintln!( + "{operation} these only for platform(s): {}", + console::style(self.platform.iter().join(", ")).bold() + ) + } + } +} + +impl HasSpecs for DependencyConfig { + fn packages(&self) -> Vec<&str> { + self.specs.iter().map(AsRef::as_ref).collect() + } } pub async fn execute(args: Args) -> miette::Result<()> { - let mut project = Project::load_or_else_discover(args.manifest_path.as_deref())? - .with_cli_config(args.config.clone()); - let dependency_type = DependencyType::from_args(&args); - let spec_platforms = &args.platform; + let (args, config, editable) = (args.dependency_config, args.config, args.editable); + let mut project = + Project::load_or_else_discover(args.manifest_path.as_deref())?.with_cli_config(config); + let dependency_type = args.dependency_type(); // Sanity check of prefix location verify_prefix_location_unchanged(project.default_environment().dir().as_path()).await?; @@ -127,82 +182,38 @@ pub async fn execute(args: Args) -> miette::Result<()> { // Add the platform if it is not already present project .manifest - .add_platforms(spec_platforms.iter(), &FeatureName::Default)?; - - let feature_name = args - .feature - .map_or(FeatureName::Default, FeatureName::Named); + .add_platforms(args.platform.iter(), &FeatureName::Default)?; match dependency_type { DependencyType::CondaDependency(spec_type) => { - let specs = args - .specs - .clone() - .into_iter() - .map(|s| MatchSpec::from_str(&s, ParseStrictness::Strict)) - .collect::, _>>() - .into_diagnostic()?; + let specs = args.specs()?; add_conda_specs_to_project( &mut project, - &feature_name, + &args.feature_name(), specs, spec_type, args.no_install, - args.no_lockfile_update, - spec_platforms, + args.lock_file_usage(), + &args.platform, ) .await } DependencyType::PypiDependency => { - // Parse specs as pep508_rs requirements - let pep508_requirements = args - .specs - .clone() - .into_iter() - .map(|input| { - pep508_rs::Requirement::parse(input.as_ref(), project.root()).into_diagnostic() - }) - .collect::>>()?; - + let specs = args.pypi_deps(&project)?.values().cloned().collect_vec(); add_pypi_requirements_to_project( &mut project, - &feature_name, - pep508_requirements, - spec_platforms, - args.no_lockfile_update, + &args.feature_name(), + specs, + &args.platform, + args.lock_file_usage(), args.no_install, - Some(args.editable), + Some(editable), ) .await } }?; - for package in args.specs { - eprintln!( - "{}Added {}", - console::style(console::Emoji("✔ ", "")).green(), - console::style(package).bold(), - ); - } - - // Print if it is something different from host and dep - if !matches!( - dependency_type, - DependencyType::CondaDependency(SpecType::Run) - ) { - eprintln!( - "Added these as {}.", - console::style(dependency_type.name()).bold() - ); - } - - // Print something if we've added for platforms - if !args.platform.is_empty() { - eprintln!( - "Added these only for platform(s): {}", - console::style(args.platform.iter().join(", ")).bold() - ) - } + args.display_success("Added"); Project::warn_on_discovered_from_env(args.manifest_path.as_deref()); Ok(()) @@ -213,33 +224,17 @@ pub async fn add_pypi_requirements_to_project( feature_name: &FeatureName, requirements: Vec, platforms: &[Platform], - no_update_lockfile: bool, + lock_file_usage: LockFileUsage, no_install: bool, editable: Option, ) -> miette::Result<()> { for requirement in &requirements { // TODO: Get best version // Add the dependency to the project - if platforms.is_empty() { - project - .manifest - .add_pypi_dependency(requirement, None, feature_name, editable)?; - } else { - for platform in platforms.iter() { - project.manifest.add_pypi_dependency( - requirement, - Some(*platform), - feature_name, - editable, - )?; - } - } + project + .manifest + .add_pypi_dependency(requirement, platforms, feature_name, editable)?; } - let lock_file_usage = if no_update_lockfile { - LockFileUsage::Frozen - } else { - LockFileUsage::Update - }; get_up_to_date_prefix(&project.default_environment(), lock_file_usage, no_install).await?; @@ -251,21 +246,12 @@ pub async fn add_pypi_requirements_to_project( pub async fn add_conda_specs_to_project( project: &mut Project, feature_name: &FeatureName, - specs: Vec, + specs: IndexMap, spec_type: SpecType, no_install: bool, - no_update_lockfile: bool, + lock_file_usage: LockFileUsage, specs_platforms: &[Platform], ) -> miette::Result<()> { - // Split the specs into package name and version specifier - let new_specs = specs - .into_iter() - .map(|spec| match &spec.name { - Some(name) => Ok((name.clone(), spec.into())), - None => Err(miette::miette!("missing package name for spec '{spec}'")), - }) - .collect::>>()?; - // Determine the best version per platform let mut package_versions = HashMap::>::new(); @@ -295,7 +281,7 @@ pub async fn add_conda_specs_to_project( // Solve the environment with the new specs added let solved_versions = match determine_best_version( &grouped_environment, - &new_specs, + &specs, spec_type, platform, grouped_environment.channels(), @@ -307,7 +293,7 @@ pub async fn add_conda_specs_to_project( Err(err) => { return Err(err).wrap_err_with(|| miette::miette!( "could not determine any available versions for {} on {platform}. Either the package could not be found or version constraints on other dependencies result in a conflict.", - new_specs.keys().map(|s| s.as_source()).join(", ") + specs.keys().map(|s| s.as_source()).join(", ") )); } }; @@ -320,9 +306,9 @@ pub async fn add_conda_specs_to_project( } // Update the specs passed on the command line with the best available versions. - for (name, spec) in new_specs { + for (name, spec) in specs { let updated_spec = if spec.version.is_none() { - let mut updated_spec = spec.clone(); + let mut updated_spec = NamelessMatchSpec::from(spec.clone()); if let Some(versions_seen) = package_versions.get(&name).cloned() { updated_spec.version = determine_version_constraint(&versions_seen); } else { @@ -332,28 +318,15 @@ pub async fn add_conda_specs_to_project( } updated_spec } else { - spec + spec.into() }; let spec = MatchSpec::from_nameless(updated_spec, Some(name)); // Add the dependency to the project - if specs_platforms.is_empty() { - project - .manifest - .add_dependency(&spec, spec_type, None, feature_name)?; - } else { - for platform in specs_platforms.iter() { - project - .manifest - .add_dependency(&spec, spec_type, Some(*platform), feature_name)?; - } - } + project + .manifest + .add_dependency(&spec, spec_type, specs_platforms, feature_name)?; } - let lock_file_usage = if no_update_lockfile { - LockFileUsage::Frozen - } else { - LockFileUsage::Update - }; // Update the prefix get_up_to_date_prefix(&project.default_environment(), lock_file_usage, no_install).await?; @@ -426,7 +399,7 @@ async fn determine_latest_versions( /// Given several specs determines the highest installable version for them. pub async fn determine_best_version<'p>( environment: &GroupedEnvironment<'p>, - new_specs: &HashMap, + new_specs: &IndexMap, new_specs_type: SpecType, platform: Platform, channels: impl IntoIterator, @@ -439,7 +412,8 @@ pub async fn determine_best_version<'p>( if spec_type == new_specs_type { for (new_name, new_spec) in new_specs.iter() { deps.remove(new_name); // Remove any existing specs - deps.insert(new_name.clone(), new_spec.clone()); // Add the new specs + deps.insert(new_name.clone(), NamelessMatchSpec::from(new_spec.clone())); + // Add the new specs } } deps diff --git a/src/cli/global/common.rs b/src/cli/global/common.rs index f4912145c..fc07a8bf8 100644 --- a/src/cli/global/common.rs +++ b/src/cli/global/common.rs @@ -3,8 +3,7 @@ use std::path::PathBuf; use indexmap::IndexMap; use miette::IntoDiagnostic; use rattler_conda_types::{ - Channel, ChannelConfig, MatchSpec, PackageName, ParseStrictness, Platform, PrefixRecord, - RepoDataRecord, + Channel, ChannelConfig, MatchSpec, PackageName, Platform, PrefixRecord, RepoDataRecord, }; use rattler_repodata_gateway::sparse::SparseRepoData; use rattler_solve::{resolvo, ChannelPriority, SolverImpl, SolverTask}; @@ -17,22 +16,6 @@ use crate::{ utils::reqwest::build_reqwest_clients, }; -/// A trait to facilitate extraction of packages data from arguments -pub(super) trait HasSpecs { - /// returns packages passed as arguments to the command - fn packages(&self) -> Vec<&str>; - - fn specs(&self) -> miette::Result> { - let mut map = IndexMap::with_capacity(self.packages().len()); - for package in self.packages() { - let spec = MatchSpec::from_str(package, ParseStrictness::Strict).into_diagnostic()?; - let name = package_name(&spec)?; - map.insert(name, spec); - } - - Ok(map) - } -} /// Global binaries directory, default to `$HOME/.pixi/bin` pub struct BinDir(pub PathBuf); diff --git a/src/cli/global/install.rs b/src/cli/global/install.rs index f66af19d1..e8c268611 100644 --- a/src/cli/global/install.rs +++ b/src/cli/global/install.rs @@ -2,6 +2,7 @@ use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::sync::Arc; +use crate::cli::has_specs::HasSpecs; use crate::config::{Config, ConfigCli}; use crate::install::execute_transaction; use crate::{config, prefix::Prefix, progress::await_in_progress}; @@ -20,7 +21,7 @@ use reqwest_middleware::ClientWithMiddleware; use super::common::{ channel_name_from_prefix, find_designated_package, get_client_and_sparse_repodata, - load_package_records, BinDir, BinEnvDir, HasSpecs, + load_package_records, BinDir, BinEnvDir, }; /// Installs the defined package in a global accessible location. diff --git a/src/cli/global/remove.rs b/src/cli/global/remove.rs index c1085b347..53a275499 100644 --- a/src/cli/global/remove.rs +++ b/src/cli/global/remove.rs @@ -6,9 +6,10 @@ use itertools::Itertools; use miette::IntoDiagnostic; use rattler_conda_types::PackageName; +use crate::cli::has_specs::HasSpecs; use crate::prefix::Prefix; -use super::common::{find_designated_package, BinDir, BinEnvDir, HasSpecs}; +use super::common::{find_designated_package, BinDir, BinEnvDir}; use super::install::{find_and_map_executable_scripts, BinScriptMapping}; /// Removes a package previously installed into a globally accessible location via `pixi global install`. diff --git a/src/cli/global/upgrade.rs b/src/cli/global/upgrade.rs index e1b9ed0af..7ba68c6e9 100644 --- a/src/cli/global/upgrade.rs +++ b/src/cli/global/upgrade.rs @@ -10,12 +10,11 @@ use miette::{IntoDiagnostic, Report}; use rattler_conda_types::{Channel, MatchSpec, PackageName, Platform}; use tokio::task::JoinSet; +use crate::cli::has_specs::HasSpecs; use crate::config::Config; use crate::progress::{global_multi_progress, long_running_progress_style}; -use super::common::{ - find_installed_package, get_client_and_sparse_repodata, load_package_records, HasSpecs, -}; +use super::common::{find_installed_package, get_client_and_sparse_repodata, load_package_records}; use super::install::globally_install_package; /// Upgrade specific package which is installed globally. diff --git a/src/cli/has_specs.rs b/src/cli/has_specs.rs new file mode 100644 index 000000000..f9928aab2 --- /dev/null +++ b/src/cli/has_specs.rs @@ -0,0 +1,37 @@ +use indexmap::IndexMap; +use miette::IntoDiagnostic; +use pep508_rs::Requirement; +use rattler_conda_types::{MatchSpec, PackageName, ParseStrictness}; + +use crate::{project::manifest::python::PyPiPackageName, Project}; + +/// A trait to facilitate extraction of packages data from arguments +pub(crate) trait HasSpecs { + /// returns packages passed as arguments to the command + fn packages(&self) -> Vec<&str>; + + fn specs(&self) -> miette::Result> { + let mut map = IndexMap::with_capacity(self.packages().len()); + for package in self.packages() { + let spec = MatchSpec::from_str(package, ParseStrictness::Strict).into_diagnostic()?; + let name = spec.name.clone().ok_or_else(|| { + miette::miette!("could not find package name in MatchSpec {}", spec) + })?; + map.insert(name, spec); + } + Ok(map) + } + + fn pypi_deps( + &self, + project: &Project, + ) -> miette::Result> { + let mut map = IndexMap::with_capacity(self.packages().len()); + for package in self.packages() { + let dep = Requirement::parse(package, project.root()).into_diagnostic()?; + let name = PyPiPackageName::from_normalized(dep.clone().name); + map.insert(name, dep); + } + Ok(map) + } +} diff --git a/src/cli/init.rs b/src/cli/init.rs index 39ff11f5f..c304184c0 100644 --- a/src/cli/init.rs +++ b/src/cli/init.rs @@ -179,26 +179,26 @@ pub async fn execute(args: Args) -> miette::Result<()> { &vec![], ); let mut project = Project::from_str(&pixi_manifest_path, &rv)?; + let platforms = platforms + .into_iter() + .map(|p| p.parse().into_diagnostic()) + .collect::, _>>()?; for spec in conda_deps { - for platform in platforms.iter() { - // TODO: fix serialization of channels in rattler_conda_types::MatchSpec - project.manifest.add_dependency( - &spec, - crate::SpecType::Run, - Some(platform.parse().into_diagnostic()?), - &FeatureName::default(), - )?; - } + // TODO: fix serialization of channels in rattler_conda_types::MatchSpec + project.manifest.add_dependency( + &spec, + crate::SpecType::Run, + &platforms, + &FeatureName::default(), + )?; } for requirement in pypi_deps { - for platform in platforms.iter() { - project.manifest.add_pypi_dependency( - &requirement, - Some(platform.parse().into_diagnostic()?), - &FeatureName::default(), - None, - )?; - } + project.manifest.add_pypi_dependency( + &requirement, + &platforms, + &FeatureName::default(), + None, + )?; } project.save()?; diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 318f2bd6d..0f48ae3cb 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -14,6 +14,7 @@ pub mod add; pub mod completion; pub mod config; pub mod global; +pub mod has_specs; pub mod info; pub mod init; pub mod install; diff --git a/src/cli/remove.rs b/src/cli/remove.rs index ca559eb7e..20692c334 100644 --- a/src/cli/remove.rs +++ b/src/cli/remove.rs @@ -1,156 +1,70 @@ -use std::path::PathBuf; -use std::str::FromStr; - use clap::Parser; -use miette::miette; -use pep508_rs::Requirement; -use rattler_conda_types::Platform; use crate::config::ConfigCli; -use crate::environment::{get_up_to_date_prefix, LockFileUsage}; -use crate::project::manifest::python::PyPiPackageName; -use crate::project::manifest::FeatureName; -use crate::{consts, project::SpecType, Project}; +use crate::environment::get_up_to_date_prefix; +use crate::DependencyType; +use crate::Project; + +use super::add::DependencyConfig; +use super::has_specs::HasSpecs; /// Removes dependencies from the project +/// +/// If the project manifest is a `pyproject.toml`, removing a pypi dependency with the `--pypi` flag will remove it from either +/// - the native pyproject `project.dependencies` array or, if a feature is specified, the native `project.optional-dependencies` table +/// - pixi `pypi-dependencies` tables of the default feature or, if a feature is specified, a named feature +/// #[derive(Debug, Default, Parser)] #[clap(arg_required_else_help = true)] pub struct Args { - /// Specify the dependencies you wish to remove from the project. - /// - /// If the project manifest is a `pyproject.toml`, removing a pypi dependency with the `--pypi` flag will remove it from either - /// - the native pyproject `project.dependencies` array or, if a feature is specified, the native `project.optional-dependencies` table - /// - pixi `pypi-dependencies` tables of the default feature or, if a feature is specified, a named feature - /// - #[arg(required = true, verbatim_doc_comment)] - pub deps: Vec, - - /// The path to 'pixi.toml' or 'pyproject.toml' - #[arg(long)] - pub manifest_path: Option, - - /// Whether dependency is a host dependency - #[arg(long, conflicts_with = "build")] - pub host: bool, - - /// Whether dependency is a build dependency - #[arg(long, conflicts_with = "host")] - pub build: bool, - - /// Whether the dependency is a pypi package - #[arg(long)] - pub pypi: bool, - - /// Don't install the environment, only remove the package from the lock-file and manifest. - #[arg(long)] - pub no_install: bool, - - /// The platform for which the dependency should be removed - #[arg(long, short)] - pub platform: Option, - - /// The feature for which the dependency should be removed - #[arg(long, short)] - pub feature: Option, + #[clap(flatten)] + pub dependency_config: DependencyConfig, #[clap(flatten)] pub config: ConfigCli, } -fn convert_pkg_name(deps: &[String]) -> miette::Result> -where - T: FromStr, -{ - deps.iter() - .map(|dep| { - T::from_str(dep) - .map_err(|_| miette!("Can't convert dependency name `{dep}` to package name")) - }) - .collect() -} - pub async fn execute(args: Args) -> miette::Result<()> { + let (args, config) = (args.dependency_config, args.config); let mut project = - Project::load_or_else_discover(args.manifest_path.as_deref())?.with_cli_config(args.config); - let deps = args.deps; - let spec_type = if args.host { - SpecType::Host - } else if args.build { - SpecType::Build - } else { - SpecType::Run - }; - - let section_name: String = if args.pypi { - consts::PYPI_DEPENDENCIES.to_string() - } else { - spec_type.name().to_string() - }; - let table_name = if let Some(p) = &args.platform { - format!("target.{}.{}", p.as_str(), section_name) - } else { - section_name - }; - let feature_name = args - .feature - .map_or(FeatureName::Default, FeatureName::Named); - - fn format_ok_message(pkg_name: &str, pkg_extras: &str, table_name: &str) -> String { - format!( - "Removed {} from [{}]", - console::style(format!("{pkg_name} {pkg_extras}")).bold(), - console::style(table_name).bold() - ) - } - let mut sucessful_output: Vec = Vec::with_capacity(deps.len()); - if args.pypi { - let all_pkg_name = convert_pkg_name::(&deps)?; - for dep in all_pkg_name.iter() { - let name = PyPiPackageName::from_normalized(dep.clone().name); - let (name, req) = - project - .manifest - .remove_pypi_dependency(&name, args.platform, &feature_name)?; - sucessful_output.push(format_ok_message( - name.as_source(), - &req.to_string(), - &table_name, - )); + Project::load_or_else_discover(args.manifest_path.as_deref())?.with_cli_config(config); + let dependency_type = args.dependency_type(); + + match dependency_type { + DependencyType::PypiDependency => { + for name in args.pypi_deps(&project)?.keys() { + project.manifest.remove_pypi_dependency( + name, + &args.platform, + &args.feature_name(), + )?; + } } - } else { - let all_pkg_name = convert_pkg_name::(&deps)?; - for dep in all_pkg_name.iter() { - // Get name or error on missing name - let name = dep - .clone() - .name - .ok_or_else(|| miette!("Can't remove dependency without a name: {}", dep))?; - let (name, req) = project.manifest.remove_dependency( - &name, - spec_type, - args.platform, - &feature_name, - )?; - sucessful_output.push(format_ok_message( - name.as_source(), - &req.to_string(), - &table_name, - )); + DependencyType::CondaDependency(spec_type) => { + for name in args.specs()?.keys() { + project.manifest.remove_dependency( + name, + spec_type, + &args.platform, + &args.feature_name(), + )?; + } } }; project.save()?; - eprintln!("{}", sucessful_output.join("\n")); // TODO: update all environments touched by this feature defined. // updating prefix after removing from toml get_up_to_date_prefix( &project.default_environment(), - LockFileUsage::Update, + args.lock_file_usage(), args.no_install, ) .await?; + args.display_success("Removed"); + Project::warn_on_discovered_from_env(args.manifest_path.as_deref()); Ok(()) } diff --git a/src/project/manifest/mod.rs b/src/project/manifest/mod.rs index ee06e21d7..2f10c3f92 100644 --- a/src/project/manifest/mod.rs +++ b/src/project/manifest/mod.rs @@ -310,20 +310,21 @@ impl Manifest { &mut self, spec: &MatchSpec, spec_type: SpecType, - platform: Option, + platforms: &[Platform], feature_name: &FeatureName, ) -> 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") }; - - // Add the dependency to the manifest - self.get_or_insert_target_mut(platform, Some(feature_name)) - .try_add_dependency(&name, &spec, spec_type)?; - // and to the TOML document - self.document - .add_dependency(&name, &spec, spec_type, platform, feature_name)?; + for platform in to_options(platforms) { + // Add the dependency to the manifest + self.get_or_insert_target_mut(platform, Some(feature_name)) + .try_add_dependency(&name, &spec, spec_type)?; + // and to the TOML document + self.document + .add_dependency(&name, &spec, spec_type, platform, feature_name)?; + } Ok(()) } @@ -331,16 +332,18 @@ impl Manifest { pub fn add_pypi_dependency( &mut self, requirement: &pep508_rs::Requirement, - platform: Option, + platforms: &[Platform], feature_name: &FeatureName, editable: Option, ) -> miette::Result<()> { - // Add the pypi dependency to the manifest - self.get_or_insert_target_mut(platform, Some(feature_name)) - .try_add_pypi_dependency(requirement, editable)?; - // and to the TOML document - self.document - .add_pypi_dependency(requirement, platform, feature_name)?; + for platform in to_options(platforms) { + // Add the pypi dependency to the manifest + self.get_or_insert_target_mut(platform, Some(feature_name)) + .try_add_pypi_dependency(requirement, editable)?; + // and to the TOML document + self.document + .add_pypi_dependency(requirement, platform, feature_name)?; + } Ok(()) } @@ -349,34 +352,36 @@ impl Manifest { &mut self, dep: &PackageName, spec_type: SpecType, - platform: Option, + platforms: &[Platform], feature_name: &FeatureName, - ) -> miette::Result<(PackageName, NamelessMatchSpec)> { - // Remove the dependency from the manifest - let res = self - .target_mut(platform, feature_name) - .remove_dependency(dep, spec_type); - // Remove the dependency from the TOML document - self.document - .remove_dependency(dep, spec_type, platform, feature_name)?; - res.into_diagnostic() + ) -> miette::Result<()> { + for platform in to_options(platforms) { + // Remove the dependency from the manifest + self.target_mut(platform, feature_name) + .remove_dependency(dep, spec_type)?; + // Remove the dependency from the TOML document + self.document + .remove_dependency(dep, spec_type, platform, feature_name)?; + } + Ok(()) } /// Removes a pypi dependency. pub fn remove_pypi_dependency( &mut self, dep: &PyPiPackageName, - platform: Option, + platforms: &[Platform], feature_name: &FeatureName, - ) -> miette::Result<(PyPiPackageName, PyPiRequirement)> { - // Remove the dependency from the manifest - let res = self - .target_mut(platform, feature_name) - .remove_pypi_dependency(dep); - // Remove the dependency from the TOML document - self.document - .remove_pypi_dependency(dep, platform, feature_name)?; - res.into_diagnostic() + ) -> miette::Result<()> { + for platform in to_options(platforms) { + // Remove the dependency from the manifest + self.target_mut(platform, feature_name) + .remove_pypi_dependency(dep)?; + // Remove the dependency from the TOML document + self.document + .remove_pypi_dependency(dep, platform, feature_name)?; + } + Ok(()) } /// Returns true if any of the features has pypi dependencies defined. @@ -630,6 +635,14 @@ impl Manifest { } } +/// Converts an array of Platforms to a non-empty Vec of Option +fn to_options(platforms: &[Platform]) -> Vec> { + match platforms.is_empty() { + true => vec![None], + false => platforms.iter().map(|p| Some(*p)).collect_vec(), + } +} + /// The environments in the project. #[derive(Debug, Clone, Default)] pub struct Environments { @@ -1390,46 +1403,50 @@ mod tests { file_contents: &str, name: &str, kind: SpecType, - platform: Option, + platforms: &[Platform], feature_name: &FeatureName, ) { let mut manifest = Manifest::from_str(Path::new("pixi.toml"), file_contents).unwrap(); // Initially the dependency should exist - assert!(manifest - .feature_mut(feature_name) - .unwrap() - .targets - .for_opt_target(platform.map(TargetSelector::Platform).as_ref()) - .unwrap() - .dependencies - .get(&kind) - .unwrap() - .get(name) - .is_some()); + for platform in to_options(platforms) { + assert!(manifest + .feature_mut(feature_name) + .unwrap() + .targets + .for_opt_target(platform.map(TargetSelector::Platform).as_ref()) + .unwrap() + .dependencies + .get(&kind) + .unwrap() + .get(name) + .is_some()); + } // Remove the dependency from the manifest manifest .remove_dependency( &PackageName::new_unchecked(name), kind, - platform, + platforms, feature_name, ) .unwrap(); // The dependency should no longer exist - assert!(manifest - .feature_mut(feature_name) - .unwrap() - .targets - .for_opt_target(platform.map(TargetSelector::Platform).as_ref()) - .unwrap() - .dependencies - .get(&kind) - .unwrap() - .get(name) - .is_none()); + for platform in to_options(platforms) { + assert!(manifest + .feature_mut(feature_name) + .unwrap() + .targets + .for_opt_target(platform.map(TargetSelector::Platform).as_ref()) + .unwrap() + .dependencies + .get(&kind) + .unwrap() + .get(name) + .is_none()); + } // Write the toml to string and verify the content assert_snapshot!( @@ -1441,7 +1458,7 @@ mod tests { fn test_remove_pypi( file_contents: &str, name: &str, - platform: Option, + platforms: &[Platform], feature_name: &FeatureName, ) { let mut manifest = Manifest::from_str(Path::new("pixi.toml"), file_contents).unwrap(); @@ -1449,35 +1466,39 @@ mod tests { let package_name = PyPiPackageName::from_str(name).unwrap(); // Initially the dependency should exist - assert!(manifest - .feature_mut(feature_name) - .unwrap() - .targets - .for_opt_target(platform.map(TargetSelector::Platform).as_ref()) - .unwrap() - .pypi_dependencies - .as_ref() - .unwrap() - .get(&package_name) - .is_some()); + for platform in to_options(platforms) { + assert!(manifest + .feature_mut(feature_name) + .unwrap() + .targets + .for_opt_target(platform.map(TargetSelector::Platform).as_ref()) + .unwrap() + .pypi_dependencies + .as_ref() + .unwrap() + .get(&package_name) + .is_some()); + } // Remove the dependency from the manifest manifest - .remove_pypi_dependency(&package_name, platform, feature_name) + .remove_pypi_dependency(&package_name, platforms, feature_name) .unwrap(); // The dependency should no longer exist - assert!(manifest - .feature_mut(feature_name) - .unwrap() - .targets - .for_opt_target(platform.map(TargetSelector::Platform).as_ref()) - .unwrap() - .pypi_dependencies - .as_ref() - .unwrap() - .get(&package_name) - .is_none()); + for platform in to_options(platforms) { + assert!(manifest + .feature_mut(feature_name) + .unwrap() + .targets + .for_opt_target(platform.map(TargetSelector::Platform).as_ref()) + .unwrap() + .pypi_dependencies + .as_ref() + .unwrap() + .get(&package_name) + .is_none()); + } // Write the toml to string and verify the content assert_snapshot!( @@ -1487,14 +1508,14 @@ mod tests { } #[rstest] - #[case::xpackage("xpackage", Some(Platform::Linux64), FeatureName::Default)] - #[case::jax("jax", Some(Platform::Win64), FeatureName::Default)] - #[case::requests("requests", None, FeatureName::Default)] - #[case::feature_dep("feature_dep", None, FeatureName::Named("test".to_string()))] - #[case::feature_target_dep("feature_target_dep", Some(Platform::Linux64), FeatureName::Named("test".to_string()))] + #[case::xpackage("xpackage", &[Platform::Linux64], FeatureName::Default)] + #[case::jax("jax", &[Platform::Win64], FeatureName::Default)] + #[case::requests("requests", &[], FeatureName::Default)] + #[case::feature_dep("feature_dep", &[], FeatureName::Named("test".to_string()))] + #[case::feature_target_dep("feature_target_dep", &[Platform::Linux64], FeatureName::Named("test".to_string()))] fn test_remove_pypi_dependencies( #[case] package_name: &str, - #[case] platform: Option, + #[case] platforms: &[Platform], #[case] feature_name: FeatureName, ) { let pixi_cfg = r#"[project] @@ -1523,7 +1544,7 @@ feature_dep = "*" [feature.test.target.linux-64.pypi-dependencies] feature_target_dep = "*" "#; - test_remove_pypi(pixi_cfg, package_name, platform, &feature_name); + test_remove_pypi(pixi_cfg, package_name, platforms, &feature_name); } #[test] @@ -1550,21 +1571,21 @@ feature_target_dep = "*" file_contents, "baz", SpecType::Build, - Some(Platform::Linux64), + &[Platform::Linux64], &FeatureName::Default, ); test_remove( file_contents, "bar", SpecType::Run, - Some(Platform::Win64), + &[Platform::Win64], &FeatureName::Default, ); test_remove( file_contents, "fooz", SpecType::Run, - None, + &[], &FeatureName::Default, ); } @@ -1595,7 +1616,7 @@ feature_target_dep = "*" .remove_dependency( &PackageName::new_unchecked("fooz"), SpecType::Run, - None, + &[], &FeatureName::Default, ) .unwrap(); @@ -2363,7 +2384,7 @@ bar = "*" .add_dependency( &MatchSpec::from_str(" baz >=1.2.3", Strict).unwrap(), SpecType::Run, - None, + &[], &FeatureName::Default, ) .unwrap(); @@ -2384,7 +2405,7 @@ bar = "*" .add_dependency( &MatchSpec::from_str(" bal >=2.3", Strict).unwrap(), SpecType::Run, - None, + &[], &FeatureName::Named("test".to_string()), ) .unwrap(); @@ -2408,7 +2429,7 @@ bar = "*" .add_dependency( &MatchSpec::from_str(" boef >=2.3", Strict).unwrap(), SpecType::Run, - Some(Platform::Linux64), + &[Platform::Linux64], &FeatureName::Named("extra".to_string()), ) .unwrap(); @@ -2433,7 +2454,7 @@ bar = "*" .add_dependency( &MatchSpec::from_str(" cmake >=2.3", ParseStrictness::Strict).unwrap(), SpecType::Build, - Some(Platform::Linux64), + &[Platform::Linux64], &FeatureName::Named("build".to_string()), ) .unwrap(); diff --git a/src/project/manifest/pyproject.rs b/src/project/manifest/pyproject.rs index 6c0115391..65bd291f3 100644 --- a/src/project/manifest/pyproject.rs +++ b/src/project/manifest/pyproject.rs @@ -409,7 +409,7 @@ mod tests { // Add numpy to pyproject let requirement = pep508_rs::Requirement::from_str("numpy>=3.12").unwrap(); manifest - .add_pypi_dependency(&requirement, None, &FeatureName::Default, None) + .add_pypi_dependency(&requirement, &[], &FeatureName::Default, None) .unwrap(); assert!(manifest @@ -428,7 +428,7 @@ mod tests { manifest .add_pypi_dependency( &requirement, - None, + &[], &FeatureName::Named("test".to_string()), None, ) @@ -456,7 +456,7 @@ mod tests { // Remove flask from pyproject let name = PyPiPackageName::from_str("flask").unwrap(); manifest - .remove_pypi_dependency(&name, None, &FeatureName::Default) + .remove_pypi_dependency(&name, &[], &FeatureName::Default) .unwrap(); assert!(manifest diff --git a/tests/add_tests.rs b/tests/add_tests.rs index fe6018615..8eca532fa 100644 --- a/tests/add_tests.rs +++ b/tests/add_tests.rs @@ -1,5 +1,6 @@ mod common; +use crate::common::builders::HasDependencyConfig; use crate::common::package_database::{Package, PackageDatabase}; use crate::common::LockFileExt; use crate::common::PixiControl; diff --git a/tests/common/builders.rs b/tests/common/builders.rs index b473af4e9..a455fc262 100644 --- a/tests/common/builders.rs +++ b/tests/common/builders.rs @@ -24,6 +24,7 @@ //! ``` use futures::FutureExt; +use pixi::cli::add::DependencyConfig; use pixi::cli::remove; use pixi::task::TaskName; use pixi::{ @@ -77,39 +78,50 @@ impl IntoFuture for InitBuilder { } } -/// Contains the arguments to pass to [`add::execute()`]. Call `.await` to call the CLI execute method -/// and await the result at the same time. -pub struct AddBuilder { - pub args: add::Args, -} +/// A trait used by AddBuilder and RemoveBuilder to set their inner DependencyConfig +pub trait HasDependencyConfig: Sized { + fn dependency_config(&mut self) -> &mut DependencyConfig; + + fn dependency_config_with_specs(specs: Vec<&str>, manifest_path: PathBuf) -> DependencyConfig { + DependencyConfig { + specs: specs.iter().map(|s| s.to_string()).collect(), + manifest_path: Some(manifest_path), + host: false, + build: false, + pypi: false, + platform: Default::default(), + feature: None, + no_install: true, + no_lockfile_update: false, + } + } -impl AddBuilder { - pub fn with_spec(mut self, spec: &str) -> Self { - self.args.specs.push(spec.to_string()); + fn with_spec(mut self, spec: &str) -> Self { + self.dependency_config().specs.push(spec.to_string()); self } /// Set as a host - pub fn set_type(mut self, t: DependencyType) -> Self { + fn set_type(mut self, t: DependencyType) -> Self { match t { DependencyType::CondaDependency(spec_type) => match spec_type { SpecType::Host => { - self.args.host = true; - self.args.build = false; + self.dependency_config().host = true; + self.dependency_config().build = false; } SpecType::Build => { - self.args.host = false; - self.args.build = true; + self.dependency_config().host = false; + self.dependency_config().build = true; } SpecType::Run => { - self.args.host = false; - self.args.build = false; + self.dependency_config().host = false; + self.dependency_config().build = false; } }, DependencyType::PypiDependency => { - self.args.host = false; - self.args.build = false; - self.args.pypi = true; + self.dependency_config().host = false; + self.dependency_config().build = false; + self.dependency_config().pypi = true; } } self @@ -117,29 +129,43 @@ impl AddBuilder { /// Set whether to also install the environment. By default, the environment is NOT /// installed to reduce test times. - pub fn with_install(mut self, install: bool) -> Self { - self.args.no_install = !install; + fn with_install(mut self, install: bool) -> Self { + self.dependency_config().no_install = !install; self } /// Skip updating lockfile, this will only check if it can add a dependencies. /// If it can add it will only add it to the manifest. Install will be skipped by default. - pub fn without_lockfile_update(mut self) -> Self { - self.args.no_lockfile_update = true; + fn without_lockfile_update(mut self) -> Self { + self.dependency_config().no_lockfile_update = true; self } - pub fn set_platforms(mut self, platforms: &[Platform]) -> Self { - self.args.platform.extend(platforms.iter()); + fn set_platforms(mut self, platforms: &[Platform]) -> Self { + self.dependency_config().platform.extend(platforms.iter()); self } +} +/// Contains the arguments to pass to [`add::execute()`]. Call `.await` to call the CLI execute method +/// and await the result at the same time. +pub struct AddBuilder { + pub args: add::Args, +} + +impl AddBuilder { pub fn set_editable(mut self, editable: bool) -> Self { self.args.editable = editable; self } } +impl HasDependencyConfig for AddBuilder { + fn dependency_config(&mut self) -> &mut DependencyConfig { + &mut self.args.dependency_config + } +} + impl IntoFuture for AddBuilder { type Output = miette::Result<()>; type IntoFuture = Pin + 'static>>; @@ -155,43 +181,9 @@ pub struct RemoveBuilder { pub args: remove::Args, } -impl RemoveBuilder { - pub fn with_spec(mut self, spec: &str) -> Self { - self.args.deps.push(spec.to_string()); - self - } - - /// Set whether to also install the environment. By default, the environment is NOT - /// installed to reduce test times. - pub fn with_install(mut self, install: bool) -> Self { - self.args.no_install = !install; - self - } - - /// Set as a host - pub fn set_type(mut self, t: DependencyType) -> Self { - match t { - DependencyType::CondaDependency(spec_type) => match spec_type { - SpecType::Host => { - self.args.host = true; - self.args.build = false; - } - SpecType::Build => { - self.args.host = false; - self.args.build = true; - } - SpecType::Run => { - self.args.host = false; - self.args.build = false; - } - }, - DependencyType::PypiDependency => { - self.args.host = false; - self.args.build = false; - self.args.pypi = true; - } - } - self +impl HasDependencyConfig for RemoveBuilder { + fn dependency_config(&mut self) -> &mut DependencyConfig { + &mut self.args.dependency_config } } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 78b02f172..5d787abab 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -36,7 +36,7 @@ use std::{ use tempfile::TempDir; use thiserror::Error; -use self::builders::RemoveBuilder; +use self::builders::{HasDependencyConfig, RemoveBuilder}; /// To control the pixi process pub struct PixiControl { @@ -227,21 +227,7 @@ impl PixiControl { /// Add a dependency to the project. Returns an [`AddBuilder`]. /// the command and await the result call `.await` on the return value. pub fn add(&self, spec: &str) -> AddBuilder { - AddBuilder { - args: add::Args { - manifest_path: Some(self.manifest_path()), - host: false, - specs: vec![spec.to_string()], - build: false, - no_install: true, - no_lockfile_update: false, - platform: Default::default(), - pypi: false, - feature: None, - config: Default::default(), - editable: false, - }, - } + self.add_multiple(vec![spec]) } /// Add dependencies to the project. Returns an [`AddBuilder`]. @@ -249,15 +235,10 @@ impl PixiControl { pub fn add_multiple(&self, specs: Vec<&str>) -> AddBuilder { AddBuilder { args: add::Args { - manifest_path: Some(self.manifest_path()), - host: false, - specs: specs.iter().map(|s| s.to_string()).collect(), - build: false, - no_install: true, - no_lockfile_update: false, - platform: Default::default(), - pypi: false, - feature: None, + dependency_config: AddBuilder::dependency_config_with_specs( + specs, + self.manifest_path(), + ), config: Default::default(), editable: false, }, @@ -268,15 +249,11 @@ impl PixiControl { pub fn remove(&self, spec: &str) -> RemoveBuilder { RemoveBuilder { args: remove::Args { - deps: vec![spec.to_string()], - manifest_path: Some(self.manifest_path()), - host: false, - build: false, - pypi: false, - platform: Default::default(), - feature: None, + dependency_config: AddBuilder::dependency_config_with_specs( + vec![spec], + self.manifest_path(), + ), config: Default::default(), - no_install: true, }, } } diff --git a/tests/install_tests.rs b/tests/install_tests.rs index b4c064472..5154bf688 100644 --- a/tests/install_tests.rs +++ b/tests/install_tests.rs @@ -3,7 +3,7 @@ mod common; use std::path::Path; use std::str::FromStr; -use crate::common::builders::string_from_iter; +use crate::common::builders::{string_from_iter, HasDependencyConfig}; use crate::common::package_database::{Package, PackageDatabase}; use common::{LockFileExt, PixiControl}; use pixi::cli::{run, LockFileUsageArgs}; diff --git a/tests/solve_group_tests.rs b/tests/solve_group_tests.rs index 2b8a09f85..05ad36bf8 100644 --- a/tests/solve_group_tests.rs +++ b/tests/solve_group_tests.rs @@ -1,6 +1,7 @@ use std::{collections::HashMap, str::FromStr}; use crate::common::{ + builders::HasDependencyConfig, package_database::{Package, PackageDatabase}, LockFileExt, PixiControl, };