From aab0e938c18b3eea588ed65b69e93d53563db436 Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Mon, 30 Sep 2024 22:21:04 -0700 Subject: [PATCH 1/6] Add test --- crates/uv/src/commands/tool/install.rs | 62 ++++++++++++++++++++++---- crates/uv/tests/tool_install.rs | 22 +++++++++ 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index 1723cb8697d7..91c8873ff4a0 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -25,12 +25,12 @@ use crate::commands::pip::loggers::{DefaultInstallLogger, DefaultResolveLogger}; use crate::commands::project::{ resolve_environment, resolve_names, sync_environment, update_environment, - EnvironmentSpecification, + EnvironmentSpecification, ProjectError, }; use crate::commands::tool::common::remove_entrypoints; use crate::commands::tool::Target; +use crate::commands::{pip, ExitStatus, SharedState}; use crate::commands::{reporters::PythonDownloadReporter, tool::common::install_executables}; -use crate::commands::{ExitStatus, SharedState}; use crate::printer::Printer; use crate::settings::ResolverInstallerSettings; @@ -328,12 +328,13 @@ pub(crate) async fn install( } // Create a `RequirementsSpecification` from the resolved requirements, to avoid re-resolving. + let spec_requirements: Vec = requirements + .iter() + .cloned() + .map(UnresolvedRequirementSpecification::from) + .collect(); let spec = RequirementsSpecification { - requirements: requirements - .iter() - .cloned() - .map(UnresolvedRequirementSpecification::from) - .collect(), + requirements: spec_requirements.clone(), ..spec }; @@ -380,7 +381,52 @@ pub(crate) async fn install( &cache, printer, ) - .await?; + .await; + + let resolution = match resolution { + Ok(resolution) => resolution, + Err( + err @ ProjectError::Operation(pip::operations::Error::Resolve( + uv_resolver::ResolveError::NoSolution(_), + )), + ) => { + if python_request.is_some() { + return Err(err.into()); + } + + let interpreter = PythonInstallation::find_or_download( + Some(&PythonRequest::parse("3.12")), + EnvironmentPreference::OnlySystem, + python_preference, + python_downloads, + &client_builder, + &cache, + Some(&reporter), + ) + .await? + .into_interpreter(); + + let new_spec = RequirementsSpecification { + requirements: spec_requirements, + ..RequirementsSpecification::default() + }; + + resolve_environment( + EnvironmentSpecification::from(new_spec), + &interpreter, + settings.as_ref().into(), + &state, + Box::new(DefaultResolveLogger), + connectivity, + concurrency, + native_tls, + &cache, + printer, + ) + .await? + } + Err(e) => return Err(e.into()), + }; let environment = installed_tools.create_environment(&from.name, interpreter)?; diff --git a/crates/uv/tests/tool_install.rs b/crates/uv/tests/tool_install.rs index d045e65168e7..6251260de138 100644 --- a/crates/uv/tests/tool_install.rs +++ b/crates/uv/tests/tool_install.rs @@ -3007,3 +3007,25 @@ fn tool_install_at_latest_upgrade() { "###); }); } + +#[test] +fn tool_install_python() { + let context = TestContext::new("3.8"); + let tool_dir = context.temp_dir.child("tools"); + let bin_dir = context.temp_dir.child("bin"); + + uv_snapshot!(context.filters(), context.tool_install() + .arg("posting") + .arg("--python") + .arg("3.8") + .env("UV_TOOL_DIR", tool_dir.as_os_str()) + .env("XDG_BIN_HOME", bin_dir.as_os_str()) + .env("PATH", bin_dir.as_os_str()), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + + "###); +} From 6bc87051c9e4d92cb2a57ea4474c3437642695cc Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Tue, 1 Oct 2024 12:45:49 -0700 Subject: [PATCH 2/6] Modifications --- crates/uv-resolver/src/error.rs | 51 ++++++++++++++++ crates/uv/src/commands/tool/install.rs | 82 +++++++++++++------------- 2 files changed, 93 insertions(+), 40 deletions(-) diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index b5e5bdfc9f41..c6a441a3bcd1 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -1,8 +1,10 @@ use std::collections::{BTreeMap, BTreeSet}; use std::fmt::Formatter; +use std::ops::{Bound, Deref}; use std::sync::Arc; use indexmap::IndexSet; +use itertools::Itertools; use pubgrub::{DefaultStringReporter, DerivationTree, Derived, External, Range, Reporter}; use rustc_hash::FxHashMap; @@ -197,6 +199,55 @@ impl NoSolutionError { pub fn header(&self) -> NoSolutionHeader { NoSolutionHeader::new(self.markers.clone()) } + + pub fn required_python_version(&self) -> Option { + let mut required_pythons = NoSolutionError::find_required_python_versions(&self.error); + required_pythons.sort(); + // Return the largest required Python version + required_pythons.last().cloned() + } + + /// Traverses a derivation tree to find required Python instances + fn find_required_python_versions(derivation_tree: &ErrorTree) -> Vec { + fn find(derivation_tree: &ErrorTree) -> Vec { + match derivation_tree { + DerivationTree::Derived(derived) => { + let mut result = find(derived.cause1.as_ref()); + result.extend(find(derived.cause2.as_ref())); + result + } + DerivationTree::External(External::FromDependencyOf( + pkg_1, + ver_1, + pkg_2, + ver_2, + )) => match (pkg_1.deref(), pkg_2.deref()) { + (PubGrubPackageInner::Python(_), _) => { + if let Some((Bound::Included(version), _)) = + ver_1.iter().collect_vec().first() + { + vec![version.clone()] + } else { + Vec::new() + } + } + (_, PubGrubPackageInner::Python(_)) => { + if let Some((Bound::Included(version), _)) = + ver_2.iter().collect_vec().first() + { + vec![version.clone()] + } else { + Vec::new() + } + } + (_, _) => Vec::new(), + }, + DerivationTree::External(_) => Vec::new(), + } + } + + find(derivation_tree) + } } impl std::error::Error for NoSolutionError {} diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index 91c8873ff4a0..2e883c118b3c 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -385,47 +385,49 @@ pub(crate) async fn install( let resolution = match resolution { Ok(resolution) => resolution, - Err( - err @ ProjectError::Operation(pip::operations::Error::Resolve( - uv_resolver::ResolveError::NoSolution(_), - )), - ) => { - if python_request.is_some() { - return Err(err.into()); + Err(err) => match err { + ProjectError::Operation(pip::operations::Error::Resolve( + uv_resolver::ResolveError::NoSolution(ref no_solution_err), + )) => { + let (None, Some(version)) = + (python_request, no_solution_err.required_python_version()) + else { + return Err(err.into()); + }; + + let interpreter = PythonInstallation::find_or_download( + Some(&PythonRequest::parse(version.to_string().as_str())), + EnvironmentPreference::OnlySystem, + python_preference, + python_downloads, + &client_builder, + &cache, + Some(&reporter), + ) + .await? + .into_interpreter(); + + let new_spec = RequirementsSpecification { + requirements: spec_requirements, + ..RequirementsSpecification::default() + }; + + resolve_environment( + EnvironmentSpecification::from(new_spec), + &interpreter, + settings.as_ref().into(), + &state, + Box::new(DefaultResolveLogger), + connectivity, + concurrency, + native_tls, + &cache, + printer, + ) + .await? } - - let interpreter = PythonInstallation::find_or_download( - Some(&PythonRequest::parse("3.12")), - EnvironmentPreference::OnlySystem, - python_preference, - python_downloads, - &client_builder, - &cache, - Some(&reporter), - ) - .await? - .into_interpreter(); - - let new_spec = RequirementsSpecification { - requirements: spec_requirements, - ..RequirementsSpecification::default() - }; - - resolve_environment( - EnvironmentSpecification::from(new_spec), - &interpreter, - settings.as_ref().into(), - &state, - Box::new(DefaultResolveLogger), - connectivity, - concurrency, - native_tls, - &cache, - printer, - ) - .await? - } - Err(e) => return Err(e.into()), + _ => return Err(err.into()), + }, }; let environment = installed_tools.create_environment(&from.name, interpreter)?; From af52178ca7a452642fad547106dce82b3775dcc9 Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Tue, 1 Oct 2024 14:51:32 -0700 Subject: [PATCH 3/6] Add changes --- crates/uv-resolver/src/error.rs | 45 +++++++++++++------------- crates/uv/src/commands/tool/install.rs | 12 +++++-- crates/uv/tests/tool_install.rs | 22 +------------ 3 files changed, 33 insertions(+), 46 deletions(-) diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index c6a441a3bcd1..2dcb76cb0bf3 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -200,22 +200,17 @@ impl NoSolutionError { NoSolutionHeader::new(self.markers.clone()) } - pub fn required_python_version(&self) -> Option { - let mut required_pythons = NoSolutionError::find_required_python_versions(&self.error); - required_pythons.sort(); - // Return the largest required Python version - required_pythons.last().cloned() - } - - /// Traverses a derivation tree to find required Python instances - fn find_required_python_versions(derivation_tree: &ErrorTree) -> Vec { - fn find(derivation_tree: &ErrorTree) -> Vec { + pub fn find_largest_required_python_version(&self) -> Option { + fn find(derivation_tree: &ErrorTree, largest_version: &mut Option) { match derivation_tree { + // For derived incompatibilities, recursively search each subtree to + // find the root cause DerivationTree::Derived(derived) => { - let mut result = find(derived.cause1.as_ref()); - result.extend(find(derived.cause2.as_ref())); - result + find(derived.cause1.as_ref(), largest_version); + find(derived.cause2.as_ref(), largest_version); } + // Base case — if a missing external dependency a Python package, + // update the largest one seen thus far DerivationTree::External(External::FromDependencyOf( pkg_1, ver_1, @@ -226,27 +221,33 @@ impl NoSolutionError { if let Some((Bound::Included(version), _)) = ver_1.iter().collect_vec().first() { - vec![version.clone()] - } else { - Vec::new() + *largest_version = match largest_version { + Some(ref v) if version > v => Some(version.clone()), + None => Some(version.clone()), + _ => largest_version.clone(), + }; } } (_, PubGrubPackageInner::Python(_)) => { if let Some((Bound::Included(version), _)) = ver_2.iter().collect_vec().first() { - vec![version.clone()] - } else { - Vec::new() + *largest_version = match largest_version { + Some(ref v) if version > v => Some(version.clone()), + None => Some(version.clone()), + _ => largest_version.clone(), + }; } } - (_, _) => Vec::new(), + (_, _) => {} }, - DerivationTree::External(_) => Vec::new(), + DerivationTree::External(_) => {} } } - find(derivation_tree) + let mut largest_version: Option = None; + find(&self.error, &mut largest_version); + largest_version } } diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index 2e883c118b3c..60a2fd179f29 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -389,12 +389,18 @@ pub(crate) async fn install( ProjectError::Operation(pip::operations::Error::Resolve( uv_resolver::ResolveError::NoSolution(ref no_solution_err), )) => { - let (None, Some(version)) = - (python_request, no_solution_err.required_python_version()) - else { + let (None, Some(version)) = ( + python_request, + no_solution_err.find_largest_required_python_version(), + ) else { return Err(err.into()); }; + let _ = writeln!( + printer.stderr(), + "Couldn't find acceptable Python version for {package}, downloading Python {version} and re-attempting install." + )?; + let interpreter = PythonInstallation::find_or_download( Some(&PythonRequest::parse(version.to_string().as_str())), EnvironmentPreference::OnlySystem, diff --git a/crates/uv/tests/tool_install.rs b/crates/uv/tests/tool_install.rs index 6251260de138..ab2d69406538 100644 --- a/crates/uv/tests/tool_install.rs +++ b/crates/uv/tests/tool_install.rs @@ -3,6 +3,7 @@ use std::process::Command; use anyhow::Result; +use assert_cmd::assert::OutputAssertExt; use assert_fs::{ assert::PathAssert, fixture::{FileTouch, FileWriteStr, PathChild}, @@ -3008,24 +3009,3 @@ fn tool_install_at_latest_upgrade() { }); } -#[test] -fn tool_install_python() { - let context = TestContext::new("3.8"); - let tool_dir = context.temp_dir.child("tools"); - let bin_dir = context.temp_dir.child("bin"); - - uv_snapshot!(context.filters(), context.tool_install() - .arg("posting") - .arg("--python") - .arg("3.8") - .env("UV_TOOL_DIR", tool_dir.as_os_str()) - .env("XDG_BIN_HOME", bin_dir.as_os_str()) - .env("PATH", bin_dir.as_os_str()), @r###" - success: true - exit_code: 0 - ----- stdout ----- - - ----- stderr ----- - - "###); -} From d3601f2a750d5c8e8e42cedc97a5f9e1c8dc9f98 Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Tue, 1 Oct 2024 14:53:50 -0700 Subject: [PATCH 4/6] Fix lint --- crates/uv-resolver/src/error.rs | 4 ++-- crates/uv/src/commands/tool/install.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index 2dcb76cb0bf3..8a24a1d22557 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -1,6 +1,6 @@ use std::collections::{BTreeMap, BTreeSet}; use std::fmt::Formatter; -use std::ops::{Bound, Deref}; +use std::ops::Bound; use std::sync::Arc; use indexmap::IndexSet; @@ -216,7 +216,7 @@ impl NoSolutionError { ver_1, pkg_2, ver_2, - )) => match (pkg_1.deref(), pkg_2.deref()) { + )) => match (&**pkg_1, &**pkg_2) { (PubGrubPackageInner::Python(_), _) => { if let Some((Bound::Included(version), _)) = ver_1.iter().collect_vec().first() diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index 60a2fd179f29..30c67c8a777b 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -396,8 +396,8 @@ pub(crate) async fn install( return Err(err.into()); }; - let _ = writeln!( - printer.stderr(), + writeln!( + printer.stderr(), "Couldn't find acceptable Python version for {package}, downloading Python {version} and re-attempting install." )?; From dc28baf9b4cf60f7f78a6cb1c5948ff902002593 Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Tue, 1 Oct 2024 14:55:02 -0700 Subject: [PATCH 5/6] Fix extraneous changes --- crates/uv/tests/tool_install.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/uv/tests/tool_install.rs b/crates/uv/tests/tool_install.rs index ab2d69406538..d045e65168e7 100644 --- a/crates/uv/tests/tool_install.rs +++ b/crates/uv/tests/tool_install.rs @@ -3,7 +3,6 @@ use std::process::Command; use anyhow::Result; -use assert_cmd::assert::OutputAssertExt; use assert_fs::{ assert::PathAssert, fixture::{FileTouch, FileWriteStr, PathChild}, @@ -3008,4 +3007,3 @@ fn tool_install_at_latest_upgrade() { "###); }); } - From d747eb31597f11397e7d0e5d4806e3c43ceac382 Mon Sep 17 00:00:00 2001 From: Tej Singh Date: Tue, 1 Oct 2024 15:20:33 -0700 Subject: [PATCH 6/6] Address comments --- crates/uv-resolver/src/error.rs | 17 ++++++----------- crates/uv/src/commands/tool/install.rs | 2 +- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/crates/uv-resolver/src/error.rs b/crates/uv-resolver/src/error.rs index 8a24a1d22557..08c9660b3217 100644 --- a/crates/uv-resolver/src/error.rs +++ b/crates/uv-resolver/src/error.rs @@ -4,7 +4,6 @@ use std::ops::Bound; use std::sync::Arc; use indexmap::IndexSet; -use itertools::Itertools; use pubgrub::{DefaultStringReporter, DerivationTree, Derived, External, Range, Reporter}; use rustc_hash::FxHashMap; @@ -218,24 +217,20 @@ impl NoSolutionError { ver_2, )) => match (&**pkg_1, &**pkg_2) { (PubGrubPackageInner::Python(_), _) => { - if let Some((Bound::Included(version), _)) = - ver_1.iter().collect_vec().first() - { - *largest_version = match largest_version { + if let Some((Bound::Included(version), _)) = ver_1.iter().next() { + *largest_version = match largest_version.take() { Some(ref v) if version > v => Some(version.clone()), + Some(v) => Some(v), None => Some(version.clone()), - _ => largest_version.clone(), }; } } (_, PubGrubPackageInner::Python(_)) => { - if let Some((Bound::Included(version), _)) = - ver_2.iter().collect_vec().first() - { - *largest_version = match largest_version { + if let Some((Bound::Included(version), _)) = ver_2.iter().next() { + *largest_version = match largest_version.take() { Some(ref v) if version > v => Some(version.clone()), + Some(v) => Some(v), None => Some(version.clone()), - _ => largest_version.clone(), }; } } diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index 30c67c8a777b..c7a05a72944b 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -398,7 +398,7 @@ pub(crate) async fn install( writeln!( printer.stderr(), - "Couldn't find acceptable Python version for {package}, downloading Python {version} and re-attempting install." + "Couldn't find acceptable Python version for `{package}`, downloading Python {version} and re-attempting install." )?; let interpreter = PythonInstallation::find_or_download(