-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PubGrub resolver #1453
base: spr/main/293590d9
Are you sure you want to change the base?
PubGrub resolver #1453
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
use crate::core::{ManifestDependency, Summary}; | ||
use once_map::OnceMap; | ||
use std::sync::Arc; | ||
|
||
/// In-memory index of package metadata. | ||
#[derive(Default, Clone)] | ||
pub struct InMemoryIndex(Arc<SharedInMemoryIndex>); | ||
|
||
#[derive(Default)] | ||
struct SharedInMemoryIndex { | ||
/// A map from package name to the metadata for that package and the index where the metadata | ||
/// came from. | ||
packages: FxOnceMap<ManifestDependency, Arc<VersionsResponse>>, | ||
} | ||
|
||
pub(crate) type FxOnceMap<K, V> = OnceMap<K, V>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? |
||
|
||
impl InMemoryIndex { | ||
/// Returns a reference to the package metadata. | ||
pub fn packages(&self) -> &FxOnceMap<ManifestDependency, Arc<VersionsResponse>> { | ||
&self.0.packages | ||
} | ||
} | ||
|
||
#[derive(Debug)] | ||
pub enum VersionsResponse { | ||
Found(Vec<Summary>), | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,153 @@ | ||
use crate::core::lockfile::Lockfile; | ||
use crate::core::registry::Registry; | ||
use crate::core::{Resolve, Summary}; | ||
use crate::core::{PackageId, Resolve, Summary}; | ||
use crate::resolver::algorithm::provider::{ | ||
rewrite_locked_dependency, DependencyProviderError, PubGrubDependencyProvider, PubGrubPackage, | ||
}; | ||
use crate::resolver::algorithm::solution::{build_resolve, validate_solution}; | ||
use crate::resolver::algorithm::state::{Request, ResolverState}; | ||
use anyhow::bail; | ||
use futures::{FutureExt, TryFutureExt}; | ||
use itertools::Itertools; | ||
use pubgrub::PubGrubError; | ||
use pubgrub::{DefaultStringReporter, Reporter}; | ||
use pubgrub::{Incompatibility, State}; | ||
use std::collections::HashSet; | ||
use std::sync::Arc; | ||
use std::thread; | ||
use tokio::sync::{mpsc, oneshot}; | ||
|
||
#[tracing::instrument(level = "trace", skip_all)] | ||
pub async fn resolve( | ||
_summaries: &[Summary], | ||
_registry: &dyn Registry, | ||
_lockfile: Lockfile, | ||
mod in_memory_index; | ||
mod provider; | ||
mod solution; | ||
mod state; | ||
|
||
pub async fn resolve<'c>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is the core algorithm, so I think it would be good to have it commented (at least the above the signature, and even better describe the steps of the algorithm). |
||
summaries: &[Summary], | ||
registry: &dyn Registry, | ||
lockfile: Lockfile, | ||
) -> anyhow::Result<Resolve> { | ||
todo!("implement") | ||
let state = Arc::new(ResolverState::default()); | ||
|
||
let (request_sink, request_stream): (mpsc::Sender<Request>, mpsc::Receiver<Request>) = | ||
mpsc::channel(300); | ||
|
||
let requests_fut = state | ||
.clone() | ||
.fetch(registry, request_stream) | ||
.map_err(|err| anyhow::format_err!(err)) | ||
.fuse(); | ||
|
||
for summary in summaries { | ||
for dep in summary.full_dependencies() { | ||
let locked_package_id = lockfile.packages_matching(dep.clone()); | ||
let dep = if let Some(locked_package_id) = locked_package_id { | ||
rewrite_locked_dependency(dep.clone(), locked_package_id?) | ||
} else { | ||
dep.clone() | ||
}; | ||
if state.index.packages().register(dep.clone()) { | ||
request_sink.send(Request::Package(dep.clone())).await?; | ||
} | ||
} | ||
} | ||
|
||
let main_package_ids: HashSet<PackageId> = | ||
HashSet::from_iter(summaries.iter().map(|sum| sum.package_id)); | ||
|
||
let (tx, rx) = oneshot::channel(); | ||
|
||
let cloned_lockfile = lockfile.clone(); | ||
thread::Builder::new() | ||
.name("scarb-resolver".into()) | ||
.spawn(move || { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider moving this to a separate named function to: reduce indentation, have something named in stack trace |
||
let result = || { | ||
let provider = PubGrubDependencyProvider::new( | ||
main_package_ids, | ||
state, | ||
request_sink, | ||
cloned_lockfile, | ||
); | ||
|
||
// Init state | ||
let main_package_ids = provider | ||
.main_package_ids() | ||
.clone() | ||
.into_iter() | ||
.collect_vec(); | ||
|
||
let Some((first, rest)) = main_package_ids.split_first() else { | ||
bail!("empty summaries"); | ||
}; | ||
let package: PubGrubPackage = (*first).into(); | ||
let version = first.version.clone(); | ||
let mut state = State::init(package.clone(), version); | ||
state | ||
.unit_propagation(package.clone()) | ||
.map_err(|err| anyhow::format_err!("unit propagation failed: {:?}", err))?; | ||
for package_id in rest { | ||
let package: PubGrubPackage = (*package_id).into(); | ||
let version = package_id.version.clone(); | ||
state.add_incompatibility(Incompatibility::not_root( | ||
package.clone(), | ||
version.clone(), | ||
)); | ||
state | ||
.unit_propagation(package) | ||
.map_err(|err| anyhow::format_err!("unit propagation failed: {:?}", err))? | ||
} | ||
|
||
// Resolve requirements | ||
let solution = | ||
pubgrub::resolve_state(&provider, &mut state, package).map_err(format_error)?; | ||
|
||
validate_solution(&solution)?; | ||
build_resolve(&provider, solution) | ||
}; | ||
let result = result(); | ||
tx.send(result).unwrap(); | ||
})?; | ||
|
||
let resolve_fut = async move { | ||
rx.await | ||
.map_err(|_| DependencyProviderError::ChannelClosed.into()) | ||
.and_then(|result| result) | ||
}; | ||
|
||
let (_, resolve) = tokio::try_join!(requests_fut, resolve_fut)?; | ||
resolve.check_checksums(&lockfile)?; | ||
Ok(resolve) | ||
} | ||
|
||
fn format_error(err: PubGrubError<PubGrubDependencyProvider>) -> anyhow::Error { | ||
match err { | ||
PubGrubError::NoSolution(derivation_tree) => { | ||
anyhow::format_err!( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: import directly |
||
"version solving failed:\n{}\n", | ||
DefaultStringReporter::report(&derivation_tree) | ||
) | ||
} | ||
PubGrubError::ErrorChoosingPackageVersion(DependencyProviderError::PackageNotFound { | ||
name, | ||
version, | ||
}) => { | ||
anyhow::format_err!("cannot find package `{name} {version}`") | ||
} | ||
PubGrubError::ErrorChoosingPackageVersion(DependencyProviderError::PackageQueryFailed( | ||
err, | ||
)) => anyhow::format_err!("{}", err).context("dependency query failed"), | ||
PubGrubError::ErrorRetrievingDependencies { | ||
package, | ||
version, | ||
source, | ||
} => anyhow::Error::from(source) | ||
.context(format!("cannot get dependencies of `{package}@{version}`")), | ||
PubGrubError::ErrorInShouldCancel(err) => { | ||
anyhow::format_err!("{}", err).context("should cancel failed") | ||
} | ||
PubGrubError::Failure(msg) => anyhow::format_err!("{}", msg).context("resolver failure"), | ||
PubGrubError::ErrorChoosingPackageVersion(DependencyProviderError::ChannelClosed) => { | ||
anyhow::format_err!("channel closed") | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you doing this instead of just depending on our fork directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
semver-pubgrub
depends onpubgrub
as well 🫤There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you change this dependency in our semver-pubgrub fork?