From ba52a2c60bed30e99c89b0d0aa0d267c2dc9b8d6 Mon Sep 17 00:00:00 2001 From: Orion Yeung <11580988+orionyeung001@users.noreply.github.com> Date: Fri, 15 Sep 2023 17:05:52 -0500 Subject: [PATCH 1/6] added build_spec module - unused in match_spec --- .../rattler_conda_types/src/build_spec/mod.rs | 97 +++++++++ .../src/build_spec/parse.rs | 196 ++++++++++++++++++ crates/rattler_conda_types/src/lib.rs | 1 + 3 files changed, 294 insertions(+) create mode 100644 crates/rattler_conda_types/src/build_spec/mod.rs create mode 100644 crates/rattler_conda_types/src/build_spec/parse.rs diff --git a/crates/rattler_conda_types/src/build_spec/mod.rs b/crates/rattler_conda_types/src/build_spec/mod.rs new file mode 100644 index 000000000..094963e23 --- /dev/null +++ b/crates/rattler_conda_types/src/build_spec/mod.rs @@ -0,0 +1,97 @@ +//! This module contains code to work with "build number spec". It represents the build number key of +//! [`crate::MatchSpec`], e.g.: `>=3,<4`. + +pub mod parse; + +use serde::{Deserialize, Serialize}; +use std::fmt::{Display, Formatter}; + +/// Named type for the BuildNumber instead of explicit u64 floating about the project +pub type BuildNumber = u64; + +/// An operator to compare two versions. +#[allow(missing_docs)] +#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize, Deserialize)] +pub enum OrdOperator { + Gt, + Ge, + Lt, + Le, + Eq, + Ne, +} + +/// Define match from some kind of operator and a specific element +/// +/// Ideally we could have some kind of type constraint to guarantee that +/// there's function relating the operator and element into a function that returns bool +/// possible TODO: create `Operator` trait +#[derive(Debug, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, Deserialize)] +pub struct OperatorConstraint { + op: Operator, + rhs: Element, +} + +impl OperatorConstraint { + /// convenience constructor wrapper + pub fn new(op: Operator, rhs: Element) -> Self { + Self { op, rhs } + } +} + +/// Define match from OrdOperator and BuildNumber as Element +pub type BuildNumberSpec = OperatorConstraint; + +impl Display for OrdOperator { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + Self::Gt => write!(f, ">"), + Self::Ge => write!(f, ">="), + Self::Lt => write!(f, "<"), + Self::Le => write!(f, "<="), + Self::Eq => write!(f, "=="), + Self::Ne => write!(f, "!="), + } + } +} + +impl Display for BuildNumberSpec { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}{}", self.op, self.rhs) + } +} + +impl BuildNumberSpec { + /// Returns whether the number matches the specification. + /// Expected use is within match_spec::MatchSpec::matches + pub fn matches(&self, build_num: &BuildNumber) -> bool { + match self.op { + OrdOperator::Gt => build_num.gt(&self.rhs), + OrdOperator::Ge => build_num.ge(&self.rhs), + OrdOperator::Lt => build_num.lt(&self.rhs), + OrdOperator::Le => build_num.le(&self.rhs), + OrdOperator::Eq => build_num.eq(&self.rhs), + OrdOperator::Ne => build_num.ne(&self.rhs), + } + } +} + +#[cfg(test)] +mod tests { + use super::{BuildNumberSpec, OrdOperator}; + + #[test] + fn test_matches() { + let test_cases = vec![ + (BuildNumberSpec::new(OrdOperator::Gt, 3), 5, true), + (BuildNumberSpec::new(OrdOperator::Ge, 3), 5, true), + (BuildNumberSpec::new(OrdOperator::Lt, 3), 5, false), + (BuildNumberSpec::new(OrdOperator::Le, 3), 7, false), + (BuildNumberSpec::new(OrdOperator::Eq, 3), 7, false), + (BuildNumberSpec::new(OrdOperator::Ne, 3), 7, true), + ]; + for (spec, test_val, is_match) in test_cases { + assert_eq!(spec.matches(&test_val), is_match); + } + } +} diff --git a/crates/rattler_conda_types/src/build_spec/parse.rs b/crates/rattler_conda_types/src/build_spec/parse.rs new file mode 100644 index 000000000..aba6bb6dc --- /dev/null +++ b/crates/rattler_conda_types/src/build_spec/parse.rs @@ -0,0 +1,196 @@ +//! this module supports the parsing features of the build number spec + +use super::{BuildNumber, BuildNumberSpec, OrdOperator}; + +use nom::{bytes::complete::take_while1, character::complete::digit1, Finish, IResult}; +use std::str::FromStr; +use thiserror::Error; + +impl FromStr for BuildNumberSpec { + type Err = ParseBuildNumberSpecError; + + fn from_str(s: &str) -> Result { + match Self::parser(s).finish()? { + ("", spec) => Ok(spec), + (_, _) => Err(ParseBuildNumberSpecError::ExpectedEof), + } + } +} + +impl BuildNumberSpec { + /// Parses a build number spec, string representation is optional operator preceding whole number + pub fn parser(input: &str) -> IResult<&str, BuildNumberSpec, ParseBuildNumberSpecError> { + // Parse the optional preceding operator + let (input, op) = match OrdOperator::parser(input) { + Err( + nom::Err::Failure(ParseOrdOperatorError::InvalidOperator(op)) + | nom::Err::Error(ParseOrdOperatorError::InvalidOperator(op)), + ) => { + return Err(nom::Err::Failure( + ParseBuildNumberSpecError::InvalidOperator( + ParseOrdOperatorError::InvalidOperator(op.to_owned()), + ), + )) + } + Err(nom::Err::Error(_)) => (input, None), + Ok((rest, op)) => (rest, Some(op)), + _ => unreachable!(), + }; + + let (rest, build_num) = digit1(input) + .map(|(rest, digits): (&str, &str)| { + ( + rest, + digits + .parse::() + .expect("nom found at least 1 digit(s)"), + ) + }) + .map_err(|_: nom::Err>| { + nom::Err::Error(ParseBuildNumberSpecError::InvalidBuildNumber( + ParseBuildNumberError, + )) + })?; + + match op { + Some(op) => Ok((rest, BuildNumberSpec::new(op, build_num))), + None => Ok((rest, BuildNumberSpec::new(OrdOperator::Eq, build_num))), + } + } +} + +/// Possible errors when parsing the OrdOperator which precedes the digits in a build number spec +#[derive(Debug, Clone, Error, Eq, PartialEq)] +pub enum ParseOrdOperatorError { + /// Indicates that operator symbols were captured, + /// but not interpretable as an OrdOperator + #[error("invalid operator '{0}'")] + InvalidOperator(String), + /// Indicates no symbol sequence found for OrdOperators, + /// callers should expect explicit operators + #[error("expected version operator")] + ExpectedOperator, + /// Indicates that there was data after an operator was read, + /// callers should handle this if they expect input to end with the operator + #[error("expected EOF")] + ExpectedEof, +} + +/// Simplified error when parsing the digits into a build number: u64 in a build number spec +#[derive(Debug, Clone, Eq, PartialEq, Error)] +#[error("could not parse build number")] +pub struct ParseBuildNumberError; + +/// Composition of possible errors when parsing the spec for build numbers +#[allow(clippy::enum_variant_names, missing_docs)] +#[derive(Debug, Clone, Error, Eq, PartialEq)] +pub enum ParseBuildNumberSpecError { + #[error("invalid version: {0}")] + InvalidBuildNumber(#[source] ParseBuildNumberError), + #[error("invalid version constraint: {0}")] + InvalidOperator(#[source] ParseOrdOperatorError), + #[error("expected EOF")] + ExpectedEof, +} + +impl FromStr for OrdOperator { + type Err = ParseOrdOperatorError; + + fn from_str(s: &str) -> Result { + match Self::parser(s).finish()? { + ("", spec) => Ok(spec), + (_, _) => Err(ParseOrdOperatorError::ExpectedEof), + } + } +} + +impl OrdOperator { + /// Parses an operator representing PartialOrd compares, returns an error if the operator is not recognized or not found. + fn parser(input: &str) -> IResult<&str, OrdOperator, ParseOrdOperatorError> { + // Take anything that looks like an operator. + let (rest, operator_str) = take_while1(|c| "=!<>".contains(c))(input).map_err( + |_: nom::Err>| { + nom::Err::Error(ParseOrdOperatorError::ExpectedOperator) + }, + )?; + + let op = match operator_str { + "==" => OrdOperator::Eq, + "!=" => OrdOperator::Ne, + "<=" => OrdOperator::Le, + ">=" => OrdOperator::Ge, + "<" => OrdOperator::Lt, + ">" => OrdOperator::Gt, + _ => { + return Err(nom::Err::Failure(ParseOrdOperatorError::InvalidOperator( + operator_str.to_string(), + ))) + } + }; + + Ok((rest, op)) + } +} + +#[cfg(test)] +mod test { + use super::{BuildNumberSpec, OrdOperator, ParseOrdOperatorError}; + + use nom::Finish; + + #[test] + fn parse_operator_from_spec() { + let test_params = vec![ + (">3.1", OrdOperator::Gt), + (">=3.1", OrdOperator::Ge), + ("<3.1", OrdOperator::Lt), + ("<=3.1", OrdOperator::Le), + ("==3.1", OrdOperator::Eq), + ("!=3.1", OrdOperator::Ne), + ]; + + for (s, op) in test_params { + assert_eq!(OrdOperator::parser(s), Ok(("3.1", op))) + } + + assert_eq!( + OrdOperator::parser("<==>3.1"), + Err(nom::Err::Failure(ParseOrdOperatorError::InvalidOperator( + "<==>".to_string() + ))) + ); + assert_eq!( + OrdOperator::parser("3.1"), + Err(nom::Err::Error(ParseOrdOperatorError::ExpectedOperator)) + ); + } + + #[test] + fn parse_spec() { + let test_params = vec![ + (">1", OrdOperator::Gt), + (">=1", OrdOperator::Ge), + ("<1", OrdOperator::Lt), + ("<=1", OrdOperator::Le), + ("==1", OrdOperator::Eq), + ("!=1", OrdOperator::Ne), + ]; + + for (s, op) in test_params { + assert_eq!( + BuildNumberSpec::parser(s), + Ok(("", BuildNumberSpec::new(op, 1))) + ) + } + + assert_eq!( + BuildNumberSpec::parser(">=1.1"), + Ok((".1", BuildNumberSpec::new(OrdOperator::Ge, 1))) + ); + + assert!(matches!( + BuildNumberSpec::parser(">=build3").finish(), + Err(_) + )); + } +} diff --git a/crates/rattler_conda_types/src/lib.rs b/crates/rattler_conda_types/src/lib.rs index 22777ccc4..7ca778939 100644 --- a/crates/rattler_conda_types/src/lib.rs +++ b/crates/rattler_conda_types/src/lib.rs @@ -2,6 +2,7 @@ //! `rattler-conda-types` contains data models for types commonly found within the Conda ecosystem. //! The library itself doesnt provide any functionality besides parsing the data types. +mod build_spec; mod channel; mod channel_data; mod explicit_environment_spec; From f9481601dd32989d8617440691228bd7b13355e7 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Mon, 18 Sep 2023 09:31:52 +0200 Subject: [PATCH 2/6] refactor: improve formatting (#343) Adds some formatting improvements for solvables and out logging. Also adds: - Additional tests - Reformats some of the insta tests to make them more clear (e.g. `9` becomes `foo=9`). --- crates/rattler_libsolv_rs/Cargo.toml | 1 + crates/rattler_libsolv_rs/src/id.rs | 11 ++ crates/rattler_libsolv_rs/src/problem.rs | 5 +- crates/rattler_libsolv_rs/src/solvable.rs | 24 ++- .../rattler_libsolv_rs/src/solver/clause.rs | 10 +- crates/rattler_libsolv_rs/src/solver/mod.rs | 142 ++++++++++++------ ...libsolv_rs__solver__test__missing_dep.snap | 6 + ...olv_rs__solver__test__no_backtracking.snap | 7 + ...__solver__test__resolve_with_conflict.snap | 6 +- ..._rs__solver__test__unsat_constrains_2.snap | 27 ++++ 10 files changed, 182 insertions(+), 57 deletions(-) create mode 100644 crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__missing_dep.snap create mode 100644 crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__no_backtracking.snap create mode 100644 crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains_2.snap diff --git a/crates/rattler_libsolv_rs/Cargo.toml b/crates/rattler_libsolv_rs/Cargo.toml index c8da21450..98d9a45f1 100644 --- a/crates/rattler_libsolv_rs/Cargo.toml +++ b/crates/rattler_libsolv_rs/Cargo.toml @@ -19,3 +19,4 @@ elsa = "1.9.0" [dev-dependencies] insta = "1.31.0" indexmap = "2.0.0" +tracing-test = "0.2.4" diff --git a/crates/rattler_libsolv_rs/src/id.rs b/crates/rattler_libsolv_rs/src/id.rs index f552c7121..a4e53e39a 100644 --- a/crates/rattler_libsolv_rs/src/id.rs +++ b/crates/rattler_libsolv_rs/src/id.rs @@ -1,4 +1,7 @@ use crate::arena::ArenaId; +use crate::solvable::DisplaySolvable; +use crate::{PackageName, Pool, VersionSet}; +use std::fmt::Display; /// The id associated to a package name #[repr(transparent)] @@ -51,6 +54,14 @@ impl SolvableId { pub(crate) fn is_null(self) -> bool { self.0 == u32::MAX } + + /// Returns an object that enables formatting the solvable. + pub fn display( + self, + pool: &Pool, + ) -> DisplaySolvable { + pool.resolve_internal_solvable(self).display(pool) + } } impl ArenaId for SolvableId { diff --git a/crates/rattler_libsolv_rs/src/problem.rs b/crates/rattler_libsolv_rs/src/problem.rs index 44a641fc7..bd6ab5c23 100644 --- a/crates/rattler_libsolv_rs/src/problem.rs +++ b/crates/rattler_libsolv_rs/src/problem.rs @@ -304,7 +304,7 @@ impl ProblemGraph { } } - pool.resolve_internal_solvable(solvable_2).to_string() + solvable_2.display(pool).to_string() } ProblemNode::UnresolvedDependency => "unresolved".to_string(), }; @@ -312,7 +312,8 @@ impl ProblemGraph { write!( f, "\"{}\" -> \"{}\"[color={color}, label=\"{label}\"];", - solvable, target + solvable.display(pool), + target )?; } } diff --git a/crates/rattler_libsolv_rs/src/solvable.rs b/crates/rattler_libsolv_rs/src/solvable.rs index f6ad63768..896c6feb5 100644 --- a/crates/rattler_libsolv_rs/src/solvable.rs +++ b/crates/rattler_libsolv_rs/src/solvable.rs @@ -1,5 +1,6 @@ use crate::id::NameId; +use crate::{PackageName, Pool, VersionSet}; use std::fmt::{Display, Formatter}; /// A solvable represents a single candidate of a package. @@ -57,13 +58,30 @@ impl InternalSolvable { pub fn solvable(&self) -> &Solvable { self.get_solvable().expect("unexpected root solvable") } + + pub fn display<'pool, VS: VersionSet, N: PackageName + Display>( + &'pool self, + pool: &'pool Pool, + ) -> DisplaySolvable<'pool, VS, N> { + DisplaySolvable { + pool, + solvable: self, + } + } } -impl Display for InternalSolvable { +pub struct DisplaySolvable<'pool, VS: VersionSet, N: PackageName> { + pool: &'pool Pool, + solvable: &'pool InternalSolvable, +} + +impl<'pool, VS: VersionSet, N: PackageName + Display> Display for DisplaySolvable<'pool, VS, N> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match &self.inner { + match &self.solvable.inner { SolvableInner::Root => write!(f, ""), - SolvableInner::Package(p) => write!(f, "{}", &p.inner), + SolvableInner::Package(p) => { + write!(f, "{}={}", self.pool.resolve_package_name(p.name), &p.inner) + } } } } diff --git a/crates/rattler_libsolv_rs/src/solver/clause.rs b/crates/rattler_libsolv_rs/src/solver/clause.rs index dfc2af993..828936877 100644 --- a/crates/rattler_libsolv_rs/src/solver/clause.rs +++ b/crates/rattler_libsolv_rs/src/solver/clause.rs @@ -490,15 +490,15 @@ impl Debug for ClauseDebug<'_, VS, N> write!( f, "{} requires {match_spec}", - self.pool.resolve_internal_solvable(solvable_id) + solvable_id.display(self.pool), ) } Clause::Constrains(s1, s2, vset_id) => { write!( f, "{} excludes {} by {}", - self.pool.resolve_internal_solvable(s1), - self.pool.resolve_internal_solvable(s2), + s1.display(self.pool), + s2.display(self.pool), self.pool.resolve_version_set(vset_id) ) } @@ -506,8 +506,8 @@ impl Debug for ClauseDebug<'_, VS, N> write!( f, "{} is locked, so {} is forbidden", - self.pool.resolve_internal_solvable(locked), - self.pool.resolve_internal_solvable(forbidden) + locked.display(self.pool), + forbidden.display(self.pool) ) } Clause::ForbidMultipleInstances(s1, _) => { diff --git a/crates/rattler_libsolv_rs/src/solver/mod.rs b/crates/rattler_libsolv_rs/src/solver/mod.rs index fac3b7a4a..470c01623 100644 --- a/crates/rattler_libsolv_rs/src/solver/mod.rs +++ b/crates/rattler_libsolv_rs/src/solver/mod.rs @@ -11,7 +11,6 @@ use crate::{ }; use elsa::{FrozenMap, FrozenVec}; -use itertools::Itertools; use std::{collections::HashSet, fmt::Display, marker::PhantomData}; use clause::{Clause, ClauseState, Literal}; @@ -454,10 +453,7 @@ impl> Sol .map_err(|_| clause_id)?; if decided { - tracing::info!( - "Set {} = false", - self.pool().resolve_internal_solvable(solvable_id) - ); + tracing::info!("Set {} = false", solvable_id.display(self.pool())); } } } @@ -552,9 +548,9 @@ impl> Sol level += 1; tracing::info!( - "=== Install {} at level {level} (required by {})", - self.pool().resolve_internal_solvable(solvable), - self.pool().resolve_internal_solvable(required_by), + "╤══ Install {} at level {level} (required by {})", + solvable.display(self.pool()), + required_by.display(self.pool()), ); self.decision_tracker @@ -565,22 +561,22 @@ impl> Sol let r = self.propagate(level); let Err((conflicting_solvable, attempted_value, conflicting_clause)) = r else { // Propagation succeeded - tracing::info!("=== Propagation succeeded"); + tracing::info!("╘══ Propagation succeeded"); break; }; { tracing::info!( - "=== Propagation conflicted: could not set {solvable} to {attempted_value}", - solvable = self.pool().resolve_internal_solvable(conflicting_solvable) + "├─ Propagation conflicted: could not set {solvable} to {attempted_value}", + solvable = solvable.display(self.pool()) ); tracing::info!( - "During unit propagation for clause: {:?}", + "│ During unit propagation for clause: {:?}", self.clauses[conflicting_clause].debug(self.pool()) ); tracing::info!( - "Previously decided value: {}. Derived from: {:?}", + "│ Previously decided value: {}. Derived from: {:?}", !attempted_value, self.clauses[self .decision_tracker @@ -594,7 +590,7 @@ impl> Sol } if level == 1 { - tracing::info!("=== UNSOLVABLE"); + tracing::info!("╘══ UNSOLVABLE"); for decision in self.decision_tracker.stack() { let clause = &self.clauses[decision.derived_from]; let level = self.decision_tracker.level(decision.solvable_id); @@ -607,7 +603,7 @@ impl> Sol tracing::info!( "* ({level}) {action} {}. Reason: {:?}", - self.pool().resolve_internal_solvable(decision.solvable_id), + decision.solvable_id.display(self.pool()), clause.debug(self.pool()), ); } @@ -619,7 +615,7 @@ impl> Sol self.analyze(level, conflicting_solvable, conflicting_clause); level = new_level; - tracing::info!("=== Backtracked to level {level}"); + tracing::info!("├─ Backtracked to level {level}"); // Optimization: propagate right now, since we know that the clause is a unit clause let decision = literal.satisfying_value(); @@ -630,8 +626,8 @@ impl> Sol ) .expect("bug: solvable was already decided!"); tracing::info!( - "=== Propagate after learn: {} = {decision}", - self.pool().resolve_internal_solvable(literal.solvable_id) + "├─ Propagate after learn: {} = {decision}", + literal.solvable_id.display(self.pool()) ); } @@ -674,8 +670,8 @@ impl> Sol if decided { tracing::info!( - "Propagate assertion {} = {}", - self.pool().resolve_internal_solvable(literal.solvable_id), + "├─ Propagate assertion {} = {}", + literal.solvable_id.display(self.pool()), decision ); } @@ -766,10 +762,8 @@ impl> Sol Clause::ForbidMultipleInstances(..) => {} _ => { tracing::info!( - "Propagate {} = {}. {:?}", - self.provider - .pool() - .resolve_internal_solvable(remaining_watch.solvable_id), + "├─ Propagate {} = {}. {:?}", + remaining_watch.solvable_id.display(self.provider.pool()), remaining_watch.satisfying_value(), clause.debug(self.provider.pool()), ); @@ -983,16 +977,14 @@ impl> Sol self.watches.start_watching(clause, clause_id); } - tracing::info!( - "Learnt disjunction:\n{}", - learnt - .into_iter() - .format_with("\n", |lit, f| f(&format_args!( - "- {}{}", - if lit.negate { "NOT " } else { "" }, - self.pool().resolve_internal_solvable(lit.solvable_id) - ))) - ); + tracing::info!("├─ Learnt disjunction:",); + for lit in learnt { + tracing::info!( + "│ - {}{}", + if lit.negate { "NOT " } else { "" }, + lit.solvable_id.display(self.pool()) + ); + } // Should revert at most to the root level let target_level = back_track_to.max(1); @@ -1289,7 +1281,7 @@ mod test { use std::fmt::Write; let mut buf = String::new(); for &solvable_id in solvables { - writeln!(buf, "{}", pool.resolve_internal_solvable(solvable_id)).unwrap(); + writeln!(buf, "{}", solvable_id.display(pool)).unwrap(); } buf @@ -1413,8 +1405,12 @@ mod test { use std::fmt::Write; let mut display_result = String::new(); for &solvable_id in &solved { - let solvable = solver.pool().resolve_internal_solvable(solvable_id); - writeln!(display_result, "{solvable}").unwrap(); + writeln!( + display_result, + "{solvable}", + solvable = solvable_id.display(solver.pool()) + ) + .unwrap(); } insta::assert_snapshot!(display_result); @@ -1521,8 +1517,8 @@ mod test { let result = transaction_to_string(&solver.pool(), &solved); insta::assert_snapshot!(result, @r###" - 2 - 1 + b=2 + a=1 "###); } // @@ -1554,9 +1550,9 @@ mod test { let result = transaction_to_string(&solver.pool(), &solved); insta::assert_snapshot!(result, @r###" - 2 - 2 - 2 + b=2 + c=2 + a=2 "###); } @@ -1572,8 +1568,8 @@ mod test { let result = transaction_to_string(&solver.pool(), &solved); insta::assert_snapshot!(result, @r###" - 2 - 5 + a=2 + b=5 "###); } @@ -1716,4 +1712,62 @@ mod test { let error = solve_unsat(provider, &["a", "c"]); insta::assert_snapshot!(error); } + + #[test] + #[tracing_test::traced_test] + fn test_unsat_constrains_2() { + let mut provider = BundleBoxProvider::from_packages(&[ + ("a", 1, vec!["b"]), + ("a", 2, vec!["b"]), + ("b", 1, vec!["c 1"]), + ("b", 2, vec!["c 2"]), + ]); + + provider.add_package("c", 1.into(), &vec![], &vec!["a 3"]); + provider.add_package("c", 2.into(), &vec![], &vec!["a 3"]); + let error = solve_unsat(provider, &["a"]); + insta::assert_snapshot!(error); + } + + #[test] + fn test_missing_dep() { + let provider = + BundleBoxProvider::from_packages(&[("a", 2, vec!["missing"]), ("a", 1, vec![])]); + let requirements = provider.requirements(&["a"]); + let mut solver = Solver::new(provider); + let result = match solver.solve(requirements) { + Ok(result) => transaction_to_string(solver.pool(), &result), + Err(problem) => problem + .display_user_friendly(&solver, &DefaultSolvableDisplay) + .to_string(), + }; + insta::assert_snapshot!(result); + } + + #[test] + #[tracing_test::traced_test] + fn test_no_backtracking() { + let provider = BundleBoxProvider::from_packages(&[ + ("quetz-server", 2, vec!["pydantic 10..20"]), + ("quetz-server", 1, vec!["pydantic 0..10"]), + ("pydantic", 1, vec![]), + ("pydantic", 2, vec![]), + ("pydantic", 3, vec![]), + ("pydantic", 4, vec![]), + ("pydantic", 5, vec![]), + ("pydantic", 6, vec![]), + ("pydantic", 7, vec![]), + ("pydantic", 8, vec![]), + ("pydantic", 9, vec![]), + ("pydantic", 10, vec![]), + ("pydantic", 11, vec![]), + ("pydantic", 12, vec![]), + ("pydantic", 13, vec![]), + ("pydantic", 14, vec![]), + ]); + let requirements = provider.requirements(&["quetz-server", "pydantic 0..10"]); + let mut solver = Solver::new(provider); + let solved = solver.solve(requirements).unwrap(); + insta::assert_snapshot!(transaction_to_string(solver.pool(), &solved)); + } } diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__missing_dep.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__missing_dep.snap new file mode 100644 index 000000000..eef54c319 --- /dev/null +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__missing_dep.snap @@ -0,0 +1,6 @@ +--- +source: crates/rattler_libsolv_rs/src/solver/mod.rs +expression: result +--- +a=1 + diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__no_backtracking.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__no_backtracking.snap new file mode 100644 index 000000000..73fc6f210 --- /dev/null +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__no_backtracking.snap @@ -0,0 +1,7 @@ +--- +source: crates/rattler_libsolv_rs/src/solver/mod.rs +expression: "transaction_to_string(solver.pool(), &solved)" +--- +quetz-server=1 +pydantic=9 + 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 bda53b091..e15535f29 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 --- -0 -3 -7 +conflicting=0 +asdf=3 +efgh=7 diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains_2.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains_2.snap new file mode 100644 index 000000000..38b75e9b3 --- /dev/null +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains_2.snap @@ -0,0 +1,27 @@ +--- +source: crates/rattler_libsolv_rs/src/solver/mod.rs +expression: error +--- +The following packages are incompatible +|-- a * cannot be installed because there are no viable options: + |-- a 2 would require + |-- b *, which cannot be installed because there are no viable options: + |-- b 2 would require + |-- c 2..3, which cannot be installed because there are no viable options: + |-- c 2 would constrain + |-- a 3..4 , which conflicts with any installable versions previously reported + |-- b 1 would require + |-- c 1..2, which cannot be installed because there are no viable options: + |-- c 1 would constrain + |-- a 3..4 , which conflicts with any installable versions previously reported + |-- a 1 would require + |-- b *, which cannot be installed because there are no viable options: + |-- b 2 would require + |-- c 2..3, which cannot be installed because there are no viable options: + |-- c 2 would constrain + |-- a 3..4 , which conflicts with any installable versions previously reported + |-- b 1 would require + |-- c 1..2, which cannot be installed because there are no viable options: + |-- c 1 would constrain + |-- a 3..4 , which conflicts with any installable versions previously reported + From 9db4c581b2da4208c50c1e7753706f1887e9ba62 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Mon, 18 Sep 2023 13:27:21 +0200 Subject: [PATCH 3/6] refactor: added range and moved files (#337) Adds a generic bounded/unbounded `Range` (taken from pubgrub, contributed there by me) struct which can be used to easily craft `VersionSet` implementations. I also moved files to the `internal` folder for types that should only be used inside the crate. --------- Co-authored-by: Tim de Jager --- crates/rattler_libsolv_rs/Cargo.toml | 2 + .../src/{ => internal}/arena.rs | 0 .../src/{ => internal}/frozen_copy_map.rs | 0 .../src/{ => internal}/id.rs | 2 +- .../src/{ => internal}/mapping.rs | 4 +- crates/rattler_libsolv_rs/src/internal/mod.rs | 5 + .../src/internal/small_vec.rs | 170 ++++++ crates/rattler_libsolv_rs/src/lib.rs | 21 +- crates/rattler_libsolv_rs/src/pool.rs | 15 +- crates/rattler_libsolv_rs/src/problem.rs | 14 +- crates/rattler_libsolv_rs/src/range.rs | 525 ++++++++++++++++++ crates/rattler_libsolv_rs/src/solvable.rs | 5 +- .../rattler_libsolv_rs/src/solver/clause.rs | 8 +- .../rattler_libsolv_rs/src/solver/decision.rs | 2 +- .../src/solver/decision_map.rs | 2 +- .../src/solver/decision_tracker.rs | 7 +- crates/rattler_libsolv_rs/src/solver/mod.rs | 87 ++- ...olver__test__unsat_after_backtracking.snap | 6 +- ...test__unsat_applies_graph_compression.snap | 6 +- ..._solver__test__unsat_bluesky_conflict.snap | 8 +- ...lv_rs__solver__test__unsat_constrains.snap | 6 +- ..._rs__solver__test__unsat_constrains_2.snap | 16 +- ..._unsat_incompatible_root_requirements.snap | 6 +- ...lver__test__unsat_locked_and_excluded.snap | 4 +- ...__test__unsat_missing_top_level_dep_2.snap | 4 +- ...test__unsat_no_candidates_for_child_1.snap | 4 +- ...test__unsat_no_candidates_for_child_2.snap | 6 +- ...__solver__test__unsat_pubgrub_article.snap | 10 +- .../src/solver/watch_map.rs | 8 +- 29 files changed, 822 insertions(+), 131 deletions(-) rename crates/rattler_libsolv_rs/src/{ => internal}/arena.rs (100%) rename crates/rattler_libsolv_rs/src/{ => internal}/frozen_copy_map.rs (100%) rename crates/rattler_libsolv_rs/src/{ => internal}/id.rs (98%) rename crates/rattler_libsolv_rs/src/{ => internal}/mapping.rs (98%) create mode 100644 crates/rattler_libsolv_rs/src/internal/mod.rs create mode 100644 crates/rattler_libsolv_rs/src/internal/small_vec.rs create mode 100644 crates/rattler_libsolv_rs/src/range.rs diff --git a/crates/rattler_libsolv_rs/Cargo.toml b/crates/rattler_libsolv_rs/Cargo.toml index 98d9a45f1..2d4e53cbb 100644 --- a/crates/rattler_libsolv_rs/Cargo.toml +++ b/crates/rattler_libsolv_rs/Cargo.toml @@ -15,8 +15,10 @@ itertools = "0.11.0" petgraph = "0.6.4" tracing = "0.1.37" elsa = "1.9.0" +serde = { version = "1.0", features = ["derive"], optional = true } [dev-dependencies] insta = "1.31.0" indexmap = "2.0.0" +proptest = "1.2.0" tracing-test = "0.2.4" diff --git a/crates/rattler_libsolv_rs/src/arena.rs b/crates/rattler_libsolv_rs/src/internal/arena.rs similarity index 100% rename from crates/rattler_libsolv_rs/src/arena.rs rename to crates/rattler_libsolv_rs/src/internal/arena.rs diff --git a/crates/rattler_libsolv_rs/src/frozen_copy_map.rs b/crates/rattler_libsolv_rs/src/internal/frozen_copy_map.rs similarity index 100% rename from crates/rattler_libsolv_rs/src/frozen_copy_map.rs rename to crates/rattler_libsolv_rs/src/internal/frozen_copy_map.rs diff --git a/crates/rattler_libsolv_rs/src/id.rs b/crates/rattler_libsolv_rs/src/internal/id.rs similarity index 98% rename from crates/rattler_libsolv_rs/src/id.rs rename to crates/rattler_libsolv_rs/src/internal/id.rs index a4e53e39a..bfeef2215 100644 --- a/crates/rattler_libsolv_rs/src/id.rs +++ b/crates/rattler_libsolv_rs/src/internal/id.rs @@ -1,4 +1,4 @@ -use crate::arena::ArenaId; +use crate::internal::arena::ArenaId; use crate::solvable::DisplaySolvable; use crate::{PackageName, Pool, VersionSet}; use std::fmt::Display; diff --git a/crates/rattler_libsolv_rs/src/mapping.rs b/crates/rattler_libsolv_rs/src/internal/mapping.rs similarity index 98% rename from crates/rattler_libsolv_rs/src/mapping.rs rename to crates/rattler_libsolv_rs/src/internal/mapping.rs index 8565d989d..5f5c17aaf 100644 --- a/crates/rattler_libsolv_rs/src/mapping.rs +++ b/crates/rattler_libsolv_rs/src/internal/mapping.rs @@ -1,4 +1,4 @@ -use crate::arena::ArenaId; +use crate::internal::arena::ArenaId; use std::cmp; use std::iter::FusedIterator; use std::marker::PhantomData; @@ -179,7 +179,7 @@ impl<'a, TId: ArenaId, TValue> FusedIterator for MappingIter<'a, TId, TValue> {} #[cfg(test)] mod tests { - use crate::arena::ArenaId; + use crate::internal::arena::ArenaId; struct Id { id: usize, diff --git a/crates/rattler_libsolv_rs/src/internal/mod.rs b/crates/rattler_libsolv_rs/src/internal/mod.rs new file mode 100644 index 000000000..7f11deaaa --- /dev/null +++ b/crates/rattler_libsolv_rs/src/internal/mod.rs @@ -0,0 +1,5 @@ +pub mod arena; +pub mod frozen_copy_map; +pub mod id; +pub mod mapping; +pub mod small_vec; diff --git a/crates/rattler_libsolv_rs/src/internal/small_vec.rs b/crates/rattler_libsolv_rs/src/internal/small_vec.rs new file mode 100644 index 000000000..74ea86fb2 --- /dev/null +++ b/crates/rattler_libsolv_rs/src/internal/small_vec.rs @@ -0,0 +1,170 @@ +// This file was originally taken from: +// +// SPDX-License-Identifier: MPL-2.0 + +use std::{ + fmt, + hash::{Hash, Hasher}, + ops::Deref, +}; + +#[derive(Clone)] +pub enum SmallVec { + Empty, + One([T; 1]), + Two([T; 2]), + Flexible(Vec), +} + +impl SmallVec { + pub fn empty() -> Self { + Self::Empty + } + + pub fn one(t: T) -> Self { + Self::One([t]) + } + + pub fn as_slice(&self) -> &[T] { + match self { + Self::Empty => &[], + Self::One(v) => v, + Self::Two(v) => v, + Self::Flexible(v) => v, + } + } + + pub fn push(&mut self, new: T) { + *self = match std::mem::take(self) { + Self::Empty => Self::One([new]), + Self::One([v1]) => Self::Two([v1, new]), + Self::Two([v1, v2]) => Self::Flexible(vec![v1, v2, new]), + Self::Flexible(mut v) => { + v.push(new); + Self::Flexible(v) + } + } + } + + pub fn pop(&mut self) -> Option { + match std::mem::take(self) { + Self::Empty => None, + Self::One([v1]) => { + *self = Self::Empty; + Some(v1) + } + Self::Two([v1, v2]) => { + *self = Self::One([v1]); + Some(v2) + } + Self::Flexible(mut v) => { + let out = v.pop(); + *self = Self::Flexible(v); + out + } + } + } + + pub fn clear(&mut self) { + if let Self::Flexible(mut v) = std::mem::take(self) { + v.clear(); + *self = Self::Flexible(v); + } // else: self already eq Empty from the take + } + + pub fn iter(&self) -> std::slice::Iter<'_, T> { + self.as_slice().iter() + } +} + +impl Default for SmallVec { + fn default() -> Self { + Self::Empty + } +} + +impl Deref for SmallVec { + type Target = [T]; + + fn deref(&self) -> &Self::Target { + self.as_slice() + } +} + +impl<'a, T> IntoIterator for &'a SmallVec { + type Item = &'a T; + + type IntoIter = std::slice::Iter<'a, T>; + + fn into_iter(self) -> Self::IntoIter { + self.iter() + } +} + +impl Eq for SmallVec {} + +impl Hash for SmallVec { + fn hash(&self, state: &mut H) { + self.as_slice().hash(state) + } +} + +impl PartialEq for SmallVec { + fn eq(&self, other: &Self) -> bool { + self.as_slice() == other.as_slice() + } +} + +impl fmt::Debug for SmallVec { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_slice().fmt(f) + } +} + +#[cfg(feature = "serde")] +impl serde::Serialize for SmallVec { + fn serialize(&self, s: S) -> Result { + serde::Serialize::serialize(self.as_slice(), s) + } +} + +#[cfg(feature = "serde")] +impl<'de, T: serde::Deserialize<'de>> serde::Deserialize<'de> for SmallVec { + fn deserialize>(d: D) -> Result { + let items: Vec = serde::Deserialize::deserialize(d)?; + + let mut v = Self::empty(); + for item in items { + v.push(item); + } + Ok(v) + } +} + +// TESTS ####################################################################### + +#[cfg(test)] +pub mod tests { + use super::*; + use proptest::prelude::*; + + proptest! { + #[test] + fn push_and_pop(comands: Vec>) { + let mut v = vec![]; + let mut sv = SmallVec::Empty; + for comand in comands { + match comand { + Some(i) => { + v.push(i); + sv.push(i); + } + None => { + assert_eq!(v.pop(), sv.pop()); + } + } + assert_eq!(v.as_slice(), sv.as_slice()); + } + } + } +} diff --git a/crates/rattler_libsolv_rs/src/lib.rs b/crates/rattler_libsolv_rs/src/lib.rs index 434de9d0b..b9af7d4dc 100644 --- a/crates/rattler_libsolv_rs/src/lib.rs +++ b/crates/rattler_libsolv_rs/src/lib.rs @@ -9,32 +9,29 @@ //! comments. #![deny(missing_docs)] - -mod arena; -mod frozen_copy_map; -mod id; -mod mapping; +pub(crate) mod internal; mod pool; pub mod problem; +pub mod range; mod solvable; mod solver; -pub use id::{NameId, SolvableId, VersionSetId}; use itertools::Itertools; +use std::{ + fmt::{Debug, Display}, + hash::Hash, +}; + +pub use internal::id::{NameId, SolvableId, VersionSetId}; pub use pool::Pool; pub use solvable::Solvable; pub use solver::Solver; -use std::fmt::{Debug, Display}; -use std::hash::Hash; - -pub(crate) use frozen_copy_map::FrozenCopyMap; -pub use mapping::Mapping; /// The solver is based around the fact that for for every package name we are trying to find a /// single variant. Variants are grouped by their respective package name. A package name is /// anything that we can compare and hash for uniqueness checks. /// -/// For most implementations a package name can simply be a string. But in some more advanced cases +/// For most implementations a package name can simply be a String. But in some more advanced cases /// like when a single package can have additive features it can make sense to create a custom type. /// /// A blanket trait implementation is provided for any type that implements [`Eq`] and [`Hash`]. diff --git a/crates/rattler_libsolv_rs/src/pool.rs b/crates/rattler_libsolv_rs/src/pool.rs index 27157c6f6..2bd00b65d 100644 --- a/crates/rattler_libsolv_rs/src/pool.rs +++ b/crates/rattler_libsolv_rs/src/pool.rs @@ -1,16 +1,21 @@ use std::fmt::{Display, Formatter}; -use crate::arena::Arena; -use crate::id::{NameId, SolvableId, VersionSetId}; -use crate::solvable::{InternalSolvable, Solvable}; -use crate::FrozenCopyMap; -use crate::{PackageName, VersionSet}; +use crate::{ + internal::{ + arena::Arena, + frozen_copy_map::FrozenCopyMap, + id::{NameId, SolvableId, VersionSetId}, + }, + solvable::{InternalSolvable, Solvable}, + PackageName, VersionSet, +}; /// A pool that stores data related to the available packages. /// /// A pool never releases its memory until it is dropped. References returned by the pool will /// remain valid for the lifetime of the pool. This allows inserting into the pool without requiring /// a mutable reference to the pool. +/// This is what we refer to as `Frozen` data can be added but old data can never be removed or mutated. pub struct Pool { /// All the solvables that have been registered pub(crate) solvables: Arena>, diff --git a/crates/rattler_libsolv_rs/src/problem.rs b/crates/rattler_libsolv_rs/src/problem.rs index bd6ab5c23..7fa568ccb 100644 --- a/crates/rattler_libsolv_rs/src/problem.rs +++ b/crates/rattler_libsolv_rs/src/problem.rs @@ -12,11 +12,12 @@ use petgraph::graph::{DiGraph, EdgeIndex, EdgeReference, NodeIndex}; use petgraph::visit::{Bfs, DfsPostOrder, EdgeRef}; use petgraph::Direction; -use crate::id::{ClauseId, SolvableId, VersionSetId}; -use crate::pool::Pool; -use crate::solver::clause::Clause; -use crate::solver::Solver; -use crate::{DependencyProvider, PackageName, SolvableDisplay, VersionSet}; +use crate::{ + internal::id::{ClauseId, SolvableId, VersionSetId}, + pool::Pool, + solver::{clause::Clause, Solver}, + DependencyProvider, PackageName, SolvableDisplay, VersionSet, +}; /// Represents the cause of the solver being unable to find a solution #[derive(Debug)] @@ -320,6 +321,7 @@ impl ProblemGraph { write!(f, "}}") } + /// Simplifies and collapses nodes so that these can be considered the same candidate fn simplify( &self, pool: &Pool, @@ -369,8 +371,6 @@ 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 m in maybe_merge.into_values() { if m.len() > 1 { let m = Rc::new(MergedProblemNode { diff --git a/crates/rattler_libsolv_rs/src/range.rs b/crates/rattler_libsolv_rs/src/range.rs new file mode 100644 index 000000000..a08e1adc8 --- /dev/null +++ b/crates/rattler_libsolv_rs/src/range.rs @@ -0,0 +1,525 @@ +// This file was originally taken from: +// +// SPDX-License-Identifier: MPL-2.0 + +//! Ranges are constraints defining sets of versions. They are a convenience struct that implements +//! [`crate::VersionSet`] for any version type that implements [`Ord`] + [`Clone`]. +//! +//! Concretely, those constraints correspond to any set of versions representable as the +//! concatenation, union, and complement of the ranges building blocks. +//! +//! Those building blocks are: +//! - [empty()](Range::empty): the empty set +//! - [full()](Range::full): the set of all possible versions +//! - [singleton(v)](Range::singleton): the set containing only the version v +//! - [higher_than(v)](Range::higher_than): the set defined by `v <= versions` +//! - [strictly_higher_than(v)](Range::strictly_higher_than): the set defined by `v < versions` +//! - [lower_than(v)](Range::lower_than): the set defined by `versions <= v` +//! - [strictly_lower_than(v)](Range::strictly_lower_than): the set defined by `versions < v` +//! - [between(v1, v2)](Range::between): the set defined by `v1 <= versions < v2` +//! +//! Ranges can be created from any type that implements [`Ord`] + [`Clone`]. + +use crate::{internal::small_vec::SmallVec, VersionSet}; +use std::{ + fmt::{Debug, Display, Formatter}, + hash::Hash, + ops::Bound::{self, Excluded, Included, Unbounded}, + ops::RangeBounds, +}; + +/// A Range represents multiple intervals of a continuous range of monotone increasing +/// values. +#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[cfg_attr(feature = "serde", serde(transparent))] +pub struct Range { + segments: SmallVec>, +} + +type Interval = (Bound, Bound); + +impl Range { + /// Empty set of versions. + pub fn empty() -> Self { + Self { + segments: SmallVec::empty(), + } + } + + /// Set of all possible versions + pub fn full() -> Self { + Self { + segments: SmallVec::one((Unbounded, Unbounded)), + } + } + + /// Set of all versions higher or equal to some version + pub fn higher_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one((Included(v.into()), Unbounded)), + } + } + + /// Set of all versions higher to some version + pub fn strictly_higher_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one((Excluded(v.into()), Unbounded)), + } + } + + /// Set of all versions lower to some version + pub fn strictly_lower_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one((Unbounded, Excluded(v.into()))), + } + } + + /// Set of all versions lower or equal to some version + pub fn lower_than(v: impl Into) -> Self { + Self { + segments: SmallVec::one((Unbounded, Included(v.into()))), + } + } + + /// Set of versions greater or equal to `v1` but less than `v2`. + pub fn between(v1: impl Into, v2: impl Into) -> Self { + Self { + segments: SmallVec::one((Included(v1.into()), Excluded(v2.into()))), + } + } +} + +impl Range { + /// Set containing exactly one version + pub fn singleton(v: impl Into) -> Self { + let v = v.into(); + Self { + segments: SmallVec::one((Included(v.clone()), Included(v))), + } + } + + /// Returns the complement of this Range. + pub fn complement(&self) -> Self { + match self.segments.first() { + // Complement of ∅ is ∞ + None => Self::full(), + + // Complement of ∞ is ∅ + Some((Unbounded, Unbounded)) => Self::empty(), + + // First high bound is +∞ + Some((Included(v), Unbounded)) => Self::strictly_lower_than(v.clone()), + Some((Excluded(v), Unbounded)) => Self::lower_than(v.clone()), + + Some((Unbounded, Included(v))) => { + Self::negate_segments(Excluded(v.clone()), &self.segments[1..]) + } + Some((Unbounded, Excluded(v))) => { + Self::negate_segments(Included(v.clone()), &self.segments[1..]) + } + Some((Included(_), Included(_))) + | Some((Included(_), Excluded(_))) + | Some((Excluded(_), Included(_))) + | Some((Excluded(_), Excluded(_))) => Self::negate_segments(Unbounded, &self.segments), + } + } + + /// Helper function performing the negation of intervals in segments. + fn negate_segments(start: Bound, segments: &[Interval]) -> Self { + let mut complement_segments: SmallVec> = SmallVec::empty(); + let mut start = start; + for (v1, v2) in segments { + complement_segments.push(( + start, + match v1 { + Included(v) => Excluded(v.clone()), + Excluded(v) => Included(v.clone()), + Unbounded => unreachable!(), + }, + )); + start = match v2 { + Included(v) => Excluded(v.clone()), + Excluded(v) => Included(v.clone()), + Unbounded => Unbounded, + } + } + if !matches!(start, Unbounded) { + complement_segments.push((start, Unbounded)); + } + + Self { + segments: complement_segments, + } + } +} + +impl Range { + /// Convert to something that can be used with + /// [BTreeMap::range](std::collections::BTreeMap::range). + /// All versions contained in self, will be in the output, + /// but there may be versions in the output that are not contained in self. + /// Returns None if the range is empty. + pub fn bounding_range(&self) -> Option<(Bound<&V>, Bound<&V>)> { + self.segments.first().map(|(start, _)| { + let end = self + .segments + .last() + .expect("if there is a first element, there must be a last element"); + (bound_as_ref(start), bound_as_ref(&end.1)) + }) + } + + /// Returns true if the this Range contains the specified value. + pub fn contains(&self, v: &V) -> bool { + for segment in self.segments.iter() { + if match segment { + (Unbounded, Unbounded) => true, + (Unbounded, Included(end)) => v <= end, + (Unbounded, Excluded(end)) => v < end, + (Included(start), Unbounded) => v >= start, + (Included(start), Included(end)) => v >= start && v <= end, + (Included(start), Excluded(end)) => v >= start && v < end, + (Excluded(start), Unbounded) => v > start, + (Excluded(start), Included(end)) => v > start && v <= end, + (Excluded(start), Excluded(end)) => v > start && v < end, + } { + return true; + } + } + false + } + + /// Construct a simple range from anything that impls [RangeBounds] like `v1..v2`. + pub fn from_range_bounds(bounds: R) -> Self + where + R: RangeBounds, + IV: Clone + Into, + { + let start = match bounds.start_bound() { + Included(v) => Included(v.clone().into()), + Excluded(v) => Excluded(v.clone().into()), + Unbounded => Unbounded, + }; + let end = match bounds.end_bound() { + Included(v) => Included(v.clone().into()), + Excluded(v) => Excluded(v.clone().into()), + Unbounded => Unbounded, + }; + if valid_segment(&start, &end) { + Self { + segments: SmallVec::one((start, end)), + } + } else { + Self::empty() + } + } + + fn check_invariants(self) -> Self { + if cfg!(debug_assertions) { + for p in self.segments.as_slice().windows(2) { + match (&p[0].1, &p[1].0) { + (Included(l_end), Included(r_start)) => assert!(l_end < r_start), + (Included(l_end), Excluded(r_start)) => assert!(l_end < r_start), + (Excluded(l_end), Included(r_start)) => assert!(l_end < r_start), + (Excluded(l_end), Excluded(r_start)) => assert!(l_end <= r_start), + (_, Unbounded) => panic!(), + (Unbounded, _) => panic!(), + } + } + for (s, e) in self.segments.iter() { + assert!(valid_segment(s, e)); + } + } + self + } +} + +/// Implementation of [`Bound::as_ref`] which is currently marked as unstable. +fn bound_as_ref(bound: &Bound) -> Bound<&V> { + match bound { + Included(v) => Included(v), + Excluded(v) => Excluded(v), + Unbounded => Unbounded, + } +} + +fn valid_segment(start: &Bound, end: &Bound) -> bool { + match (start, end) { + (Included(s), Included(e)) => s <= e, + (Included(s), Excluded(e)) => s < e, + (Excluded(s), Included(e)) => s < e, + (Excluded(s), Excluded(e)) => s < e, + (Unbounded, _) | (_, Unbounded) => true, + } +} + +impl Range { + /// Computes the union of this `Range` and another. + pub fn union(&self, other: &Self) -> Self { + self.complement() + .intersection(&other.complement()) + .complement() + .check_invariants() + } + + /// Computes the intersection of two sets of versions. + pub fn intersection(&self, other: &Self) -> Self { + let mut segments: SmallVec> = SmallVec::empty(); + let mut left_iter = self.segments.iter().peekable(); + let mut right_iter = other.segments.iter().peekable(); + + while let (Some((left_start, left_end)), Some((right_start, right_end))) = + (left_iter.peek(), right_iter.peek()) + { + let start = match (left_start, right_start) { + (Included(l), Included(r)) => Included(std::cmp::max(l, r)), + (Excluded(l), Excluded(r)) => Excluded(std::cmp::max(l, r)), + + (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if i <= e => Excluded(e), + (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if e < i => Included(i), + (s, Unbounded) | (Unbounded, s) => bound_as_ref(s), + _ => unreachable!(), + } + .cloned(); + let end = match (left_end, right_end) { + (Included(l), Included(r)) => Included(std::cmp::min(l, r)), + (Excluded(l), Excluded(r)) => Excluded(std::cmp::min(l, r)), + + (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if i >= e => Excluded(e), + (Included(i), Excluded(e)) | (Excluded(e), Included(i)) if e > i => Included(i), + (s, Unbounded) | (Unbounded, s) => bound_as_ref(s), + _ => unreachable!(), + } + .cloned(); + left_iter.next_if(|(_, e)| e == &end); + right_iter.next_if(|(_, e)| e == &end); + if valid_segment(&start, &end) { + segments.push((start, end)) + } + } + + Self { segments }.check_invariants() + } +} + +impl VersionSet for Range { + type V = T; + + fn contains(&self, v: &Self::V) -> bool { + Range::contains(self, v) + } +} + +impl Display for Range { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + if self.segments.is_empty() { + write!(f, "∅")?; + } else { + for (idx, segment) in self.segments.iter().enumerate() { + if idx > 0 { + write!(f, ", ")?; + } + match segment { + (Unbounded, Unbounded) => write!(f, "*")?, + (Unbounded, Included(v)) => write!(f, "<={v}")?, + (Unbounded, Excluded(v)) => write!(f, "<{v}")?, + (Included(v), Unbounded) => write!(f, ">={v}")?, + (Included(v), Included(b)) => { + if v == b { + write!(f, "{v}")? + } else { + write!(f, ">={v},<={b}")? + } + } + (Included(v), Excluded(b)) => write!(f, ">={v}, <{b}")?, + (Excluded(v), Unbounded) => write!(f, ">{v}")?, + (Excluded(v), Included(b)) => write!(f, ">{v}, <={b}")?, + (Excluded(v), Excluded(b)) => write!(f, ">{v}, <{b}")?, + }; + } + } + Ok(()) + } +} + +#[cfg(test)] +pub mod tests { + use proptest::prelude::*; + + use super::*; + + /// Generate version sets from a random vector of deltas between bounds. + /// Each bound is randomly inclusive or exclusive. + pub fn strategy() -> impl Strategy> { + ( + any::(), + prop::collection::vec(any::<(u32, bool)>(), 1..10), + ) + .prop_map(|(start_unbounded, deltas)| { + let mut start = if start_unbounded { + Some(Unbounded) + } else { + None + }; + let mut largest: u32 = 0; + let mut last_bound_was_inclusive = false; + let mut segments = SmallVec::Empty; + for (delta, inclusive) in deltas { + // Add the offset to the current bound + largest = match largest.checked_add(delta) { + Some(s) => s, + None => { + // Skip this offset, if it would result in a too large bound. + continue; + } + }; + + let current_bound = if inclusive { + Included(largest) + } else { + Excluded(largest) + }; + + // If we already have a start bound, the next offset defines the complete range. + // If we don't have a start bound, we have to generate one. + if let Some(start_bound) = start.take() { + // If the delta from the start bound is 0, the only authorized configuration is + // Included(x), Included(x) + if delta == 0 && !(matches!(start_bound, Included(_)) && inclusive) { + start = Some(start_bound); + continue; + } + last_bound_was_inclusive = inclusive; + segments.push((start_bound, current_bound)); + } else { + // If the delta from the end bound of the last range is 0 and + // any of the last ending or current starting bound is inclusive, + // we skip the delta because they basically overlap. + if delta == 0 && (last_bound_was_inclusive || inclusive) { + continue; + } + start = Some(current_bound); + } + } + + // If we still have a start bound, but didn't have enough deltas to complete another + // segment, we add an unbounded upperbound. + if let Some(start_bound) = start { + segments.push((start_bound, Unbounded)); + } + + return Range { segments }.check_invariants(); + }) + } + + fn version_strat() -> impl Strategy { + any::() + } + + proptest! { + + // Testing negate ---------------------------------- + + #[test] + fn negate_is_different(range in strategy()) { + assert_ne!(range.complement(), range); + } + + #[test] + fn double_negate_is_identity(range in strategy()) { + assert_eq!(range.complement().complement(), range); + } + + #[test] + fn negate_contains_opposite(range in strategy(), version in version_strat()) { + assert_ne!(range.contains(&version), range.complement().contains(&version)); + } + + // Testing intersection ---------------------------- + + #[test] + fn intersection_is_symmetric(r1 in strategy(), r2 in strategy()) { + assert_eq!(r1.intersection(&r2), r2.intersection(&r1)); + } + + #[test] + fn intersection_with_any_is_identity(range in strategy()) { + assert_eq!(Range::full().intersection(&range), range); + } + + #[test] + fn intersection_with_none_is_none(range in strategy()) { + assert_eq!(Range::empty().intersection(&range), Range::empty()); + } + + #[test] + fn intersection_is_idempotent(r1 in strategy(), r2 in strategy()) { + assert_eq!(r1.intersection(&r2).intersection(&r2), r1.intersection(&r2)); + } + + #[test] + fn intersection_is_associative(r1 in strategy(), r2 in strategy(), r3 in strategy()) { + assert_eq!(r1.intersection(&r2).intersection(&r3), r1.intersection(&r2.intersection(&r3))); + } + + #[test] + fn intesection_of_complements_is_none(range in strategy()) { + assert_eq!(range.complement().intersection(&range), Range::empty()); + } + + #[test] + fn intesection_contains_both(r1 in strategy(), r2 in strategy(), version in version_strat()) { + assert_eq!(r1.intersection(&r2).contains(&version), r1.contains(&version) && r2.contains(&version)); + } + + // Testing union ----------------------------------- + + #[test] + fn union_of_complements_is_any(range in strategy()) { + assert_eq!(range.complement().union(&range), Range::full()); + } + + #[test] + fn union_contains_either(r1 in strategy(), r2 in strategy(), version in version_strat()) { + assert_eq!(r1.union(&r2).contains(&version), r1.contains(&version) || r2.contains(&version)); + } + + // Testing contains -------------------------------- + + #[test] + fn always_contains_exact(version in version_strat()) { + assert!(Range::singleton(version).contains(&version)); + } + + #[test] + fn contains_negation(range in strategy(), version in version_strat()) { + assert_ne!(range.contains(&version), range.complement().contains(&version)); + } + + #[test] + fn contains_intersection(range in strategy(), version in version_strat()) { + assert_eq!(range.contains(&version), range.intersection(&Range::singleton(version)) != Range::empty()); + } + + #[test] + fn contains_bounding_range(range in strategy(), version in version_strat()) { + if range.contains(&version) { + assert!(range.bounding_range().map(|b| b.contains(&version)).unwrap_or(false)); + } + } + + #[test] + fn from_range_bounds(range in any::<(Bound, Bound)>(), version in version_strat()) { + let rv: Range<_> = Range::from_range_bounds(range); + assert_eq!(range.contains(&version), rv.contains(&version)); + } + + #[test] + fn from_range_bounds_round_trip(range in any::<(Bound, Bound)>()) { + let rv: Range = Range::from_range_bounds(range); + let rv2: Range = rv.bounding_range().map(Range::from_range_bounds::<_, u32>).unwrap_or_else(Range::empty); + assert_eq!(rv, rv2); + } + } +} diff --git a/crates/rattler_libsolv_rs/src/solvable.rs b/crates/rattler_libsolv_rs/src/solvable.rs index 896c6feb5..3b8a6c135 100644 --- a/crates/rattler_libsolv_rs/src/solvable.rs +++ b/crates/rattler_libsolv_rs/src/solvable.rs @@ -1,9 +1,12 @@ -use crate::id::NameId; +use crate::internal::id::NameId; use crate::{PackageName, Pool, VersionSet}; use std::fmt::{Display, Formatter}; /// A solvable represents a single candidate of a package. +/// This is type is generic on `V` which can be supplied by the user. In most cases this is going +/// to be something like a record that contains the version of the package and other metadata. +/// A solvable is always associated with a [`NameId`], which is an interned name in the [`Pool`]. pub struct Solvable { pub(crate) inner: V, pub(crate) name: NameId, diff --git a/crates/rattler_libsolv_rs/src/solver/clause.rs b/crates/rattler_libsolv_rs/src/solver/clause.rs index 828936877..2569f1e58 100644 --- a/crates/rattler_libsolv_rs/src/solver/clause.rs +++ b/crates/rattler_libsolv_rs/src/solver/clause.rs @@ -1,6 +1,8 @@ use crate::{ - arena::Arena, - id::{ClauseId, LearntClauseId, SolvableId, VersionSetId}, + internal::{ + arena::Arena, + id::{ClauseId, LearntClauseId, SolvableId, VersionSetId}, + }, pool::Pool, solver::decision_map::DecisionMap, PackageName, VersionSet, @@ -526,7 +528,7 @@ impl Debug for ClauseDebug<'_, VS, N> #[cfg(test)] mod test { use super::*; - use crate::arena::ArenaId; + use crate::internal::arena::ArenaId; fn clause(next_clauses: [ClauseId; 2], watched_solvables: [SolvableId; 2]) -> ClauseState { ClauseState { diff --git a/crates/rattler_libsolv_rs/src/solver/decision.rs b/crates/rattler_libsolv_rs/src/solver/decision.rs index 57fd0f244..b550d038e 100644 --- a/crates/rattler_libsolv_rs/src/solver/decision.rs +++ b/crates/rattler_libsolv_rs/src/solver/decision.rs @@ -1,4 +1,4 @@ -use crate::id::{ClauseId, SolvableId}; +use crate::internal::id::{ClauseId, SolvableId}; /// Represents an assignment to a variable #[derive(Copy, Clone, Eq, PartialEq)] diff --git a/crates/rattler_libsolv_rs/src/solver/decision_map.rs b/crates/rattler_libsolv_rs/src/solver/decision_map.rs index beb466597..6b97006ac 100644 --- a/crates/rattler_libsolv_rs/src/solver/decision_map.rs +++ b/crates/rattler_libsolv_rs/src/solver/decision_map.rs @@ -1,4 +1,4 @@ -use crate::{arena::ArenaId, id::SolvableId}; +use crate::internal::{arena::ArenaId, id::SolvableId}; use std::cmp::Ordering; /// Represents a decision (i.e. an assignment to a solvable) and the level at which it was made diff --git a/crates/rattler_libsolv_rs/src/solver/decision_tracker.rs b/crates/rattler_libsolv_rs/src/solver/decision_tracker.rs index 0d1b92d44..729009ec6 100644 --- a/crates/rattler_libsolv_rs/src/solver/decision_tracker.rs +++ b/crates/rattler_libsolv_rs/src/solver/decision_tracker.rs @@ -1,6 +1,7 @@ -use crate::id::SolvableId; -use crate::solver::decision::Decision; -use crate::solver::decision_map::DecisionMap; +use crate::{ + internal::id::SolvableId, + solver::{decision::Decision, decision_map::DecisionMap}, +}; /// Tracks the assignments to solvables, keeping a log that can be used to backtrack, and a map that /// can be used to query the current value assigned diff --git a/crates/rattler_libsolv_rs/src/solver/mod.rs b/crates/rattler_libsolv_rs/src/solver/mod.rs index 470c01623..e4338c056 100644 --- a/crates/rattler_libsolv_rs/src/solver/mod.rs +++ b/crates/rattler_libsolv_rs/src/solver/mod.rs @@ -1,9 +1,10 @@ use crate::{ - arena::{Arena, ArenaId}, - frozen_copy_map::FrozenCopyMap, - id::{CandidatesId, ClauseId, DependenciesId, SolvableId}, - id::{LearntClauseId, NameId}, - mapping::Mapping, + internal::{ + arena::{Arena, ArenaId}, + frozen_copy_map::FrozenCopyMap, + id::{CandidatesId, ClauseId, DependenciesId, LearntClauseId, NameId, SolvableId}, + mapping::Mapping, + }, pool::Pool, problem::Problem, solvable::SolvableInner, @@ -25,9 +26,6 @@ mod decision_tracker; mod watch_map; /// Drives the SAT solving process -/// -/// Keeps solvables in a `Pool`, which contains references to `PackageRecord`s (the `'a` lifetime -/// comes from the original `PackageRecord`s) pub struct Solver> { provider: D, @@ -1010,12 +1008,12 @@ impl> Sol #[cfg(test)] mod test { use super::*; - use crate::{Candidates, DefaultSolvableDisplay, Dependencies}; + use crate::{range::Range, Candidates, DefaultSolvableDisplay, Dependencies}; use indexmap::IndexMap; + use std::num::ParseIntError; use std::{ collections::HashMap, fmt::{Debug, Display, Formatter}, - ops::Range, str::FromStr, }; @@ -1055,29 +1053,23 @@ mod test { } } + impl FromStr for Pack { + type Err = ParseIntError; + + fn from_str(s: &str) -> Result { + u32::from_str(s).map(Self) + } + } + /// We can use this to see if a `Pack` is contained in a range of package versions or a `Spec` #[derive(Clone, Debug, PartialEq, Eq, Hash)] struct Spec { name: String, - versions: PackRange, - } - - #[repr(transparent)] - #[derive(Clone, Debug, PartialEq, Eq, Hash)] - struct PackRange(Option>); - - impl Display for PackRange { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - if let Some(versions) = &self.0 { - write!(f, "{}..{}", versions.start, versions.end) - } else { - write!(f, "*") - } - } + versions: Range, } impl Spec { - pub fn new(name: String, versions: PackRange) -> Self { + pub fn new(name: String, versions: Range) -> Self { Self { name, versions } } } @@ -1092,19 +1084,20 @@ mod test { .expect("spec does not have a name") .to_string(); - fn version_range(s: Option<&&str>) -> PackRange { + fn version_range(s: Option<&&str>) -> Range { if let Some(s) = s { - let split = s.split("..").collect::>(); - let start = split[0].parse().unwrap(); - PackRange(Some(Range { - start, - end: split - .get(1) - .map(|s| s.parse().unwrap()) - .unwrap_or_else(|| start + 1), - })) + let (start, end) = s + .split_once("..") + .map_or((*s, None), |(start, end)| (start, Some(end))); + let start: Pack = start.parse().unwrap(); + let end = end + .map(FromStr::from_str) + .transpose() + .unwrap() + .unwrap_or(Pack(start.0 + 1)); + Range::between(start, end) } else { - PackRange(None) + Range::full() } } @@ -1114,22 +1107,10 @@ mod test { } } - impl VersionSet for PackRange { - type V = Pack; - - fn contains(&self, v: &Self::V) -> bool { - if let Some(versions) = &self.0 { - versions.contains(&v.0) - } else { - true - } - } - } - /// This provides sorting functionality for our `BundleBox` packaging system #[derive(Default)] struct BundleBoxProvider { - pool: Pool, + pool: Pool>, packages: IndexMap>, favored: HashMap, locked: HashMap, @@ -1205,14 +1186,14 @@ mod test { } } - impl DependencyProvider for BundleBoxProvider { - fn pool(&self) -> &Pool { + impl DependencyProvider> for BundleBoxProvider { + fn pool(&self) -> &Pool> { &self.pool } fn sort_candidates( &self, - _solver: &Solver, + _solver: &Solver, String, Self>, solvables: &mut [SolvableId], ) { solvables.sort_by(|a, b| { diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_after_backtracking.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_after_backtracking.snap index f1f72e53d..7a0b3eb81 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_after_backtracking.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_after_backtracking.snap @@ -1,15 +1,15 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1654 +assertion_line: 1618 expression: error --- The following packages are incompatible |-- b * can be installed with any of the following options: |-- b 6 | 7 would require - |-- d 1..2, which can be installed with any of the following options: + |-- d >=1, <2, which can be installed with any of the following options: |-- d 1 |-- c * cannot be installed because there are no viable options: |-- c 1 | 2 would require - |-- d 2..3, which cannot be installed because there are no viable options: + |-- d >=2, <3, which cannot be installed because there are no viable options: |-- d 2, which conflicts with the versions reported above. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_applies_graph_compression.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_applies_graph_compression.snap index a00366dc6..1d665891f 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_applies_graph_compression.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_applies_graph_compression.snap @@ -1,6 +1,6 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1720 +assertion_line: 1684 expression: error --- The following packages are incompatible @@ -8,8 +8,8 @@ The following packages are incompatible |-- a 9 | 10 would require |-- b *, which can be installed with any of the following options: |-- b 42 | 100 would require - |-- c 0..100, which can be installed with any of the following options: + |-- c >=0, <100, which can be installed with any of the following options: |-- c 99 -|-- c 101..104 cannot be installed because there are no viable options: +|-- c >=101, <104 cannot be installed because there are no viable options: |-- c 101 | 103, which conflicts with the versions reported above. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_bluesky_conflict.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_bluesky_conflict.snap index 43fccecd1..af13412ee 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_bluesky_conflict.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_bluesky_conflict.snap @@ -1,13 +1,13 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1687 +assertion_line: 1651 expression: error --- The following packages are incompatible -|-- bluesky-widgets 0..100 can be installed with any of the following options: +|-- bluesky-widgets >=0, <100 can be installed with any of the following options: |-- bluesky-widgets 42 would require - |-- suitcase-utils 0..54, which can be installed with any of the following options: + |-- suitcase-utils >=0, <54, which can be installed with any of the following options: |-- suitcase-utils 53 -|-- suitcase-utils 54..100 cannot be installed because there are no viable options: +|-- suitcase-utils >=54, <100 cannot be installed because there are no viable options: |-- suitcase-utils 54, which conflicts with the versions reported above. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains.snap index e73dd1bd5..0f8d9c775 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains.snap @@ -1,14 +1,14 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1794 +assertion_line: 1699 expression: error --- The following packages are incompatible |-- a * can be installed with any of the following options: |-- a 9 | 10 would require - |-- b 50..100, which can be installed with any of the following options: + |-- b >=50, <100, which can be installed with any of the following options: |-- b 50 |-- c * cannot be installed because there are no viable options: |-- c 8 | 10 would constrain - |-- b 0..50 , which conflicts with any installable versions previously reported + |-- b >=0, <50 , which conflicts with any installable versions previously reported diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains_2.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains_2.snap index 38b75e9b3..983105ed4 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains_2.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains_2.snap @@ -7,21 +7,21 @@ The following packages are incompatible |-- a 2 would require |-- b *, which cannot be installed because there are no viable options: |-- b 2 would require - |-- c 2..3, which cannot be installed because there are no viable options: + |-- c >=2, <3, which cannot be installed because there are no viable options: |-- c 2 would constrain - |-- a 3..4 , which conflicts with any installable versions previously reported + |-- a >=3, <4 , which conflicts with any installable versions previously reported |-- b 1 would require - |-- c 1..2, which cannot be installed because there are no viable options: + |-- c >=1, <2, which cannot be installed because there are no viable options: |-- c 1 would constrain - |-- a 3..4 , which conflicts with any installable versions previously reported + |-- a >=3, <4 , which conflicts with any installable versions previously reported |-- a 1 would require |-- b *, which cannot be installed because there are no viable options: |-- b 2 would require - |-- c 2..3, which cannot be installed because there are no viable options: + |-- c >=2, <3, which cannot be installed because there are no viable options: |-- c 2 would constrain - |-- a 3..4 , which conflicts with any installable versions previously reported + |-- a >=3, <4 , which conflicts with any installable versions previously reported |-- b 1 would require - |-- c 1..2, which cannot be installed because there are no viable options: + |-- c >=1, <2, which cannot be installed because there are no viable options: |-- c 1 would constrain - |-- a 3..4 , which conflicts with any installable versions previously reported + |-- a >=3, <4 , which conflicts with any installable versions previously reported diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_incompatible_root_requirements.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_incompatible_root_requirements.snap index 5ffb0a7db..26af1b63b 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_incompatible_root_requirements.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_incompatible_root_requirements.snap @@ -1,11 +1,11 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1661 +assertion_line: 1625 expression: error --- The following packages are incompatible -|-- a 5..10 can be installed with any of the following options: +|-- a >=5, <10 can be installed with any of the following options: |-- a 5 -|-- a 0..4 cannot be installed because there are no viable options: +|-- a >=0, <4 cannot be installed because there are no viable options: |-- a 2, which conflicts with the versions reported above. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_locked_and_excluded.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_locked_and_excluded.snap index 2ffd98c68..6523e76ea 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_locked_and_excluded.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_locked_and_excluded.snap @@ -1,12 +1,12 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1636 +assertion_line: 1571 expression: error --- The following packages are incompatible |-- asdf * can be installed with any of the following options: |-- asdf 1 would require - |-- c 2..3, which can be installed with any of the following options: + |-- c >=2, <3, which can be installed with any of the following options: |-- c 2 |-- c 1 is locked, but another version is required as reported above diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap index 91951ceac..5eea4b5a1 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap @@ -1,7 +1,7 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1675 +assertion_line: 1601 expression: error --- -No candidates were found for b 14..15. +No candidates were found for b >=14, <15. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_1.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_1.snap index 71d5974ea..e1131af0b 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_1.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_1.snap @@ -1,9 +1,9 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1646 +assertion_line: 1579 expression: error --- asdf * cannot be installed because there are no viable options: |-- asdf 1 would require - |-- c 2..3, for which no candidates were found. + |-- c >=2, <3, for which no candidates were found. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap index c5d3e2f03..aa199f087 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap @@ -1,9 +1,9 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1656 +assertion_line: 1586 expression: error --- -a 0..1000 cannot be installed because there are no viable options: +a >=0, <1000 cannot be installed because there are no viable options: |-- a 41 would require - |-- B 0..20, for which no candidates were found. + |-- B >=0, <20, for which no candidates were found. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_pubgrub_article.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_pubgrub_article.snap index 824a463ba..d754f1a34 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_pubgrub_article.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_pubgrub_article.snap @@ -1,17 +1,17 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs -assertion_line: 1704 +assertion_line: 1668 expression: error --- The following packages are incompatible |-- menu * can be installed with any of the following options: |-- menu 10 would require - |-- dropdown 1..2, which can be installed with any of the following options: + |-- dropdown >=1, <2, which can be installed with any of the following options: |-- dropdown 1 would require - |-- intl 3..4, which can be installed with any of the following options: + |-- intl >=3, <4, which can be installed with any of the following options: |-- intl 3 -|-- icons 1..2 can be installed with any of the following options: +|-- icons >=1, <2 can be installed with any of the following options: |-- icons 1 -|-- intl 5..6 cannot be installed because there are no viable options: +|-- intl >=5, <6 cannot be installed because there are no viable options: |-- intl 5, which conflicts with the versions reported above. diff --git a/crates/rattler_libsolv_rs/src/solver/watch_map.rs b/crates/rattler_libsolv_rs/src/solver/watch_map.rs index 05e98c9eb..f6647b0cb 100644 --- a/crates/rattler_libsolv_rs/src/solver/watch_map.rs +++ b/crates/rattler_libsolv_rs/src/solver/watch_map.rs @@ -1,7 +1,7 @@ -use crate::id::ClauseId; -use crate::id::SolvableId; -use crate::mapping::Mapping; -use crate::solver::clause::ClauseState; +use crate::{ + internal::{id::ClauseId, id::SolvableId, mapping::Mapping}, + solver::clause::ClauseState, +}; /// A map from solvables to the clauses that are watching them pub(crate) struct WatchMap { From cee703a159db8ef111fa3dc8390b9a56e04ffcba Mon Sep 17 00:00:00 2001 From: Orion Yeung <11580988+orionyeung001@users.noreply.github.com> Date: Fri, 15 Sep 2023 17:05:52 -0500 Subject: [PATCH 4/6] added build_spec module - unused in match_spec --- .../rattler_conda_types/src/build_spec/mod.rs | 97 +++++++++ .../src/build_spec/parse.rs | 196 ++++++++++++++++++ crates/rattler_conda_types/src/lib.rs | 1 + 3 files changed, 294 insertions(+) create mode 100644 crates/rattler_conda_types/src/build_spec/mod.rs create mode 100644 crates/rattler_conda_types/src/build_spec/parse.rs diff --git a/crates/rattler_conda_types/src/build_spec/mod.rs b/crates/rattler_conda_types/src/build_spec/mod.rs new file mode 100644 index 000000000..094963e23 --- /dev/null +++ b/crates/rattler_conda_types/src/build_spec/mod.rs @@ -0,0 +1,97 @@ +//! This module contains code to work with "build number spec". It represents the build number key of +//! [`crate::MatchSpec`], e.g.: `>=3,<4`. + +pub mod parse; + +use serde::{Deserialize, Serialize}; +use std::fmt::{Display, Formatter}; + +/// Named type for the BuildNumber instead of explicit u64 floating about the project +pub type BuildNumber = u64; + +/// An operator to compare two versions. +#[allow(missing_docs)] +#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize, Deserialize)] +pub enum OrdOperator { + Gt, + Ge, + Lt, + Le, + Eq, + Ne, +} + +/// Define match from some kind of operator and a specific element +/// +/// Ideally we could have some kind of type constraint to guarantee that +/// there's function relating the operator and element into a function that returns bool +/// possible TODO: create `Operator` trait +#[derive(Debug, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, Deserialize)] +pub struct OperatorConstraint { + op: Operator, + rhs: Element, +} + +impl OperatorConstraint { + /// convenience constructor wrapper + pub fn new(op: Operator, rhs: Element) -> Self { + Self { op, rhs } + } +} + +/// Define match from OrdOperator and BuildNumber as Element +pub type BuildNumberSpec = OperatorConstraint; + +impl Display for OrdOperator { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + Self::Gt => write!(f, ">"), + Self::Ge => write!(f, ">="), + Self::Lt => write!(f, "<"), + Self::Le => write!(f, "<="), + Self::Eq => write!(f, "=="), + Self::Ne => write!(f, "!="), + } + } +} + +impl Display for BuildNumberSpec { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}{}", self.op, self.rhs) + } +} + +impl BuildNumberSpec { + /// Returns whether the number matches the specification. + /// Expected use is within match_spec::MatchSpec::matches + pub fn matches(&self, build_num: &BuildNumber) -> bool { + match self.op { + OrdOperator::Gt => build_num.gt(&self.rhs), + OrdOperator::Ge => build_num.ge(&self.rhs), + OrdOperator::Lt => build_num.lt(&self.rhs), + OrdOperator::Le => build_num.le(&self.rhs), + OrdOperator::Eq => build_num.eq(&self.rhs), + OrdOperator::Ne => build_num.ne(&self.rhs), + } + } +} + +#[cfg(test)] +mod tests { + use super::{BuildNumberSpec, OrdOperator}; + + #[test] + fn test_matches() { + let test_cases = vec![ + (BuildNumberSpec::new(OrdOperator::Gt, 3), 5, true), + (BuildNumberSpec::new(OrdOperator::Ge, 3), 5, true), + (BuildNumberSpec::new(OrdOperator::Lt, 3), 5, false), + (BuildNumberSpec::new(OrdOperator::Le, 3), 7, false), + (BuildNumberSpec::new(OrdOperator::Eq, 3), 7, false), + (BuildNumberSpec::new(OrdOperator::Ne, 3), 7, true), + ]; + for (spec, test_val, is_match) in test_cases { + assert_eq!(spec.matches(&test_val), is_match); + } + } +} diff --git a/crates/rattler_conda_types/src/build_spec/parse.rs b/crates/rattler_conda_types/src/build_spec/parse.rs new file mode 100644 index 000000000..aba6bb6dc --- /dev/null +++ b/crates/rattler_conda_types/src/build_spec/parse.rs @@ -0,0 +1,196 @@ +//! this module supports the parsing features of the build number spec + +use super::{BuildNumber, BuildNumberSpec, OrdOperator}; + +use nom::{bytes::complete::take_while1, character::complete::digit1, Finish, IResult}; +use std::str::FromStr; +use thiserror::Error; + +impl FromStr for BuildNumberSpec { + type Err = ParseBuildNumberSpecError; + + fn from_str(s: &str) -> Result { + match Self::parser(s).finish()? { + ("", spec) => Ok(spec), + (_, _) => Err(ParseBuildNumberSpecError::ExpectedEof), + } + } +} + +impl BuildNumberSpec { + /// Parses a build number spec, string representation is optional operator preceding whole number + pub fn parser(input: &str) -> IResult<&str, BuildNumberSpec, ParseBuildNumberSpecError> { + // Parse the optional preceding operator + let (input, op) = match OrdOperator::parser(input) { + Err( + nom::Err::Failure(ParseOrdOperatorError::InvalidOperator(op)) + | nom::Err::Error(ParseOrdOperatorError::InvalidOperator(op)), + ) => { + return Err(nom::Err::Failure( + ParseBuildNumberSpecError::InvalidOperator( + ParseOrdOperatorError::InvalidOperator(op.to_owned()), + ), + )) + } + Err(nom::Err::Error(_)) => (input, None), + Ok((rest, op)) => (rest, Some(op)), + _ => unreachable!(), + }; + + let (rest, build_num) = digit1(input) + .map(|(rest, digits): (&str, &str)| { + ( + rest, + digits + .parse::() + .expect("nom found at least 1 digit(s)"), + ) + }) + .map_err(|_: nom::Err>| { + nom::Err::Error(ParseBuildNumberSpecError::InvalidBuildNumber( + ParseBuildNumberError, + )) + })?; + + match op { + Some(op) => Ok((rest, BuildNumberSpec::new(op, build_num))), + None => Ok((rest, BuildNumberSpec::new(OrdOperator::Eq, build_num))), + } + } +} + +/// Possible errors when parsing the OrdOperator which precedes the digits in a build number spec +#[derive(Debug, Clone, Error, Eq, PartialEq)] +pub enum ParseOrdOperatorError { + /// Indicates that operator symbols were captured, + /// but not interpretable as an OrdOperator + #[error("invalid operator '{0}'")] + InvalidOperator(String), + /// Indicates no symbol sequence found for OrdOperators, + /// callers should expect explicit operators + #[error("expected version operator")] + ExpectedOperator, + /// Indicates that there was data after an operator was read, + /// callers should handle this if they expect input to end with the operator + #[error("expected EOF")] + ExpectedEof, +} + +/// Simplified error when parsing the digits into a build number: u64 in a build number spec +#[derive(Debug, Clone, Eq, PartialEq, Error)] +#[error("could not parse build number")] +pub struct ParseBuildNumberError; + +/// Composition of possible errors when parsing the spec for build numbers +#[allow(clippy::enum_variant_names, missing_docs)] +#[derive(Debug, Clone, Error, Eq, PartialEq)] +pub enum ParseBuildNumberSpecError { + #[error("invalid version: {0}")] + InvalidBuildNumber(#[source] ParseBuildNumberError), + #[error("invalid version constraint: {0}")] + InvalidOperator(#[source] ParseOrdOperatorError), + #[error("expected EOF")] + ExpectedEof, +} + +impl FromStr for OrdOperator { + type Err = ParseOrdOperatorError; + + fn from_str(s: &str) -> Result { + match Self::parser(s).finish()? { + ("", spec) => Ok(spec), + (_, _) => Err(ParseOrdOperatorError::ExpectedEof), + } + } +} + +impl OrdOperator { + /// Parses an operator representing PartialOrd compares, returns an error if the operator is not recognized or not found. + fn parser(input: &str) -> IResult<&str, OrdOperator, ParseOrdOperatorError> { + // Take anything that looks like an operator. + let (rest, operator_str) = take_while1(|c| "=!<>".contains(c))(input).map_err( + |_: nom::Err>| { + nom::Err::Error(ParseOrdOperatorError::ExpectedOperator) + }, + )?; + + let op = match operator_str { + "==" => OrdOperator::Eq, + "!=" => OrdOperator::Ne, + "<=" => OrdOperator::Le, + ">=" => OrdOperator::Ge, + "<" => OrdOperator::Lt, + ">" => OrdOperator::Gt, + _ => { + return Err(nom::Err::Failure(ParseOrdOperatorError::InvalidOperator( + operator_str.to_string(), + ))) + } + }; + + Ok((rest, op)) + } +} + +#[cfg(test)] +mod test { + use super::{BuildNumberSpec, OrdOperator, ParseOrdOperatorError}; + + use nom::Finish; + + #[test] + fn parse_operator_from_spec() { + let test_params = vec![ + (">3.1", OrdOperator::Gt), + (">=3.1", OrdOperator::Ge), + ("<3.1", OrdOperator::Lt), + ("<=3.1", OrdOperator::Le), + ("==3.1", OrdOperator::Eq), + ("!=3.1", OrdOperator::Ne), + ]; + + for (s, op) in test_params { + assert_eq!(OrdOperator::parser(s), Ok(("3.1", op))) + } + + assert_eq!( + OrdOperator::parser("<==>3.1"), + Err(nom::Err::Failure(ParseOrdOperatorError::InvalidOperator( + "<==>".to_string() + ))) + ); + assert_eq!( + OrdOperator::parser("3.1"), + Err(nom::Err::Error(ParseOrdOperatorError::ExpectedOperator)) + ); + } + + #[test] + fn parse_spec() { + let test_params = vec![ + (">1", OrdOperator::Gt), + (">=1", OrdOperator::Ge), + ("<1", OrdOperator::Lt), + ("<=1", OrdOperator::Le), + ("==1", OrdOperator::Eq), + ("!=1", OrdOperator::Ne), + ]; + + for (s, op) in test_params { + assert_eq!( + BuildNumberSpec::parser(s), + Ok(("", BuildNumberSpec::new(op, 1))) + ) + } + + assert_eq!( + BuildNumberSpec::parser(">=1.1"), + Ok((".1", BuildNumberSpec::new(OrdOperator::Ge, 1))) + ); + + assert!(matches!( + BuildNumberSpec::parser(">=build3").finish(), + Err(_) + )); + } +} diff --git a/crates/rattler_conda_types/src/lib.rs b/crates/rattler_conda_types/src/lib.rs index 22777ccc4..7ca778939 100644 --- a/crates/rattler_conda_types/src/lib.rs +++ b/crates/rattler_conda_types/src/lib.rs @@ -2,6 +2,7 @@ //! `rattler-conda-types` contains data models for types commonly found within the Conda ecosystem. //! The library itself doesnt provide any functionality besides parsing the data types. +mod build_spec; mod channel; mod channel_data; mod explicit_environment_spec; From 252e682923068db0ba69d78487293b11ce82a35a Mon Sep 17 00:00:00 2001 From: Orion Yeung <11580988+orionyeung001@users.noreply.github.com> Date: Tue, 19 Sep 2023 22:16:43 -0500 Subject: [PATCH 5/6] per feedback on #340, allow unused with remark on temporary usage --- crates/rattler_conda_types/src/build_spec/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/rattler_conda_types/src/build_spec/mod.rs b/crates/rattler_conda_types/src/build_spec/mod.rs index 094963e23..925de5470 100644 --- a/crates/rattler_conda_types/src/build_spec/mod.rs +++ b/crates/rattler_conda_types/src/build_spec/mod.rs @@ -62,6 +62,9 @@ impl Display for BuildNumberSpec { } impl BuildNumberSpec { + // for PR #340 will allow this pass CI + // expect following PR where `matches` will be provided with an impl of publicly declared trait + #[allow(unused)] /// Returns whether the number matches the specification. /// Expected use is within match_spec::MatchSpec::matches pub fn matches(&self, build_num: &BuildNumber) -> bool { From d00f4cbbb404296548d841f22e9f57a14efb751e Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Wed, 20 Sep 2023 09:16:12 +0200 Subject: [PATCH 6/6] fix: clippy warning --- crates/rattler_conda_types/src/build_spec/parse.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rattler_conda_types/src/build_spec/parse.rs b/crates/rattler_conda_types/src/build_spec/parse.rs index aba6bb6dc..639eb6ede 100644 --- a/crates/rattler_conda_types/src/build_spec/parse.rs +++ b/crates/rattler_conda_types/src/build_spec/parse.rs @@ -28,7 +28,7 @@ impl BuildNumberSpec { ) => { return Err(nom::Err::Failure( ParseBuildNumberSpecError::InvalidOperator( - ParseOrdOperatorError::InvalidOperator(op.to_owned()), + ParseOrdOperatorError::InvalidOperator(op), ), )) }