diff --git a/crates/rattler_conda_types/src/generic_virtual_package.rs b/crates/rattler_conda_types/src/generic_virtual_package.rs index 258fedeac..68f14875c 100644 --- a/crates/rattler_conda_types/src/generic_virtual_package.rs +++ b/crates/rattler_conda_types/src/generic_virtual_package.rs @@ -1,28 +1,28 @@ -use crate::{PackageName, Version}; -use std::fmt::{Display, Formatter}; - -/// A `GenericVirtualPackage` is a Conda package description that contains a `name` and a -/// `version` and a `build_string`. -#[derive(Clone, Debug, Eq, PartialEq, Hash)] -pub struct GenericVirtualPackage { - /// The name of the package - pub name: PackageName, - - /// The version of the package - pub version: Version, - - /// The build identifier of the package. - pub build_string: String, -} - -impl Display for GenericVirtualPackage { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!( - f, - "{}={}={}", - &self.name.as_normalized(), - &self.version, - &self.build_string - ) - } -} +use crate::{PackageName, Version}; +use std::fmt::{Display, Formatter}; + +/// A `GenericVirtualPackage` is a Conda package description that contains a `name` and a +/// `version` and a `build_string`. +#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] +pub struct GenericVirtualPackage { + /// The name of the package + pub name: PackageName, + + /// The version of the package + pub version: Version, + + /// The build identifier of the package. + pub build_string: String, +} + +impl Display for GenericVirtualPackage { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{}={}={}", + &self.name.as_normalized(), + &self.version, + &self.build_string + ) + } +} diff --git a/crates/rattler_libsolv_rs/src/lib.rs b/crates/rattler_libsolv_rs/src/lib.rs index 7df56c157..84758ebda 100644 --- a/crates/rattler_libsolv_rs/src/lib.rs +++ b/crates/rattler_libsolv_rs/src/lib.rs @@ -21,6 +21,7 @@ mod solver; mod transaction; pub use id::{NameId, SolvableId, VersionSetId}; +use itertools::Itertools; pub use pool::Pool; pub use solvable::PackageSolvable; pub use solve_jobs::SolveJobs; @@ -36,26 +37,16 @@ pub use mapping::Mapping; pub trait PackageName: Eq + Hash {} impl PackageName for N {} -/// Version is a name and a version specification. -pub trait VersionTrait: Display { - /// The version associated with this record. - type Version: Display + Ord + Clone; - - /// Returns the version associated with this record - // TODO: We could maybe get rid of this, but would need to know what is generic to display and replace sorting in `problem.rs` - fn version(&self) -> Self::Version; -} - /// Trait describing sets of versions. pub trait VersionSet: Debug + Display + Clone + Eq + Hash { /// Version type associated with the sets manipulated. - type V: VersionTrait; + type V: Display + Ord; /// Evaluate membership of a version in this set. fn contains(&self, v: &Self::V) -> bool; } - -/// Bla +/// Describes how to sort tentative candidates, for a specific dependency provider. E.g conda +/// pypi, etc. pub trait DependencyProvider { /// Sort the specified solvables based on which solvable to try first. fn sort_candidates( @@ -65,3 +56,31 @@ pub trait DependencyProvider { match_spec_to_candidates: &Mapping>>, ); } +/// Defines how merged candidates should be displayed. +pub trait SolvableDisplay { + /// A method that is used to display multiple solvables in a user friendly way. + /// For example the conda provider should only display the versions (not build strings etc.) + /// and merges multiple solvables into one line. + fn display_candidates(&self, pool: &Pool, candidates: &[SolvableId]) -> String; +} + +/// Display merged candidates on single line with `|` as separator. +pub struct DefaultSolvableDisplay; + +impl SolvableDisplay for DefaultSolvableDisplay +where + VS::V: Ord, +{ + fn display_candidates( + &self, + pool: &Pool, + merged_candidates: &[SolvableId], + ) -> String { + merged_candidates + .iter() + .map(|&id| &pool.resolve_solvable(id).inner) + .sorted() + .map(|s| s.to_string()) + .join(" | ") + } +} diff --git a/crates/rattler_libsolv_rs/src/problem.rs b/crates/rattler_libsolv_rs/src/problem.rs index d9df46e73..3f51c8f66 100644 --- a/crates/rattler_libsolv_rs/src/problem.rs +++ b/crates/rattler_libsolv_rs/src/problem.rs @@ -5,6 +5,7 @@ use std::collections::{HashMap, HashSet}; use std::fmt; use std::fmt::{Display, Formatter}; use std::hash::Hash; + use std::rc::Rc; use itertools::Itertools; @@ -16,7 +17,7 @@ use crate::id::{ClauseId, SolvableId, VersionSetId}; use crate::pool::Pool; use crate::solver::clause::Clause; use crate::solver::Solver; -use crate::{DependencyProvider, PackageName, VersionSet, VersionTrait}; +use crate::{DependencyProvider, PackageName, SolvableDisplay, VersionSet}; /// Represents the cause of the solver being unable to find a solution #[derive(Debug)] @@ -148,12 +149,14 @@ impl Problem { VS: VersionSet, N: PackageName + Display, D: DependencyProvider, + M: SolvableDisplay, >( &self, solver: &'a Solver, - ) -> DisplayUnsat<'a, VS, N> { + merged_solvable_display: &'a M, + ) -> DisplayUnsat<'a, VS, N, M> { let graph = self.graph(solver); - DisplayUnsat::new(graph, solver.pool()) + DisplayUnsat::new(graph, solver.pool(), merged_solvable_display) } } @@ -368,9 +371,8 @@ impl ProblemGraph { let mut merged_candidates = HashMap::default(); // TODO: could probably use `sort_candidates` by the dependency provider directly // but we need to mantain the mapping in `m` which goes from `NodeIndex` to `SolvableId` - for mut m in maybe_merge.into_values() { + for m in maybe_merge.into_values() { if m.len() > 1 { - m.sort_unstable_by_key(|&(_, id)| pool.resolve_solvable(id).inner.version()); let m = Rc::new(MergedProblemNode { ids: m.into_iter().map(|(_, snd)| snd).collect(), }); @@ -477,16 +479,24 @@ impl ProblemGraph { /// A struct implementing [`fmt::Display`] that generates a user-friendly representation of a /// problem graph -pub struct DisplayUnsat<'pool, VS: VersionSet, N: PackageName + Display> { +pub struct DisplayUnsat<'pool, VS: VersionSet, N: PackageName + Display, M: SolvableDisplay> +{ graph: ProblemGraph, merged_candidates: HashMap>, installable_set: HashSet, missing_set: HashSet, pool: &'pool Pool, + merged_solvable_display: &'pool M, } -impl<'pool, VS: VersionSet, N: PackageName + Display> DisplayUnsat<'pool, VS, N> { - pub(crate) fn new(graph: ProblemGraph, pool: &'pool Pool) -> Self { +impl<'pool, VS: VersionSet, N: PackageName + Display, M: SolvableDisplay> + DisplayUnsat<'pool, VS, N, M> +{ + pub(crate) fn new( + graph: ProblemGraph, + pool: &'pool Pool, + merged_solvable_display: &'pool M, + ) -> Self { let merged_candidates = graph.simplify(pool); let installable_set = graph.get_installable_set(); let missing_set = graph.get_missing_set(); @@ -497,6 +507,7 @@ impl<'pool, VS: VersionSet, N: PackageName + Display> DisplayUnsat<'pool, VS, N> installable_set, missing_set, pool, + merged_solvable_display, } } @@ -627,13 +638,11 @@ impl<'pool, VS: VersionSet, N: PackageName + Display> DisplayUnsat<'pool, VS, N> let name = self.pool.resolve_package_name(solvable.name); let version = if let Some(merged) = self.merged_candidates.get(&solvable_id) { reported.extend(merged.ids.iter().cloned()); - merged - .ids - .iter() - .map(|&id| self.pool.resolve_solvable(id).inner.version().to_string()) - .join(" | ") + self.merged_solvable_display + .display_candidates(self.pool, &merged.ids) } else { - solvable.inner.version().to_string() + self.merged_solvable_display + .display_candidates(self.pool, &[solvable_id]) }; let already_installed = graph.edges(candidate).any(|e| { @@ -704,7 +713,9 @@ impl<'pool, VS: VersionSet, N: PackageName + Display> DisplayUnsat<'pool, VS, N> } } -impl fmt::Display for DisplayUnsat<'_, VS, N> { +impl> fmt::Display + for DisplayUnsat<'_, VS, N, M> +{ fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { let (top_level_missing, top_level_conflicts): (Vec<_>, _) = self .graph @@ -741,7 +752,8 @@ impl fmt::Display for DisplayUnsat<'_, f, "{indent}{} {} is locked, but another version is required as reported above", locked.name.display(self.pool), - locked.inner.version() + self.merged_solvable_display + .display_candidates(self.pool, &[locked_id]) )?; } } diff --git a/crates/rattler_libsolv_rs/src/solver/mod.rs b/crates/rattler_libsolv_rs/src/solver/mod.rs index 402329d1e..fe0e981b3 100644 --- a/crates/rattler_libsolv_rs/src/solver/mod.rs +++ b/crates/rattler_libsolv_rs/src/solver/mod.rs @@ -924,7 +924,7 @@ impl> Sol mod test { use super::*; use crate::solvable::Solvable; - use crate::VersionTrait; + use crate::DefaultSolvableDisplay; use std::fmt::{Debug, Display, Formatter}; use std::ops::Range; use std::str::FromStr; @@ -948,7 +948,7 @@ mod test { fn name(&self) -> &Self::Name; } - #[derive(Debug)] + #[derive(Debug, Ord, PartialOrd, Eq, PartialEq)] /// This is `Pack` which is a unique version and name in our bespoke packaging system struct Pack(u32); @@ -966,15 +966,7 @@ mod test { impl Display for Pack { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "{:?}", self) - } - } - - impl VersionTrait for Pack { - type Version = u32; - - fn version(&self) -> Self::Version { - self.0 + write!(f, "{}", self.0) } } @@ -1158,7 +1150,9 @@ mod test { let mut solver = Solver::new(pool, provider); match solver.solve(jobs) { Ok(_) => panic!("expected unsat, but a solution was found"), - Err(problem) => problem.display_user_friendly(&solver).to_string(), + Err(problem) => problem + .display_user_friendly(&solver, &DefaultSolvableDisplay) + .to_string(), } } @@ -1177,7 +1171,7 @@ mod test { .package(); assert_eq!(solver.pool.resolve_package_name(solvable.name), "asdf"); - assert_eq!(solvable.inner.version(), 1); + assert_eq!(solvable.inner.0, 1); } /// Test if we can also select a nested version @@ -1199,14 +1193,14 @@ mod test { .resolve_solvable_inner(solved.steps[0]) .package(); assert_eq!(solver.pool.resolve_package_name(solvable.name), "asdf"); - assert_eq!(solvable.inner.version(), 1); + assert_eq!(solvable.inner.0, 1); let solvable = solver .pool .resolve_solvable_inner(solved.steps[1]) .package(); assert_eq!(solver.pool.resolve_package_name(solvable.name), "efgh"); - assert_eq!(solvable.inner.version(), 4); + assert_eq!(solvable.inner.0, 4); } /// Test if we can resolve multiple versions at once @@ -1229,14 +1223,14 @@ mod test { .resolve_solvable_inner(solved.steps[0]) .package(); assert_eq!(solver.pool.resolve_package_name(solvable.name), "asdf"); - assert_eq!(solvable.inner.version(), 2); + assert_eq!(solvable.inner.0, 2); let solvable = solver .pool .resolve_solvable_inner(solved.steps[1]) .package(); assert_eq!(solver.pool.resolve_package_name(solvable.name), "efgh"); - assert_eq!(solvable.inner.version(), 5); + assert_eq!(solvable.inner.0, 5); } /// In case of a conflict the version should not be selected with the conflict @@ -1255,7 +1249,10 @@ mod test { let solved = solver.solve(jobs); let solved = match solved { Ok(solved) => solved, - Err(p) => panic!("{}", p.display_user_friendly(&solver)), + Err(p) => panic!( + "{}", + p.display_user_friendly(&solver, &DefaultSolvableDisplay) + ), }; use std::fmt::Write; @@ -1287,7 +1284,7 @@ mod test { .resolve_solvable_inner(solved.steps[0]) .package(); assert_eq!(solver.pool.resolve_package_name(solvable.name), "asdf"); - assert_eq!(solvable.inner.version(), 3); + assert_eq!(solvable.inner.0, 3); } /// Locking a specific package version in this case a lower version namely `3` should result @@ -1302,7 +1299,7 @@ mod test { .iter() .position(|s: &Solvable<_>| { if let Some(package) = s.get_package() { - package.inner.version() == 3 + package.inner.0 == 3 } else { false } @@ -1325,7 +1322,7 @@ mod test { .resolve_solvable_inner(solvable_id) .package() .inner - .version(), + .0, 3 ); } @@ -1345,7 +1342,7 @@ mod test { .iter() .position(|s| { if let Some(package) = s.get_package() { - package.inner.version() == 1 + package.inner.0 == 1 } else { false } @@ -1366,7 +1363,7 @@ mod test { .resolve_solvable_inner(solved.steps[0]) .package(); assert_eq!(solver.pool.resolve_package_name(solvable.name), "asdf"); - assert_eq!(solvable.inner.version(), 4); + assert_eq!(solvable.inner.0, 4); } /// Test checks if favoring without a conflict results in a package upgrade @@ -1388,7 +1385,7 @@ mod test { .iter() .enumerate() .skip(1) // Skip the root solvable - .filter(|(_, s)| s.package().inner.version() == 1) + .filter(|(_, s)| s.package().inner.0 == 1) .map(|(i, _)| SolvableId::from_usize(i)); for solvable_id in already_installed { @@ -1399,13 +1396,16 @@ mod test { let solved = solver.solve(jobs); let solved = match solved { Ok(solved) => solved, - Err(p) => panic!("{}", p.display_user_friendly(&solver)), + Err(p) => panic!( + "{}", + p.display_user_friendly(&solver, &DefaultSolvableDisplay) + ), }; let result = transaction_to_string(&solver.pool, &solved); insta::assert_snapshot!(result, @r###" - Pack(2) - Pack(1) + 2 + 1 "###); } // @@ -1429,7 +1429,7 @@ mod test { .iter() .enumerate() .skip(1) // Skip the root solvable - .filter(|(_, s)| s.package().inner.version() == 1) + .filter(|(_, s)| s.package().inner.0 == 1) .map(|(i, _)| SolvableId::from_usize(i)); for solvable_id in already_installed { @@ -1440,14 +1440,17 @@ mod test { let solved = solver.solve(jobs); let solved = match solved { Ok(solved) => solved, - Err(p) => panic!("{}", p.display_user_friendly(&solver)), + Err(p) => panic!( + "{}", + p.display_user_friendly(&solver, &DefaultSolvableDisplay) + ), }; let result = transaction_to_string(&solver.pool, &solved); insta::assert_snapshot!(result, @r###" - Pack(2) - Pack(2) - Pack(2) + 2 + 2 + 2 "###); } @@ -1460,8 +1463,8 @@ mod test { let result = transaction_to_string(&solver.pool, &solved); insta::assert_snapshot!(result, @r###" - Pack(2) - Pack(5) + 2 + 5 "###); } diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__resolve_with_conflict.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__resolve_with_conflict.snap index 58c768e1b..bda53b091 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__resolve_with_conflict.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__resolve_with_conflict.snap @@ -2,7 +2,7 @@ source: crates/rattler_libsolv_rs/src/solver/mod.rs expression: display_result --- -Pack(0) -Pack(3) -Pack(7) +0 +3 +7 diff --git a/crates/rattler_solve/Cargo.toml b/crates/rattler_solve/Cargo.toml index 31d3300f2..64138366e 100644 --- a/crates/rattler_solve/Cargo.toml +++ b/crates/rattler_solve/Cargo.toml @@ -18,6 +18,7 @@ anyhow = "1.0.75" chrono = "0.4.27" thiserror = "1.0.47" tracing = "0.1.37" +itertools = "0.11.0" serde = { version = "1.0.188", features = ["derive"] } url = "2.4.1" hex = "0.4.3" diff --git a/crates/rattler_solve/src/libsolv_rs/mod.rs b/crates/rattler_solve/src/libsolv_rs/mod.rs index b904b0b12..af34ae539 100644 --- a/crates/rattler_solve/src/libsolv_rs/mod.rs +++ b/crates/rattler_solve/src/libsolv_rs/mod.rs @@ -6,8 +6,8 @@ use rattler_conda_types::{ GenericVirtualPackage, NamelessMatchSpec, PackageRecord, RepoDataRecord, }; use rattler_libsolv_rs::{ - DependencyProvider, Mapping, Pool, SolvableId, SolveJobs, Solver as LibSolvRsSolver, - VersionSet, VersionSetId, VersionTrait, + DependencyProvider, Mapping, Pool, SolvableDisplay, SolvableId, SolveJobs, + Solver as LibSolvRsSolver, VersionSet, VersionSetId, }; use std::{ cell::OnceCell, @@ -17,6 +17,8 @@ use std::{ ops::Deref, }; +use itertools::Itertools; + mod conda_util; mod input; @@ -99,6 +101,7 @@ impl<'a> VersionSet for SolverMatchSpec<'a> { } /// Wrapper around [`PackageRecord`] so that we can use it in libsolv_rs pool +#[derive(Ord, PartialOrd, Eq, PartialEq)] enum SolverPackageRecord<'a> { Record(&'a RepoDataRecord), VirtualPackage(&'a GenericVirtualPackage), @@ -148,19 +151,7 @@ impl<'a> Display for SolverPackageRecord<'a> { } } -impl<'a> VersionTrait for SolverPackageRecord<'a> { - type Version = rattler_conda_types::Version; - - fn version(&self) -> Self::Version { - match self { - SolverPackageRecord::Record(rec) => rec.package_record.version.version().clone(), - SolverPackageRecord::VirtualPackage(rec) => rec.version.clone(), - } - } -} - /// Dependency provider for conda - #[derive(Default)] pub(crate) struct CondaDependencyProvider { // TODO: cache is dangerous as it is now because it is not invalidated when the pool changes @@ -193,6 +184,24 @@ impl<'a> DependencyProvider> for CondaDependencyProvider { } } +/// Displays the different candidates by their version and sorted by their version +pub struct CondaSolvableDisplay; + +impl SolvableDisplay> for CondaSolvableDisplay { + fn display_candidates( + &self, + pool: &Pool, + merged_candidates: &[SolvableId], + ) -> String { + merged_candidates + .iter() + .map(|&id| pool.resolve_solvable(id).inner().version()) + .sorted() + .map(|s| s.to_string()) + .join(" | ") + } +} + /// A [`Solver`] implemented using the `libsolv` library #[derive(Default)] pub struct Solver; @@ -271,7 +280,9 @@ impl super::SolverImpl for Solver { // Construct a solver and solve the problems in the queue let mut solver = LibSolvRsSolver::new(pool, CondaDependencyProvider::default()); let transaction = solver.solve(goal).map_err(|problem| { - SolveError::Unsolvable(vec![problem.display_user_friendly(&solver).to_string()]) + SolveError::Unsolvable(vec![problem + .display_user_friendly(&solver, &CondaSolvableDisplay) + .to_string()]) })?; let required_records = transaction