Skip to content

Commit

Permalink
Handle when multiple packages embed the same package
Browse files Browse the repository at this point in the history
The solver was panicking with "state not found" in the case where it
encounters a package that embeds some other package, but that other
package has already been embedded by some third package. It wanted to
"eject" the real package (again) but no "real" package was in the
solution at that point.

Now it checks that the package it wants to "eject" is a not an embedded
package. This situation was given its own `IncompatibleReason` so the
solver output will detail the packages involved.

Signed-off-by: J Robert Ray <jrray@jrray.org>
  • Loading branch information
jrray committed Aug 13, 2024
1 parent 48e0c44 commit 5607b12
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 1 deletion.
13 changes: 13 additions & 0 deletions crates/spk-schema/crates/foundation/src/version/compat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,26 @@ impl Ord for CompatRule {
/// Denotes whether or not something is compatible.
#[derive(Clone, Debug, Eq, PartialEq)]
pub enum IncompatibleReason {
AlreadyEmbeddedPackage {
embedded: PkgNameBuf,
embedded_by: PkgNameBuf,
},
ConflictingEmbeddedPackage(PkgNameBuf),
Other(String),
}

impl std::fmt::Display for IncompatibleReason {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
IncompatibleReason::AlreadyEmbeddedPackage {
embedded,
embedded_by,
} => {
write!(
f,
"embedded package {embedded} already embedded by another package in solve: {embedded_by}"
)
}
IncompatibleReason::ConflictingEmbeddedPackage(pkg) => {
write!(
f,
Expand Down
33 changes: 32 additions & 1 deletion crates/spk-solve/src/solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -643,13 +643,44 @@ impl Solver {
// solve. Jump back to before the conflicting
// package was added and try adding this
// build again.
let (_, _, state_id) = node
let (conflicting_pkg, conflicting_pkg_source, state_id) = node
.state
.get_current_resolve(&conflicting_package_name)
.map_err(|_| {
Error::String("package not found in resolve".into())
})?;

// Is the conflicting package already embedded
// by some other package?
if conflicting_pkg.ident().is_embedded() {
notes.push(Note::SkipPackageNote(SkipPackageNote::new(
spec.ident().to_any(),
Compatibility::Incompatible({
match conflicting_pkg_source {
PackageSource::Embedded { parent } => {
IncompatibleReason::AlreadyEmbeddedPackage {
embedded: conflicting_package_name,
embedded_by: parent.name().to_owned(),
}
}
_ => {
// As the solver exhausts
// all possibilities it
// eventually tries to
// resolve an embedded stub
// for a package that is
// already in the solve.
IncompatibleReason::ConflictingEmbeddedPackage(
conflicting_package_name,
)
}
}
}),
)));
self.number_builds_skipped += 1;
continue;
}

let graph_lock = graph.read().await;

let target_state_node = graph_lock
Expand Down
56 changes: 56 additions & 0 deletions crates/spk-solve/src/solver_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1421,6 +1421,62 @@ async fn test_solver_impossible_request_but_embedded_package_makes_solvable(mut
};
}

/// When multiple packages try to embed the same package the solver doesn't
/// panic.
#[rstest]
#[tokio::test]
async fn test_multiple_packages_embed_same_package(
mut solver: Solver,
#[values(true, false)] resolve_validation_impossible_checks: bool,
) {
init_logging();

let repo = make_repo!(
[
{
"pkg": "embedded-pkg/1.0.0",
"build": {"script": "echo BUILD"},
},
{
"pkg": "dep1/1.0.0",
"build": {"script": "echo BUILD"},
"install": {"embedded": [{"pkg": "embedded-pkg/1.0.0"}]},
},
{
"pkg": "dep2/1.0.0",
"build": {"script": "echo BUILD"},
"install": {"embedded": [{"pkg": "embedded-pkg/1.0.0"}]},
},
{
"pkg": "top-level/1.0.0",
"build": {"script": "echo BUILD"},
"install": {"requirements": [
// Resolve the "real" package first
{"pkg": "embedded-pkg"},
// dep1 embeds 'embedded-pkg' and should eject the real one
{"pkg": "dep1"},
// dep2 also embeds 'embedded-pkg' and in the past would
// cause the solver to panic
{"pkg": "dep2"}]},
},
]
);

solver.add_repository(Arc::new(repo));
solver.add_request(request!("top-level"));
solver.set_resolve_validation_impossible_checks(resolve_validation_impossible_checks);

match run_and_print_resolve_for_tests(&solver).await {
Err(Error::GraphError(spk_solve_graph::Error::FailedToResolve(_))) => {}
Ok(_) => {
panic!("No solution expected");
}
Err(err) => {
panic!("Expected FailedToResolve, but got this error: {err}");
}
};
}

#[rstest]
#[tokio::test]
async fn test_solver_with_impossible_checks_in_build_keys(mut solver: Solver) {
Expand Down

0 comments on commit 5607b12

Please sign in to comment.