From e2588bc01662d045ef6986e3c8f415a81028179b Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 31 Jul 2024 14:26:05 +0200 Subject: [PATCH 1/8] Expose internal interfaces used by uv --- src/internal/arena.rs | 4 ++-- src/internal/core.rs | 16 ++++++++-------- src/internal/incompatibility.rs | 16 +++++++++------- src/internal/mod.rs | 7 +++++-- src/internal/partial_solution.rs | 13 ++++++------- src/lib.rs | 3 +++ src/term.rs | 2 +- 7 files changed, 34 insertions(+), 27 deletions(-) diff --git a/src/internal/arena.rs b/src/internal/arena.rs index 6edb85df..826de1a4 100644 --- a/src/internal/arena.rs +++ b/src/internal/arena.rs @@ -10,7 +10,7 @@ use std::ops::{Index, Range}; /// that we actually don't need since it is phantom. /// /// -pub(crate) struct Id { +pub struct Id { raw: u32, _ty: PhantomData T>, } @@ -71,7 +71,7 @@ impl Id { /// to have references between those items. /// They are all dropped at once when the arena is dropped. #[derive(Clone, PartialEq, Eq)] -pub(crate) struct Arena { +pub struct Arena { data: Vec, } diff --git a/src/internal/core.rs b/src/internal/core.rs index 4a8b7bca..dc2baf1c 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -14,12 +14,13 @@ use crate::{DependencyProvider, DerivationTree, Map, NoSolutionError, VersionSet /// Current state of the PubGrub algorithm. #[derive(Clone)] -pub(crate) struct State { +pub struct State { root_package: DP::P, root_version: DP::V, + /// All incompatibilities indexed by package. #[allow(clippy::type_complexity)] - incompatibilities: Map>>, + pub incompatibilities: Map>>, /// Store the ids of incompatibilities that are already contradicted. /// For each one keep track of the decision level when it was found to be contradicted. @@ -32,11 +33,10 @@ pub(crate) struct State { merged_dependencies: Map<(DP::P, DP::P), SmallVec>>, /// Partial solution. - /// TODO: remove pub. - pub(crate) partial_solution: PartialSolution, + pub partial_solution: PartialSolution, /// The store is the reference storage for all incompatibilities. - pub(crate) incompatibility_store: Arena>, + pub incompatibility_store: Arena>, /// This is a stack of work to be done in `unit_propagation`. /// It can definitely be a local variable to that method, but @@ -46,7 +46,7 @@ pub(crate) struct State { impl State { /// Initialization of PubGrub state. - pub(crate) fn init(root_package: DP::P, root_version: DP::V) -> Self { + pub fn init(root_package: DP::P, root_version: DP::V) -> Self { let mut incompatibility_store = Arena::new(); let not_root_id = incompatibility_store.alloc(Incompatibility::not_root( root_package.clone(), @@ -67,7 +67,7 @@ impl State { } /// Add an incompatibility to the state. - pub(crate) fn add_incompatibility(&mut self, incompat: Incompatibility) { + pub fn add_incompatibility(&mut self, incompat: Incompatibility) { let id = self.incompatibility_store.alloc(incompat); self.merge_incompatibility(id); } @@ -98,7 +98,7 @@ impl State { /// Unit propagation is the core mechanism of the solving algorithm. /// CF - pub(crate) fn unit_propagation(&mut self, package: DP::P) -> Result<(), NoSolutionError> { + pub fn unit_propagation(&mut self, package: DP::P) -> Result<(), NoSolutionError> { self.unit_propagation_buffer.clear(); self.unit_propagation_buffer.push(package); while let Some(current_package) = self.unit_propagation_buffer.pop() { diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 9740d50f..f37dfcf9 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -28,9 +28,10 @@ use crate::{ /// during conflict resolution. More about all this in /// [PubGrub documentation](https://github.com/dart-lang/pub/blob/master/doc/solver.md#incompatibility). #[derive(Debug, Clone)] -pub(crate) struct Incompatibility { +pub struct Incompatibility { package_terms: SmallMap>, - kind: Kind, + /// The reason for the incompatibility. + pub kind: Kind, } /// Type alias of unique identifiers for incompatibilities. @@ -42,8 +43,9 @@ pub(crate) type IncompDpId = IncompId< ::M, >; +/// The reason for the incompatibility. #[derive(Debug, Clone)] -enum Kind { +pub enum Kind { /// Initial incompatibility aiming at picking the root package for the first decision. /// /// This incompatibility drives the resolution, it requires that we pick the (virtual) root @@ -104,7 +106,7 @@ impl Incompatibilit } /// Create an incompatibility to remember that a given set does not contain any version. - pub(crate) fn no_versions(package: P, term: Term) -> Self { + pub fn no_versions(package: P, term: Term) -> Self { let set = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), @@ -117,7 +119,7 @@ impl Incompatibilit /// Create an incompatibility for a reason outside pubgrub. #[allow(dead_code)] // Used by uv - pub(crate) fn custom_term(package: P, term: Term, metadata: M) -> Self { + pub fn custom_term(package: P, term: Term, metadata: M) -> Self { let set = match &term { Term::Positive(r) => r.clone(), Term::Negative(_) => panic!("No version should have a positive term"), @@ -129,7 +131,7 @@ impl Incompatibilit } /// Create an incompatibility for a reason outside pubgrub. - pub(crate) fn custom_version(package: P, version: VS::V, metadata: M) -> Self { + pub fn custom_version(package: P, version: VS::V, metadata: M) -> Self { let set = VS::singleton(version); let term = Term::Positive(set.clone()); Self { @@ -139,7 +141,7 @@ impl Incompatibilit } /// Build an incompatibility from a given dependency. - pub(crate) fn from_dependency(package: P, versions: VS, dep: (P, VS)) -> Self { + pub fn from_dependency(package: P, versions: VS, dep: (P, VS)) -> Self { let (p2, set2) = dep; Self { package_terms: if set2 == VS::empty() { diff --git a/src/internal/mod.rs b/src/internal/mod.rs index 643634f3..fd5ed9ba 100644 --- a/src/internal/mod.rs +++ b/src/internal/mod.rs @@ -10,8 +10,11 @@ mod small_map; mod small_vec; pub(crate) use arena::{Arena, Id}; -pub(crate) use core::State; -pub(crate) use incompatibility::{IncompDpId, IncompId, Incompatibility, Relation}; +pub(crate) use incompatibility::{IncompDpId, IncompId, Relation}; pub(crate) use partial_solution::{DecisionLevel, PartialSolution, SatisfierSearch}; pub(crate) use small_map::SmallMap; pub(crate) use small_vec::SmallVec; + +// uv-specific additions +pub use core::State; +pub use incompatibility::{Incompatibility, Kind}; diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 3d078162..4edf243f 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -9,8 +9,7 @@ use std::hash::BuildHasherDefault; use priority_queue::PriorityQueue; use rustc_hash::FxHasher; -use super::small_vec::SmallVec; -use crate::internal::{Arena, IncompDpId, IncompId, Incompatibility, Relation, SmallMap}; +use crate::internal::{Arena, IncompDpId, IncompId, Incompatibility, Relation, SmallMap, SmallVec}; use crate::{DependencyProvider, Package, SelectedDependencies, Term, VersionSet}; type FnvIndexMap = indexmap::IndexMap>; @@ -27,7 +26,7 @@ impl DecisionLevel { /// The partial solution contains all package assignments, /// organized by package and historically ordered. #[derive(Clone, Debug)] -pub(crate) struct PartialSolution { +pub struct PartialSolution { next_global_index: u32, current_decision_level: DecisionLevel, /// `package_assignments` is primarily a HashMap from a package to its @@ -156,7 +155,7 @@ impl PartialSolution { } /// Add a decision. - pub(crate) fn add_decision(&mut self, package: DP::P, version: DP::V) { + pub fn add_decision(&mut self, package: DP::P, version: DP::V) { // Check that add_decision is never used in the wrong context. if cfg!(debug_assertions) { match self.package_assignments.get_mut(&package) { @@ -255,7 +254,7 @@ impl PartialSolution { } } - pub(crate) fn pick_highest_priority_pkg( + pub fn pick_highest_priority_pkg( &mut self, prioritizer: impl Fn(&DP::P, &DP::VS) -> DP::Priority, ) -> Option { @@ -286,7 +285,7 @@ impl PartialSolution { /// If a partial solution has, for every positive derivation, /// a corresponding decision that satisfies that assignment, /// it's a total solution and version solving has succeeded. - pub(crate) fn extract_solution(&self) -> SelectedDependencies { + pub fn extract_solution(&self) -> SelectedDependencies { self.package_assignments .iter() .take(self.current_decision_level.0 as usize) @@ -386,7 +385,7 @@ impl PartialSolution { } /// Retrieve intersection of terms related to package. - pub(crate) fn term_intersection_for_package(&self, package: &DP::P) -> Option<&Term> { + pub fn term_intersection_for_package(&self, package: &DP::P) -> Option<&Term> { self.package_assignments .get(package) .map(|pa| pa.assignments_intersection.term()) diff --git a/src/lib.rs b/src/lib.rs index 344ca0cd..7c4a563c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -233,4 +233,7 @@ pub use type_aliases::{DependencyConstraints, Map, SelectedDependencies, Set}; pub use version::{SemanticVersion, VersionParseError}; pub use version_set::VersionSet; +// uv-specific additions +pub use internal::{Incompatibility, Kind, State}; + mod internal; diff --git a/src/term.rs b/src/term.rs index f67a8109..7f6d65f6 100644 --- a/src/term.rs +++ b/src/term.rs @@ -72,7 +72,7 @@ impl Term { /// Unwrap the set contained in a positive term. /// Will panic if used on a negative set. - pub(crate) fn unwrap_positive(&self) -> &VS { + pub fn unwrap_positive(&self) -> &VS { match self { Self::Positive(set) => set, _ => panic!("Negative term cannot unwrap positive set"), From 40f338c780c0ca3c0fdccd0d1e41d9aaf3dd7d4e Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 31 Jul 2024 14:34:05 +0200 Subject: [PATCH 2/8] Add `PartialSolution::prioritized_packages` --- src/internal/partial_solution.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 4edf243f..83389b6f 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -254,6 +254,24 @@ impl PartialSolution { } } + pub fn prioritized_packages(&self) -> impl Iterator { + let check_all = self.changed_this_decision_level + == self.current_decision_level.0.saturating_sub(1) as usize; + let current_decision_level = self.current_decision_level; + self.package_assignments + .get_range(self.changed_this_decision_level..) + .unwrap() + .iter() + .filter(move |(_, pa)| { + // We only actually need to update the package if its Been changed + // since the last time we called prioritize. + // Which means it's highest decision level is the current decision level, + // or if we backtracked in the mean time. + check_all || pa.highest_decision_level == current_decision_level + }) + .filter_map(|(p, pa)| pa.assignments_intersection.potential_package_filter(p)) + } + pub fn pick_highest_priority_pkg( &mut self, prioritizer: impl Fn(&DP::P, &DP::VS) -> DP::Priority, From 5b911b102a9deec83ddeab3cddfe0ac129183f4c Mon Sep 17 00:00:00 2001 From: konstin Date: Wed, 31 Jul 2024 14:34:35 +0200 Subject: [PATCH 3/8] Use PEP440 style for range formatting --- src/range.rs | 2 +- tests/examples.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/range.rs b/src/range.rs index 378deb96..aca8655e 100644 --- a/src/range.rs +++ b/src/range.rs @@ -748,7 +748,7 @@ impl Display for Range { (Included(v), Unbounded) => write!(f, ">={v}")?, (Included(v), Included(b)) => { if v == b { - write!(f, "{v}")? + write!(f, "=={v}")? } else { write!(f, ">={v}, <={b}")? } diff --git a/tests/examples.rs b/tests/examples.rs index ed2677d3..10bdb379 100644 --- a/tests/examples.rs +++ b/tests/examples.rs @@ -237,13 +237,13 @@ fn confusing_with_lots_of_holes() { }; assert_eq!( &DefaultStringReporter::report(&derivation_tree), - r#"Because there is no available version for bar and foo 1 | 2 | 3 | 4 | 5 depends on bar, foo 1 | 2 | 3 | 4 | 5 is forbidden. -And because there is no version of foo in <1 | >1, <2 | >2, <3 | >3, <4 | >4, <5 | >5 and root 1 depends on foo, root 1 is forbidden."# + r#"Because there is no available version for bar and foo ==1 | ==2 | ==3 | ==4 | ==5 depends on bar, foo ==1 | ==2 | ==3 | ==4 | ==5 is forbidden. +And because there is no version of foo in <1 | >1, <2 | >2, <3 | >3, <4 | >4, <5 | >5 and root ==1 depends on foo, root ==1 is forbidden."# ); derivation_tree.collapse_no_versions(); assert_eq!( &DefaultStringReporter::report(&derivation_tree), - "Because foo depends on bar and root 1 depends on foo, root 1 is forbidden." + "Because foo depends on bar and root ==1 depends on foo, root ==1 is forbidden." ); assert_eq!( derivation_tree.packages(), From 45a39ae83d8a5aad93d7d7f105eacfbe274d2df6 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Thu, 16 Nov 2023 13:19:40 -0600 Subject: [PATCH 4/8] Add GitHub workflow to automatically tag each commit on `main` (#2) --- .github/workflows/permaref.yaml | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 .github/workflows/permaref.yaml diff --git a/.github/workflows/permaref.yaml b/.github/workflows/permaref.yaml new file mode 100644 index 00000000..29b5dbef --- /dev/null +++ b/.github/workflows/permaref.yaml @@ -0,0 +1,40 @@ +# Automatically creates a tag for each commit to `main` so when we rebase +# changes on top of the upstream, we retain permanent references to each +# previous commit so they are not orphaned and eventually deleted. +name: Create permanent reference + +on: + push: + branches: + - "main" + +jobs: + release: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Get the permanent ref number + id: get_version + run: | + # Enable pipefail so git command failures do not result in null versions downstream + set -x + + echo ::set-output name=LAST_PERMA_NUMBER::$(\ + git ls-remote --tags --refs --sort="v:refname" \ + https://github.com/zanieb/pubgrub.git | grep "tags/perma-" | tail -n1 | sed 's/.*\/perma-//' \ + ) + + - name: Configure Git + run: | + git config user.name "$GITHUB_ACTOR" + git config user.email "$GITHUB_ACTOR@users.noreply.github.com" + + - name: Create and push the new tag + run: | + TAG="perma-$((LAST_PERMA_NUMBER + 1))" + git tag -a "$TAG" -m "Automatically created on push to `main`" + git push origin "$TAG" + env: + LAST_PERMA_NUMBER: ${{ steps.get_version.outputs.LAST_PERMA_NUMBER }} From 0de551a57adc2ce54fb23867a6e12b66cc371aad Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Thu, 16 Nov 2023 13:36:19 -0600 Subject: [PATCH 5/8] Fix-ups to tag generating action (#3) * Use new GitHub output syntax * Fix tag message --- .github/workflows/permaref.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/permaref.yaml b/.github/workflows/permaref.yaml index 29b5dbef..eee99e6f 100644 --- a/.github/workflows/permaref.yaml +++ b/.github/workflows/permaref.yaml @@ -9,7 +9,7 @@ on: - "main" jobs: - release: + create-permaref: runs-on: ubuntu-latest steps: - name: Checkout @@ -21,10 +21,10 @@ jobs: # Enable pipefail so git command failures do not result in null versions downstream set -x - echo ::set-output name=LAST_PERMA_NUMBER::$(\ + echo "LAST_PERMA_NUMBER=$(\ git ls-remote --tags --refs --sort="v:refname" \ https://github.com/zanieb/pubgrub.git | grep "tags/perma-" | tail -n1 | sed 's/.*\/perma-//' \ - ) + )" >> $GITHUB_OUTPUT - name: Configure Git run: | @@ -34,7 +34,7 @@ jobs: - name: Create and push the new tag run: | TAG="perma-$((LAST_PERMA_NUMBER + 1))" - git tag -a "$TAG" -m "Automatically created on push to `main`" + git tag -a "$TAG" -m 'Automatically created on push to `main`' git push origin "$TAG" env: LAST_PERMA_NUMBER: ${{ steps.get_version.outputs.LAST_PERMA_NUMBER }} From 0bdab4c28b1d7c5a07ce7d58566e30818da58098 Mon Sep 17 00:00:00 2001 From: konstin Date: Sat, 1 Jun 2024 17:04:00 +0200 Subject: [PATCH 6/8] Merge external custom reasons --- src/report.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/report.rs b/src/report.rs index af232260..d2445225 100644 --- a/src/report.rs +++ b/src/report.rs @@ -144,6 +144,9 @@ impl DerivationTree // // Cannot be merged because the reason may not match DerivationTree::External(External::NoVersions(_, _)) => None, + DerivationTree::External(External::Custom(_, r, reason)) => Some( + DerivationTree::External(External::Custom(package, set.union(&r), reason)), + ), DerivationTree::External(External::FromDependencyOf(p1, r1, p2, r2)) => { if p1 == package { Some(DerivationTree::External(External::FromDependencyOf( @@ -161,8 +164,6 @@ impl DerivationTree ))) } } - // Cannot be merged because the reason may not match - DerivationTree::External(External::Custom(_, _, _)) => None, } } } From 2fac39371a47e7cb821e510aaa4de25405413d29 Mon Sep 17 00:00:00 2001 From: konsti Date: Wed, 19 Jun 2024 12:55:23 +0200 Subject: [PATCH 7/8] Add `State::add_incompatibility_from_dependencies` (#27) This wrapper avoids accessing the `incompatibility_store` directly in uv code. Before: ```rust let dep_incompats = self.pubgrub.add_version( package.clone(), version.clone(), dependencies, ); self.pubgrub.partial_solution.add_version( package.clone(), version.clone(), dep_incompats, &self.pubgrub.incompatibility_store, ); ``` After: ```rust self.pubgrub.add_incompatibility_from_dependencies(package.clone(), version.clone(), dependencies); ``` `add_incompatibility_from_dependencies` is one of the main methods for the custom interface to pubgrub. --- src/internal/core.rs | 20 ++++++++++++++++++++ src/internal/partial_solution.rs | 2 +- src/solver.rs | 10 +--------- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index dc2baf1c..afa349b9 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -66,6 +66,26 @@ impl State { } } + /// Add the dependencies for the current version of the current package as incompatibilities. + pub fn add_package_version_dependencies( + &mut self, + package: DP::P, + version: DP::V, + dependencies: impl IntoIterator, + ) { + let dep_incompats = self.add_incompatibility_from_dependencies( + package.clone(), + version.clone(), + dependencies, + ); + self.partial_solution.add_package_version_incompatibilities( + package.clone(), + version.clone(), + dep_incompats, + &self.incompatibility_store, + ) + } + /// Add an incompatibility to the state. pub fn add_incompatibility(&mut self, incompat: Incompatibility) { let id = self.incompatibility_store.alloc(incompat); diff --git a/src/internal/partial_solution.rs b/src/internal/partial_solution.rs index 83389b6f..32a6272c 100644 --- a/src/internal/partial_solution.rs +++ b/src/internal/partial_solution.rs @@ -362,7 +362,7 @@ impl PartialSolution { /// In practice I think it can only produce a conflict if one of the dependencies /// (which are used to make the new incompatibilities) /// is already in the partial solution with an incompatible version. - pub(crate) fn add_version( + pub(crate) fn add_package_version_incompatibilities( &mut self, package: DP::P, version: DP::V, diff --git a/src/solver.rs b/src/solver.rs index 9cf2142f..743ffcff 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -163,15 +163,7 @@ pub fn resolve( }; // Add that package and version if the dependencies are not problematic. - let dep_incompats = - state.add_incompatibility_from_dependencies(p.clone(), v.clone(), dependencies); - - state.partial_solution.add_version( - p.clone(), - v.clone(), - dep_incompats, - &state.incompatibility_store, - ); + state.add_package_version_dependencies(p.clone(), v.clone(), dependencies); } else { // `dep_incompats` are already in `incompatibilities` so we know there are not satisfied // terms and can add the decision directly. From b3a273d5d33edf87392064a98282df9122536167 Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Fri, 16 Aug 2024 19:51:20 -0400 Subject: [PATCH 8/8] consistency between `Range::iter` and `Range::bounding_range` --- src/range.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/range.rs b/src/range.rs index aca8655e..6f92477f 100644 --- a/src/range.rs +++ b/src/range.rs @@ -685,8 +685,10 @@ impl Range { } /// Iterate over the parts of the range. - pub fn iter(&self) -> impl Iterator, &Bound)> { - self.segments.iter().map(|(start, end)| (start, end)) + pub fn iter(&self) -> impl Iterator, Bound<&V>)> { + self.segments + .iter() + .map(|(start, end)| (start.as_ref(), end.as_ref())) } }