diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0385d8cc..94fef458 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,19 @@ jobs: - run: cargo build --workspace - run: cargo test --all-features --workspace + test-i686: + name: Tests (32-bit) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 + - uses: dtolnay/rust-toolchain@stable + - run: | + sudo apt-get update + sudo apt-get install -y gcc-multilib + - run: rustup target add i686-unknown-linux-gnu + - run: cargo build --workspace --target i686-unknown-linux-gnu + - run: cargo test --all-features --workspace --target i686-unknown-linux-gnu + clippy: name: Clippy runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index fd954866..3cd9ece8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ All notable changes to this project will be documented in this file. +## 0.4.0 + +`DependencyConstraints` is now an opaque type that retains the ordering of the dependencies across platforms. This fixes +unstable resolutions across platform (https://github.com/pubgrub-rs/pubgrub/issues/373). `DependencyConstraints` can be +constructed from an iterator. It is currently backed by a `Vec` internally. + +`SelectedDependencies` is now an opaque type that support iteration and `get()`. + ## [0.3.0] - 2025-02-12 - [(diff with 0.2.1)][0.2.1-diff] PubGrub 0.3 has a more flexible interface and speeds resolution significantly. The public API is very different now, we diff --git a/Cargo.lock b/Cargo.lock index ff055302..dce89f8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -680,7 +680,7 @@ dependencies = [ [[package]] name = "pubgrub" -version = "0.3.0" +version = "0.4.0" dependencies = [ "codspeed-criterion-compat", "env_logger", diff --git a/Cargo.toml b/Cargo.toml index dde54ca6..25fcb305 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,7 +5,7 @@ members = ["version-ranges"] [package] name = "pubgrub" -version = "0.3.0" +version = "0.4.0" authors = [ "Matthieu Pizenberg ", "Alex Tokarev ", diff --git a/src/lib.rs b/src/lib.rs index 75fe4bbe..cfe01b79 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -229,9 +229,12 @@ pub use report::{ DefaultStringReportFormatter, DefaultStringReporter, DerivationTree, Derived, External, ReportFormatter, Reporter, }; -pub use solver::{Dependencies, DependencyProvider, PackageResolutionStatistics, resolve}; +pub use solver::{ + Dependencies, DependencyConstraints, DependencyProvider, PackageResolutionStatistics, + SelectedDependencies, resolve, +}; pub use term::Term; -pub use type_aliases::{DependencyConstraints, Map, SelectedDependencies, Set}; +pub use type_aliases::{Map, Set}; pub use version::{SemanticVersion, VersionParseError}; pub use version_ranges::Ranges; #[deprecated(note = "Use `Ranges` instead")] diff --git a/src/solver.rs b/src/solver.rs index 5d8c0560..2ca5106a 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -4,12 +4,9 @@ use std::collections::BTreeSet as Set; use std::error::Error; use std::fmt::{Debug, Display}; -use log::{debug, info}; - use crate::internal::{Id, Incompatibility, State}; -use crate::{ - DependencyConstraints, Map, Package, PubGrubError, SelectedDependencies, Term, VersionSet, -}; +use crate::{Map, Package, PubGrubError, Term, VersionSet}; +use log::{debug, info}; /// Statistics on how often a package conflicted with other packages. #[derive(Debug, Default, Clone)] @@ -48,6 +45,36 @@ impl PackageResolutionStatistics { } } +/// The resolved dependencies and their versions. +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct SelectedDependencies(Map); + +impl SelectedDependencies { + /// Iterate over the resolved dependencies and their versions. + pub fn iter(&self) -> impl Iterator { + self.0.iter() + } + + /// Get the version of a specific dependencies. + pub fn get(&self, package: &P) -> Option<&V> { + self.0.get(package) + } +} + +impl FromIterator<(P, V)> for SelectedDependencies { + fn from_iter>(iter: I) -> Self { + Self(Map::from_iter(iter)) + } +} + +impl IntoIterator for SelectedDependencies { + type Item = (P, V); + type IntoIter = as IntoIterator>::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.0.into_iter() + } +} /// Finds a set of packages satisfying dependency bounds for a given package + version pair. /// /// It consists in efficiently finding a set of packages and versions @@ -109,7 +136,7 @@ pub fn resolve( dependency_provider: &DP, package: DP::P, version: impl Into, -) -> Result, PubGrubError> { +) -> Result, PubGrubError> { let mut state: State = State::init(package.clone(), version.into()); let mut conflict_tracker: Map, PackageResolutionStatistics> = Map::default(); let mut added_dependencies: Map, Set> = Map::default(); @@ -154,11 +181,13 @@ pub fn resolve( ) }) else { - return Ok(state - .partial_solution - .extract_solution() - .map(|(p, v)| (state.package_store[p].clone(), v)) - .collect()); + return Ok(SelectedDependencies( + state + .partial_solution + .extract_solution() + .map(|(p, v)| (state.package_store[p].clone(), v)) + .collect(), + )); }; next = highest_priority_pkg; @@ -247,6 +276,69 @@ pub fn resolve( } } +/// The dependencies of a package with their version ranges. +/// +/// There is a difference in semantics between an empty [DependencyConstraints] and +/// [Dependencies::Unavailable]: +/// The former means the package has no dependency and it is a known fact, +/// while the latter means they could not be fetched by the [DependencyProvider]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DependencyConstraints(Vec<(P, VS)>); + +/// Backwards compatibility: Serialize as map. +#[cfg(feature = "serde")] +impl serde::Serialize + for DependencyConstraints +{ + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + Map::from_iter(self.0.iter().map(|(p, v)| (p, v))).serialize(serializer) + } +} + +/// Backwards compatibility: Deserialize as map. +#[cfg(feature = "serde")] +impl<'de, P: Package + serde::Deserialize<'de>, VS: serde::Deserialize<'de>> serde::Deserialize<'de> + for DependencyConstraints +{ + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + Ok(Self::from_iter(Map::deserialize(deserializer)?)) + } +} + +impl DependencyConstraints { + /// Iterate over each dependency in order. + pub fn iter(&self) -> impl Iterator { + self.0.iter() + } +} + +impl Default for DependencyConstraints { + fn default() -> Self { + Self(Vec::new()) + } +} + +impl FromIterator<(P, VS)> for DependencyConstraints { + fn from_iter>(iter: T) -> Self { + Self(iter.into_iter().collect()) + } +} + +impl IntoIterator for DependencyConstraints { + type Item = (P, VS); + type IntoIter = as IntoIterator>::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.0.into_iter() + } +} + /// An enum used by [DependencyProvider] that holds information about package dependencies. /// For each [Package] there is a set of versions allowed as a dependency. #[derive(Clone)] diff --git a/src/type_aliases.rs b/src/type_aliases.rs index 6bbd9dd0..5ce08d6b 100644 --- a/src/type_aliases.rs +++ b/src/type_aliases.rs @@ -2,22 +2,8 @@ //! Publicly exported type aliases. -use crate::DependencyProvider; - /// Map implementation used by the library. pub type Map = rustc_hash::FxHashMap; /// Set implementation used by the library. pub type Set = rustc_hash::FxHashSet; - -/// Concrete dependencies picked by the library during [resolve](crate::solver::resolve) -/// from [DependencyConstraints]. -pub type SelectedDependencies = - Map<::P, ::V>; - -/// Holds information about all possible versions a given package can accept. -/// There is a difference in semantics between an empty map -/// inside [DependencyConstraints] and [Dependencies::Unavailable](crate::solver::Dependencies::Unavailable): -/// the former means the package has no dependency and it is a known fact, -/// while the latter means they could not be fetched by the [DependencyProvider]. -pub type DependencyConstraints = Map; diff --git a/tests/examples.rs b/tests/examples.rs index 216985fd..b91fecde 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -2,7 +2,7 @@ use pubgrub::{ DefaultStringReporter, Map, OfflineDependencyProvider, PubGrubError, Ranges, Reporter as _, - SemanticVersion, Set, resolve, + SelectedDependencies, SemanticVersion, Set, resolve, }; type NumVS = Ranges; @@ -48,7 +48,10 @@ fn no_conflict() { expected_solution.insert("bar", (1, 0, 0).into()); // Comparing the true solution with the one computed by the algorithm. - assert_eq!(expected_solution, computed_solution); + assert_eq!( + SelectedDependencies::from_iter(expected_solution), + computed_solution + ); } #[test] @@ -84,7 +87,10 @@ fn avoiding_conflict_during_decision_making() { expected_solution.insert("bar", (1, 1, 0).into()); // Comparing the true solution with the one computed by the algorithm. - assert_eq!(expected_solution, computed_solution); + assert_eq!( + SelectedDependencies::from_iter(expected_solution), + computed_solution + ); } #[test] @@ -118,7 +124,10 @@ fn conflict_resolution() { expected_solution.insert("foo", (1, 0, 0).into()); // Comparing the true solution with the one computed by the algorithm. - assert_eq!(expected_solution, computed_solution); + assert_eq!( + SelectedDependencies::from_iter(expected_solution), + computed_solution + ); } #[test] @@ -177,7 +186,10 @@ fn conflict_with_partial_satisfier() { expected_solution.insert("target", (2, 0, 0).into()); // Comparing the true solution with the one computed by the algorithm. - assert_eq!(expected_solution, computed_solution); + assert_eq!( + SelectedDependencies::from_iter(expected_solution), + computed_solution + ); } #[test] @@ -208,7 +220,10 @@ fn double_choices() { // Run the algorithm. let computed_solution = resolve(&dependency_provider, "a", 0u32).unwrap(); - assert_eq!(expected_solution, computed_solution); + assert_eq!( + SelectedDependencies::from_iter(expected_solution), + computed_solution + ); } #[test] diff --git a/tests/proptest.rs b/tests/proptest.rs index ef592e46..c643df36 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -130,10 +130,7 @@ fn timeout_resolve( dependency_provider: DP, name: DP::P, version: impl Into, -) -> Result< - SelectedDependencies>, - PubGrubError>, -> { +) -> Result, PubGrubError>> { resolve( &TimeoutDependencyProvider::new(dependency_provider, 50_000), name, @@ -292,7 +289,7 @@ fn meta_test_deep_trees_from_strategy() { let res = resolve(&dependency_provider, name, ver); dis[res .as_ref() - .map(|x| std::cmp::min(x.len(), dis.len()) - 1) + .map(|x| std::cmp::min(x.iter().count(), dis.len()) - 1) .unwrap_or(0)] += 1; if dis.iter().all(|&x| x > 0) { return; @@ -553,7 +550,7 @@ proptest! { .packages() .flat_map(|p| { dependency_provider - .versions(&p) + .versions(p) .unwrap() .map(move |&v| (*p, v)) }) diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index a1403d86..8e5fe426 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -68,10 +68,10 @@ impl SatResolve { Dependencies::Unavailable(_) => panic!(), Dependencies::Available(d) => d, }; - for (p1, range) in &deps { + for (p1, range) in deps { let empty_vec = vec![]; let mut matches: Vec = all_versions_by_p - .get(p1) + .get(&p1) .unwrap_or(&empty_vec) .iter() .filter(|(v1, _)| range.contains(v1)) @@ -117,7 +117,7 @@ impl SatResolve { pub fn is_valid_solution>( &mut self, - pids: &SelectedDependencies, + pids: &SelectedDependencies, ) -> bool { let mut assumption = vec![]; @@ -137,7 +137,7 @@ impl SatResolve { pub fn check_resolve>( &mut self, - res: &Result, PubGrubError>, + res: &Result, PubGrubError>, p: &P, v: &VS::V, ) { diff --git a/tests/tests.rs b/tests/tests.rs index e8390f86..f5db392a 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -1,6 +1,10 @@ // SPDX-License-Identifier: MPL-2.0 -use pubgrub::{OfflineDependencyProvider, PubGrubError, Ranges, resolve}; +use pubgrub::{ + Dependencies, DependencyProvider, OfflineDependencyProvider, Package, + PackageResolutionStatistics, PubGrubError, Ranges, VersionSet, resolve, +}; +use std::convert::Infallible; type NumVS = Ranges; @@ -50,3 +54,83 @@ fn depend_on_self() { dependency_provider.add_dependencies("a", 66u32, [("a", Ranges::singleton(111u32))]); assert!(resolve(&dependency_provider, "a", 66u32).is_err()); } + +/// Test the prioritization is stable across platforms. +/// +/// https://github.com/pubgrub-rs/pubgrub/issues/373#issuecomment-3384608891 +#[test] +fn same_result_across_platforms() { + struct UnprioritizingDependencyProvider { + dependency_provider: OfflineDependencyProvider, + } + + impl UnprioritizingDependencyProvider { + fn new() -> Self { + Self { + dependency_provider: OfflineDependencyProvider::new(), + } + } + + pub fn add_dependencies>( + &mut self, + package: P, + version: impl Into, + dependencies: I, + ) { + self.dependency_provider + .add_dependencies(package, version, dependencies); + } + } + + impl DependencyProvider for UnprioritizingDependencyProvider { + type P = P; + type V = VS::V; + type VS = VS; + type M = String; + type Priority = u32; + type Err = Infallible; + + fn choose_version(&self, package: &P, range: &VS) -> Result, Infallible> { + self.dependency_provider.choose_version(package, range) + } + + fn prioritize( + &self, + _package: &Self::P, + _range: &Self::VS, + _package_statistics: &PackageResolutionStatistics, + ) -> Self::Priority { + 0 + } + + fn get_dependencies( + &self, + package: &P, + version: &VS::V, + ) -> Result, Infallible> { + self.dependency_provider.get_dependencies(package, version) + } + } + + let mut dependency_provider = UnprioritizingDependencyProvider::<_, NumVS>::new(); + + let x = (0..1000) + .map(|i| (i.to_string(), Ranges::full())) + .collect::>(); + dependency_provider.add_dependencies("root".to_string(), 1u32, x); + + for i in 0..1000 { + let x = (0..1000) + .filter(|j| *j != i) + .map(|i| (i.to_string(), Ranges::::singleton(1u32))) + .collect::>(); + dependency_provider.add_dependencies(i.to_string(), 2u32, x); + dependency_provider.add_dependencies(i.to_string(), 1u32, []); + } + + let name = "root".to_string(); + let ver: u32 = 1; + let resolution = resolve(&dependency_provider, name, ver).unwrap(); + let (p, _v) = resolution.into_iter().find(|(_p, v)| *v == 2).unwrap(); + assert_eq!(p, "0".to_string()); +}