Skip to content

Commit ab28dbe

Browse files
jrrayjrray
committed
More work on solving embedded packages
Pass one more test, handling the mix of embedded packages and components. There's still that `todo!` in the middle of `get_dependencies` that no test has run into yet. Signed-off-by: J Robert Ray <[email protected]>
1 parent 178ba78 commit ab28dbe

File tree

3 files changed

+177
-35
lines changed

3 files changed

+177
-35
lines changed

‎crates/spk-solve/src/cdcl_solver/mod.rs‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,13 @@ mod pkg_request_version_set;
66
mod spk_provider;
77

88
use std::borrow::Cow;
9-
use std::collections::{BTreeMap, BTreeSet};
9+
use std::collections::{BTreeMap, BTreeSet, HashMap};
1010
use std::sync::Arc;
1111

1212
use pkg_request_version_set::{SpkSolvable, SyntheticComponent};
1313
use spk_provider::SpkProvider;
1414
use spk_schema::ident::{InclusionPolicy, PinPolicy, PkgRequest, RangeIdent};
15+
use spk_schema::ident_component::Component;
1516
use spk_schema::prelude::{HasVersion, Named, Versioned};
1617
use spk_schema::version_range::VersionFilter;
1718
use spk_schema::{OptionMap, Package, Request};
@@ -123,13 +124,42 @@ impl AbstractSolver for Solver {
123124

124125
let mut solution_options = OptionMap::default();
125126
let mut solution_adds = Vec::with_capacity(solvables.len());
127+
// Keep track of the index of each package added to `solution_adds` in
128+
// order to merge components. Components of a package come out of the
129+
// solver as separate solvables. The solver logic guarantees that any
130+
// two entries in this list with the same package name are for the same
131+
// package and this merging of components is valid.
132+
let mut seen_packages = HashMap::new();
126133
for located_build_ident_with_component in solvables {
127134
let SyntheticComponent::Actual(solvable_component) =
128135
&located_build_ident_with_component.component
129136
else {
130137
continue;
131138
};
132139

140+
if let Some(existing_index) =
141+
seen_packages.get(located_build_ident_with_component.ident.name())
142+
{
143+
if let Some((
144+
PkgRequest {
145+
pkg: RangeIdent { components, .. },
146+
..
147+
},
148+
_,
149+
_,
150+
)) = solution_adds.get_mut(*existing_index)
151+
{
152+
// If we visit a solvable for the "All" component, the
153+
// solver guarantees that we will have all the components.
154+
if !components.contains(&Component::All) {
155+
components.insert(solvable_component.clone());
156+
} else if solvable_component.is_all() {
157+
*components = BTreeSet::from([Component::All]);
158+
}
159+
}
160+
continue;
161+
}
162+
133163
let pkg_request = PkgRequest {
134164
pkg: RangeIdent {
135165
repository_name: None,
@@ -181,6 +211,11 @@ impl AbstractSolver for Solver {
181211
}
182212
}
183213
}
214+
let next_index = solution_adds.len();
215+
seen_packages.insert(
216+
located_build_ident_with_component.ident.name().to_owned(),
217+
next_index,
218+
);
184219
solution_adds.push((
185220
pkg_request,
186221
package,

‎crates/spk-solve/src/cdcl_solver/spk_provider.rs‎

Lines changed: 137 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use std::cell::{Cell, RefCell};
66
use std::collections::{BTreeSet, HashMap, HashSet};
7+
use std::str::FromStr;
78
use std::sync::Arc;
89

910
use resolvo::utils::Pool;
@@ -35,8 +36,9 @@ use spk_schema::ident_build::{Build, EmbeddedSource, EmbeddedSourcePackage};
3536
use spk_schema::ident_component::Component;
3637
use spk_schema::name::{OptNameBuf, PkgNameBuf};
3738
use spk_schema::prelude::{HasVersion, Named};
39+
use spk_schema::version::Version;
3840
use spk_schema::version_range::{DoubleEqualsVersion, VersionFilter};
39-
use spk_schema::{Opt, Package, Request, VersionIdent};
41+
use spk_schema::{BuildIdent, Opt, Package, Request, VersionIdent};
4042
use spk_storage::RepositoryHandle;
4143

4244
use super::pkg_request_version_set::{
@@ -467,21 +469,31 @@ impl DependencyProvider for SpkProvider {
467469
let located_build_ident =
468470
LocatedBuildIdent::new(repo.name().to_owned(), build.clone());
469471
if let SyntheticComponent::Actual(pkg_name_component) = &pkg_name.component {
470-
if let Build::Embedded(EmbeddedSource::Package(parent)) = build.build() {
471-
// Embedded packages don't have components, but
472-
// their parent has a component.
473-
if parent.components.contains(pkg_name_component) {
474-
located_builds.push(LocatedBuildIdentWithComponent {
475-
ident: located_build_ident,
476-
component: SyntheticComponent::Actual(
477-
pkg_name_component.clone(),
478-
),
479-
});
480-
}
481-
continue;
482-
}
483-
let components =
484-
repo.list_build_components(&build).await.unwrap_or_default();
472+
let components = if let Build::Embedded(EmbeddedSource::Package(_parent)) =
473+
build.build()
474+
{
475+
// Does this embedded stub contain the component
476+
// being requested? For whatever reason,
477+
// list_build_components returns an empty list for
478+
// embedded stubs.
479+
itertools::Either::Right(
480+
if let Ok(stub) = repo.read_embed_stub(&build).await {
481+
itertools::Either::Right(
482+
stub.components()
483+
.iter()
484+
.map(|component_spec| component_spec.name.clone())
485+
.collect::<Vec<_>>()
486+
.into_iter(),
487+
)
488+
} else {
489+
itertools::Either::Left(std::iter::empty())
490+
},
491+
)
492+
} else {
493+
itertools::Either::Left(
494+
repo.list_build_components(&build).await.unwrap_or_default(),
495+
)
496+
};
485497
for component in components.into_iter().chain(
486498
// A build representing the All component is included so
487499
// when a request for it is found it can act as a
@@ -615,12 +627,15 @@ impl DependencyProvider for SpkProvider {
615627
else {
616628
return Dependencies::Known(KnownDependencies::default());
617629
};
618-
if let SyntheticComponent::Base = located_build_ident_with_component.component {
619-
// Base can't depend on anything because we don't know what
620-
// components actually exist or if requests exist for whatever it
621-
// was we picked if we were to pick a component to depend on.
622-
return Dependencies::Known(KnownDependencies::default());
623-
}
630+
let actual_component = match &located_build_ident_with_component.component {
631+
SyntheticComponent::Base => {
632+
// Base can't depend on anything because we don't know what
633+
// components actually exist or if requests exist for whatever it
634+
// was we picked if we were to pick a component to depend on.
635+
return Dependencies::Known(KnownDependencies::default());
636+
}
637+
SyntheticComponent::Actual(component) => component,
638+
};
624639
// XXX: This find runtime will add up.
625640
let repo = self
626641
.repos
@@ -666,9 +681,7 @@ impl DependencyProvider for SpkProvider {
666681
);
667682
}
668683
return Dependencies::Known(known_deps);
669-
} else if let SyntheticComponent::Actual(ref solvable_component) =
670-
located_build_ident_with_component.component
671-
{
684+
} else {
672685
// For any non-All/non-Base component, add a dependency on
673686
// the base to ensure all components come from the same
674687
// base version.
@@ -692,7 +705,7 @@ impl DependencyProvider for SpkProvider {
692705
if let Some(component_spec) = package
693706
.components()
694707
.iter()
695-
.find(|component_spec| component_spec.name == *solvable_component)
708+
.find(|component_spec| component_spec.name == *actual_component)
696709
{
697710
component_spec.uses.iter().for_each(|uses| {
698711
let dep_name = self.pool.intern_package_name(PkgNameBufWithComponent {
@@ -718,8 +731,6 @@ impl DependencyProvider for SpkProvider {
718731
}
719732
// Also add dependencies on any packages embedded in this
720733
// component.
721-
// XXX: This ignores the detail of embeds inside components for
722-
// now.
723734
for embedded in package.embedded().iter() {
724735
let dep_name = self.pool.intern_package_name(PkgNameBufWithComponent {
725736
name: embedded.name().to_owned(),
@@ -746,12 +757,11 @@ impl DependencyProvider for SpkProvider {
746757
),
747758
// This needs to match the build of
748759
// the stub for get_candidates to like
749-
// it.
760+
// it. Stub parents are always the Run
761+
// component.
750762
build: Some(Build::Embedded(EmbeddedSource::Package(
751763
Box::new(EmbeddedSourcePackage {
752764
ident: package.ident().into(),
753-
// XXX: Hard coded to Run for
754-
// now.
755765
components: BTreeSet::from_iter([Component::Run]),
756766
}),
757767
))),
@@ -777,6 +787,102 @@ impl DependencyProvider for SpkProvider {
777787
todo!("{embedded_component_requirement:?}");
778788
}
779789
}
790+
// If this solvable is an embedded stub and it is
791+
// representing that it provides a component that lives in a
792+
// component of the parent, then that parent component needs
793+
// to be included in the solution.
794+
if let Build::Embedded(EmbeddedSource::Package(parent)) =
795+
located_build_ident_with_component.ident.build()
796+
{
797+
match actual_component {
798+
Component::Run => {
799+
// The Run component is the default "home" of
800+
// embedded packages, no dependency needed in this
801+
// case.
802+
}
803+
component => 'invalid_parent: {
804+
// XXX: Do we not have a convenient way to read the
805+
// parent package from an embedded stub ident?
806+
let Ok(pkg_name) = PkgNameBuf::from_str(&parent.ident.pkg_name) else {
807+
break 'invalid_parent;
808+
};
809+
let Some(version_str) = parent.ident.version_str.as_ref() else {
810+
break 'invalid_parent;
811+
};
812+
let Ok(version) = Version::from_str(&version_str) else {
813+
break 'invalid_parent;
814+
};
815+
let Some(build_str) = parent.ident.build_str.as_ref() else {
816+
break 'invalid_parent;
817+
};
818+
let Ok(build) = Build::from_str(build_str) else {
819+
break 'invalid_parent;
820+
};
821+
let ident = BuildIdent::new(
822+
VersionIdent::new(pkg_name, version),
823+
build.clone(),
824+
);
825+
let Ok(parent) = repo.read_package(&ident).await else {
826+
break 'invalid_parent;
827+
};
828+
// Look through the components of the parent to see
829+
// if one (or more?) of them embeds this component.
830+
for parent_component in parent.components().iter() {
831+
parent_component
832+
.embedded
833+
.iter()
834+
.filter(|embedded_package| {
835+
embedded_package.pkg.name()
836+
== located_build_ident_with_component.ident.name()
837+
&& embedded_package.components().contains(component)
838+
})
839+
.for_each(|_embedded_package| {
840+
let dep_name = self.pool.intern_package_name(
841+
PkgNameBufWithComponent {
842+
name: ident.name().to_owned(),
843+
component: SyntheticComponent::Actual(
844+
parent_component.name.clone(),
845+
),
846+
},
847+
);
848+
known_deps.requirements.push(
849+
self.pool.intern_version_set(
850+
dep_name,
851+
RequestVS::SpkRequest(Request::Pkg(
852+
PkgRequest::new(
853+
RangeIdent {
854+
repository_name: Some(
855+
located_build_ident_with_component
856+
.ident
857+
.repository_name()
858+
.to_owned(),
859+
),
860+
name: ident.name().to_owned(),
861+
components: BTreeSet::from_iter([
862+
parent_component.name.clone(),
863+
]),
864+
version: VersionFilter::single(
865+
DoubleEqualsVersion::version_range(
866+
ident.version().clone(),
867+
),
868+
),
869+
build: Some(build.clone()),
870+
},
871+
RequestedBy::Embedded(
872+
located_build_ident_with_component
873+
.ident
874+
.target()
875+
.clone(),
876+
),
877+
),
878+
))
879+
).into(),
880+
);
881+
});
882+
}
883+
}
884+
}
885+
}
780886
for option in package.get_build_options() {
781887
let Opt::Var(var_opt) = option else {
782888
continue;

‎crates/spk-solve/src/solver_test.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2058,8 +2058,6 @@ async fn test_solver_components(#[case] mut solver: SolverImpl) {
20582058

20592059
#[rstest]
20602060
#[case::og(og_solver())]
2061-
// Remove #[should_panic] once cdcl handles this case
2062-
#[should_panic]
20632061
#[case::cdcl(cdcl_solver())]
20642062
#[tokio::test]
20652063
async fn test_solver_components_interaction_with_embeds(#[case] mut solver: SolverImpl) {
@@ -2123,7 +2121,10 @@ async fn test_solver_components_interaction_with_embeds(#[case] mut solver: Solv
21232121
solver.add_request(request!("fake-pkg:comp1"));
21242122
solver.add_request(request!("victim"));
21252123

2126-
let Ok(solution) = run_and_print_resolve_for_tests(&mut solver).await else {
2124+
let Ok(solution) = run_and_print_resolve_for_tests(&mut solver)
2125+
.await
2126+
.tap_err(|e| eprintln!("{e}"))
2127+
else {
21272128
panic!("Expected a valid solution");
21282129
};
21292130

0 commit comments

Comments
 (0)