Skip to content
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

Handle when multiple packages embed the same package #1102

Merged
merged 1 commit into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading