From b2c8eb77fa7a19b205f2ff73a43386dd8bf3c83e Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Fri, 15 Aug 2025 10:50:40 +0200 Subject: [PATCH 1/2] fix: remove manual conflicts check for --frozen --locked and check with clap --- src/cli/add.rs | 2 +- src/cli/cli_config.rs | 12 +- src/cli/install.rs | 2 +- src/cli/list.rs | 2 +- src/cli/mod.rs | 134 ++---------------- src/cli/reinstall.rs | 2 +- src/cli/remove.rs | 2 +- src/cli/run.rs | 2 +- src/cli/shell.rs | 2 +- src/cli/shell_hook.rs | 2 +- src/cli/tree.rs | 2 +- src/cli/upgrade.rs | 2 +- .../workspace/export/conda_explicit_spec.rs | 2 +- tests/integration_rust/common/mod.rs | 2 +- tests/integration_rust/upgrade_tests.rs | 2 +- 15 files changed, 31 insertions(+), 141 deletions(-) diff --git a/src/cli/add.rs b/src/cli/add.rs index 1b268e9754..3eb2411e21 100644 --- a/src/cli/add.rs +++ b/src/cli/add.rs @@ -198,7 +198,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { pypi_deps, source_specs, prefix_update_config.no_install, - &lock_file_update_config.lock_file_usage()?, + &lock_file_update_config.lock_file_usage(), &dependency_config.feature, &dependency_config.platforms, args.editable, diff --git a/src/cli/cli_config.rs b/src/cli/cli_config.rs index 82491a1d92..487763c098 100644 --- a/src/cli/cli_config.rs +++ b/src/cli/cli_config.rs @@ -112,16 +112,12 @@ pub struct LockFileUpdateConfig { } impl LockFileUpdateConfig { - pub fn lock_file_usage(&self) -> miette::Result { - let usage: LockFileUsage = self - .lock_file_usage - .clone() - .try_into() - .map_err(|e: crate::cli::LockFileUsageError| miette::miette!(e))?; + pub fn lock_file_usage(&self) -> LockFileUsage { + let usage: LockFileUsage = self.lock_file_usage.clone().into(); if self.no_lockfile_update { - Ok(LockFileUsage::Frozen) + LockFileUsage::Frozen } else { - Ok(usage) + usage } } } diff --git a/src/cli/install.rs b/src/cli/install.rs index 752877c430..acd0ba4031 100644 --- a/src/cli/install.rs +++ b/src/cli/install.rs @@ -89,7 +89,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { &environments, UpdateMode::Revalidate, UpdateLockFileOptions { - lock_file_usage: args.lock_file_usage.try_into()?, + lock_file_usage: args.lock_file_usage.into(), no_install: false, max_concurrent_solves: workspace.config().max_concurrent_solves(), }, diff --git a/src/cli/list.rs b/src/cli/list.rs index 70a3e2744a..6121cb1d15 100644 --- a/src/cli/list.rs +++ b/src/cli/list.rs @@ -184,7 +184,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { let lock_file = workspace .update_lock_file(UpdateLockFileOptions { - lock_file_usage: args.lock_file_update_config.lock_file_usage()?, + lock_file_usage: args.lock_file_update_config.lock_file_usage(), no_install: false, max_concurrent_solves: workspace.config().max_concurrent_solves(), }) diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 623886f8d0..9bc5c5247f 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -198,11 +198,11 @@ pub struct LockFileUsageArgs { struct LockFileUsageArgsRaw { /// Install the environment as defined in the lockfile, doesn't update /// lockfile if it isn't up-to-date with the manifest file. - #[clap(long, env = "PIXI_FROZEN", help_heading = consts::CLAP_UPDATE_OPTIONS)] + #[clap(long, env = "PIXI_FROZEN", help_heading = consts::CLAP_UPDATE_OPTIONS, conflicts_with = "locked")] frozen: bool, /// Check if lockfile is up-to-date before installing the environment, /// aborts when lockfile isn't up-to-date with the manifest file. - #[clap(long, env = "PIXI_LOCKED", help_heading = consts::CLAP_UPDATE_OPTIONS)] + #[clap(long, env = "PIXI_LOCKED", help_heading = consts::CLAP_UPDATE_OPTIONS, conflicts_with = "frozen")] locked: bool, } @@ -216,15 +216,10 @@ impl LockFileUsageArgs { } } -// Automatic validation when converting from raw args -impl TryFrom for LockFileUsageArgs { - type Error = LockFileUsageError; - - fn try_from(raw: LockFileUsageArgsRaw) -> Result { - if raw.frozen && raw.locked { - return Err(LockFileUsageError::FrozenAndLocked); - } - Ok(LockFileUsageArgs { inner: raw }) +// Automatic conversion from raw args (conflicts handled by clap) +impl From for LockFileUsageArgs { + fn from(raw: LockFileUsageArgsRaw) -> Self { + LockFileUsageArgs { inner: raw } } } @@ -232,9 +227,7 @@ impl TryFrom for LockFileUsageArgs { impl clap::FromArgMatches for LockFileUsageArgs { fn from_arg_matches(matches: &clap::ArgMatches) -> Result { let raw = LockFileUsageArgsRaw::from_arg_matches(matches)?; - raw.try_into().map_err(|e: LockFileUsageError| { - clap::Error::raw(clap::error::ErrorKind::ArgumentConflict, e.to_string()) - }) + Ok(raw.into()) } fn update_from_arg_matches(&mut self, matches: &clap::ArgMatches) -> Result<(), clap::Error> { @@ -265,17 +258,14 @@ impl From for pixi_core::environment::LockFileUsage { } } -impl TryFrom for pixi_core::environment::LockFileUsage { - type Error = LockFileUsageError; - - fn try_from(value: LockFileUsageConfig) -> Result { - value.validate()?; +impl From for pixi_core::environment::LockFileUsage { + fn from(value: LockFileUsageConfig) -> Self { if value.frozen { - Ok(Self::Frozen) + Self::Frozen } else if value.locked { - Ok(Self::Locked) + Self::Locked } else { - Ok(Self::Update) + Self::Update } } } @@ -285,23 +275,14 @@ impl TryFrom for pixi_core::environment::LockFileUsage { pub struct LockFileUsageConfig { /// Install the environment as defined in the lockfile, doesn't update /// lockfile if it isn't up-to-date with the manifest file. - #[clap(long, env = "PIXI_FROZEN", help_heading = consts::CLAP_UPDATE_OPTIONS)] + #[clap(long, env = "PIXI_FROZEN", help_heading = consts::CLAP_UPDATE_OPTIONS, conflicts_with = "locked")] pub frozen: bool, /// Check if lockfile is up-to-date before installing the environment, /// aborts when lockfile isn't up-to-date with the manifest file. - #[clap(long, env = "PIXI_LOCKED", help_heading = consts::CLAP_UPDATE_OPTIONS)] + #[clap(long, env = "PIXI_LOCKED", help_heading = consts::CLAP_UPDATE_OPTIONS, conflicts_with = "frozen")] pub locked: bool, } -impl LockFileUsageConfig { - /// Validate that the configuration is valid - pub fn validate(&self) -> Result<(), LockFileUsageError> { - if self.frozen && self.locked { - return Err(LockFileUsageError::FrozenAndLocked); - } - Ok(()) - } -} pub async fn execute() -> miette::Result<()> { let args = Args::parse(); @@ -583,94 +564,7 @@ mod tests { use super::*; use temp_env; - #[test] - fn test_frozen_and_locked_conflict() { - // Test that --frozen and --locked conflict is caught by validation - let config_result = LockFileUsageConfig::try_parse_from(["test", "--frozen", "--locked"]); - assert!(config_result.is_ok(), "Parsing should succeed"); - let parsed = config_result.unwrap(); - assert!(parsed.frozen, "Expected frozen to be true"); - assert!(parsed.locked, "Expected locked to be true"); - assert!( - parsed.validate().is_err(), - "Expected validation to fail when both --frozen and --locked are provided" - ); - } - #[test] - fn test_lockfile_usage_args_try_from_validation() { - // Valid case - let valid_raw = LockFileUsageArgsRaw { - frozen: true, - locked: false, - }; - let result = LockFileUsageArgs::try_from(valid_raw); - assert!(result.is_ok()); - - // Valid case - let valid_raw = LockFileUsageArgsRaw { - frozen: false, - locked: true, - }; - let result = LockFileUsageArgs::try_from(valid_raw); - assert!(result.is_ok()); - - // Valid case - let valid_raw = LockFileUsageArgsRaw { - frozen: false, - locked: false, - }; - let result = LockFileUsageArgs::try_from(valid_raw); - assert!(result.is_ok()); - - // Invalid case - let invalid_raw = LockFileUsageArgsRaw { - frozen: true, - locked: true, - }; - let result = LockFileUsageArgs::try_from(invalid_raw); - assert!(result.is_err()); - - let error = result.unwrap_err(); - assert!(error.to_string().contains("cannot be used together with")); - } - - #[test] - fn test_pixi_frozen_true_with_locked_flag_should_fail() { - // PIXI_FROZEN=true with --locked should fail validation - temp_env::with_var("PIXI_FROZEN", Some("true"), || { - let result = LockFileUsageConfig::try_parse_from(["test", "--locked"]); - - assert!( - result.is_ok(), - "Parsing should succeed, but validation should fail" - ); - let parsed = result.unwrap(); - assert!(parsed.frozen, "Expected frozen flag to be true"); - assert!(parsed.locked, "Expected locked flag to be true"); - assert!( - parsed.validate().is_err(), - "Expected validation to fail when both frozen and locked are true" - ); - }); - } - - #[test] - fn test_pixi_frozen_false_with_locked_flag_should_pass() { - // PIXI_FROZEN=false with --locked should pass validation - temp_env::with_var("PIXI_FROZEN", Some("false"), || { - let result = LockFileUsageConfig::try_parse_from(["test", "--locked"]); - - assert!( - result.is_ok(), - "Expected success when PIXI_FROZEN=false and --locked is used" - ); - let parsed = result.unwrap(); - assert!(parsed.locked, "Expected locked flag to be true"); - assert!(!parsed.frozen, "Expected frozen flag to be false"); - assert!(parsed.validate().is_ok(), "Expected validation to pass"); - }); - } #[test] fn test_clap_boolean_env_var_behavior() { diff --git a/src/cli/reinstall.rs b/src/cli/reinstall.rs index 56f767c127..73be5bea89 100644 --- a/src/cli/reinstall.rs +++ b/src/cli/reinstall.rs @@ -82,7 +82,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { &environment, UpdateMode::Revalidate, UpdateLockFileOptions { - lock_file_usage: args.lock_file_usage.clone().try_into()?, + lock_file_usage: args.lock_file_usage.clone().into(), no_install: false, max_concurrent_solves: workspace.config().max_concurrent_solves(), }, diff --git a/src/cli/remove.rs b/src/cli/remove.rs index 70fb8b5faa..45c3d8d06e 100644 --- a/src/cli/remove.rs +++ b/src/cli/remove.rs @@ -121,7 +121,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { &workspace.default_environment(), UpdateMode::Revalidate, UpdateLockFileOptions { - lock_file_usage: lock_file_update_config.lock_file_usage()?, + lock_file_usage: lock_file_update_config.lock_file_usage(), no_install: prefix_update_config.no_install, max_concurrent_solves: workspace.config().max_concurrent_solves(), }, diff --git a/src/cli/run.rs b/src/cli/run.rs index 1d9727b581..54ef93c97b 100644 --- a/src/cli/run.rs +++ b/src/cli/run.rs @@ -127,7 +127,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { // Ensure that the lock-file is up-to-date. let lock_file = workspace .update_lock_file(UpdateLockFileOptions { - lock_file_usage: args.lock_file_update_config.lock_file_usage()?, + lock_file_usage: args.lock_file_update_config.lock_file_usage(), max_concurrent_solves: workspace.config().max_concurrent_solves(), ..UpdateLockFileOptions::default() }) diff --git a/src/cli/shell.rs b/src/cli/shell.rs index 17318396e1..0d592c2099 100644 --- a/src/cli/shell.rs +++ b/src/cli/shell.rs @@ -280,7 +280,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { &environment, UpdateMode::QuickValidate, UpdateLockFileOptions { - lock_file_usage: args.lock_file_update_config.lock_file_usage()?, + lock_file_usage: args.lock_file_update_config.lock_file_usage(), no_install: args.prefix_update_config.no_install && args.lock_file_update_config.no_lockfile_update, max_concurrent_solves: workspace.config().max_concurrent_solves(), diff --git a/src/cli/shell_hook.rs b/src/cli/shell_hook.rs index d0181cbc40..70a2a7a789 100644 --- a/src/cli/shell_hook.rs +++ b/src/cli/shell_hook.rs @@ -162,7 +162,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { &environment, args.prefix_update_config.update_mode(), UpdateLockFileOptions { - lock_file_usage: args.lock_file_update_config.lock_file_usage()?, + lock_file_usage: args.lock_file_update_config.lock_file_usage(), no_install: args.prefix_update_config.no_install && args.lock_file_update_config.no_lockfile_update, max_concurrent_solves: workspace.config().max_concurrent_solves(), diff --git a/src/cli/tree.rs b/src/cli/tree.rs index 3531ae9624..686cd183ab 100644 --- a/src/cli/tree.rs +++ b/src/cli/tree.rs @@ -81,7 +81,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { let lock_file = workspace .update_lock_file(UpdateLockFileOptions { - lock_file_usage: args.lock_file_update_config.lock_file_usage()?, + lock_file_usage: args.lock_file_update_config.lock_file_usage(), no_install: args.lock_file_update_config.no_lockfile_update, max_concurrent_solves: workspace.config().max_concurrent_solves(), }) diff --git a/src/cli/upgrade.rs b/src/cli/upgrade.rs index deef9c2dc9..eadc2177da 100644 --- a/src/cli/upgrade.rs +++ b/src/cli/upgrade.rs @@ -93,7 +93,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { pypi_deps, IndexMap::default(), args.prefix_update_config.no_install, - &args.lock_file_update_config.lock_file_usage()?, + &args.lock_file_update_config.lock_file_usage(), &args.specs.feature, &[], false, diff --git a/src/cli/workspace/export/conda_explicit_spec.rs b/src/cli/workspace/export/conda_explicit_spec.rs index ac0a22f90b..2083a89eb8 100644 --- a/src/cli/workspace/export/conda_explicit_spec.rs +++ b/src/cli/workspace/export/conda_explicit_spec.rs @@ -168,7 +168,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { let lockfile = workspace .update_lock_file(UpdateLockFileOptions { - lock_file_usage: args.lock_file_update_config.lock_file_usage()?, + lock_file_usage: args.lock_file_update_config.lock_file_usage(), no_install: args.lock_file_update_config.no_lockfile_update, max_concurrent_solves: workspace.config().max_concurrent_solves(), }) diff --git a/tests/integration_rust/common/mod.rs b/tests/integration_rust/common/mod.rs index 1ab9beca5a..f594deb909 100644 --- a/tests/integration_rust/common/mod.rs +++ b/tests/integration_rust/common/mod.rs @@ -504,7 +504,7 @@ impl PixiControl { // Ensure the lock-file is up-to-date let lock_file = project .update_lock_file(UpdateLockFileOptions { - lock_file_usage: args.lock_file_update_config.lock_file_usage()?, + lock_file_usage: args.lock_file_update_config.lock_file_usage(), ..UpdateLockFileOptions::default() }) .await?; diff --git a/tests/integration_rust/upgrade_tests.rs b/tests/integration_rust/upgrade_tests.rs index 72f6d78ef1..a44a0b1f43 100644 --- a/tests/integration_rust/upgrade_tests.rs +++ b/tests/integration_rust/upgrade_tests.rs @@ -45,7 +45,7 @@ async fn pypi_dependency_index_preserved_on_upgrade() { pypi_deps, IndexMap::default(), args.prefix_update_config.no_install, - &args.lock_file_update_config.lock_file_usage().unwrap(), + &args.lock_file_update_config.lock_file_usage(), &args.specs.feature, &[], true, From 003a0b857f6b483c0acd7ae7543f209bbd3bdf62 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Fri, 15 Aug 2025 10:51:24 +0200 Subject: [PATCH 2/2] fix: fmt --- src/cli/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 9bc5c5247f..7d451eaa1c 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -283,7 +283,6 @@ pub struct LockFileUsageConfig { pub locked: bool, } - pub async fn execute() -> miette::Result<()> { let args = Args::parse(); @@ -564,8 +563,6 @@ mod tests { use super::*; use temp_env; - - #[test] fn test_clap_boolean_env_var_behavior() { // Test PIXI_FROZEN=true