Fix resolvo solver with migration-to-components

The solver tests were not passing if this feature is enabled, and this
exposed a few problems in not handling ':all' requests correctly. When
generating the "VersionSet" for dependencies, the PkgRequest needs to
have the correct component populated. The logic for skipping or not
skipping candidates when build from source needed to handle ':all' and
it was restructured for clarity. When filtering candidates for a request
of a component of a specific build, source builds needed to be filtered
out.

Signed-off-by: J Robert Ray <[email protected]>

Author: J Robert Ray

crates/spk-schema/crates/foundation/src/ident_build/build.rs

@@ -130,6 +130,10 @@ impl Build {
         }
     }
 
+    pub fn is_buildid(&self) -> bool {
+        matches!(self, Build::BuildId(_))
+    }
+
     pub fn is_source(&self) -> bool {
         matches!(self, Build::Source)
     }

crates/spk-solve/src/cdcl_solver/cdcl_solver_tests.rs

@@ -8,6 +8,7 @@ use rstest::rstest;
 use spk_schema::prelude::HasVersion;
 use spk_schema::{opt_name, Package};
 use spk_solve_macros::{make_repo, request};
+use tap::TapFallible;
 
 use super::Solver;
 use crate::cdcl_solver::AbstractSolverMut;
@@ -101,7 +102,7 @@ async fn package_with_dependency() {
 
     let mut solver = Solver::new(vec![repo.into()], Cow::Borrowed(&[]));
     solver.add_request(request!("needs-dep/1.0.0"));
-    let solution = solver.solve().await.unwrap();
+    let solution = solver.solve().await.tap_err(|e| eprintln!("{e}")).unwrap();
     assert_eq!(solution.len(), 2);
 }
 

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

@@ -322,7 +322,16 @@ impl AbstractSolver for Solver {
 
 #[async_trait::async_trait]
 impl AbstractSolverMut for Solver {
-    fn add_request(&mut self, request: Request) {
+    fn add_request(&mut self, mut request: Request) {
+        if let Request::Pkg(request) = &mut request {
+            if request.pkg.components.is_empty() {
+                if request.pkg.is_source() {
+                    request.pkg.components.insert(Component::Source);
+                } else {
+                    request.pkg.components.insert(Component::default_for_run());
+                }
+            }
+        }
         self.requests.push(request);
     }
 

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

@@ -69,7 +69,7 @@ use crate::AbstractSolverMut;
 // be a relationship between every component of a package and a "base"
 // component, to prevent a solve containing a mix of components from different
 // versions of the same package.
-#[derive(Clone, Eq, Hash, PartialEq)]
+#[derive(Clone, Debug, Eq, Hash, PartialEq)]
 pub(crate) struct PkgNameBufWithComponent {
     pub(crate) name: PkgNameBuf,
     pub(crate) component: SyntheticComponent,
@@ -128,7 +128,10 @@ impl ResolvoPackageName {
                 None
             }
             ResolvoPackageName::PkgNameBufWithComponent(pkg_name) => {
-                let mut located_builds = Vec::new();
+                // Prevent duplicate solvables by using a set. Ties (same build
+                // but "requires_build_from_source" differs) are resolved with
+                // "requires_build_from_source" being true.
+                let mut located_builds = HashSet::new();
 
                 let root_pkg_request = provider.global_pkg_requests.get(&pkg_name.name);
 
@@ -201,23 +204,62 @@ impl ResolvoPackageName {
                                     // when a request for it is found it can act as a
                                     // surrogate that depends on all the individual
                                     // components.
-                                    [Component::All],
+                                    {
+                                        if !build.is_source() {
+                                            itertools::Either::Left([Component::All].into_iter())
+                                        } else {
+                                            // XXX: Unclear if this is the right
+                                            // approach but without this special
+                                            // case the Solution can incorrectly
+                                            // end up with a src build marked as
+                                            // requires_build_from_source for
+                                            // requests that are asking for
+                                            // :src.
+                                            itertools::Either::Right([].into_iter())
+                                        }
+                                    },
                                 ) {
-                                    if component != *pkg_name_component
-                                        && (provider.binary_only || !component.is_source())
+                                    let requires_build_from_source = build.is_source()
+                                        && (component != *pkg_name_component
+                                            || pkg_name_component.is_all());
+
+                                    if requires_build_from_source && provider.binary_only {
+                                        // Deny anything that requires build
+                                        // from source when binary_only is
+                                        // enabled.
+                                        continue;
+                                    }
+
+                                    if (!requires_build_from_source || !build.is_source())
+                                        && component != *pkg_name_component
                                     {
+                                        // Deny components that don't match
+                                        // unless it is possible to build from
+                                        // source.
                                         continue;
                                     }
 
-                                    located_builds.push(LocatedBuildIdentWithComponent {
+                                    let new_entry = LocatedBuildIdentWithComponent {
                                         ident: located_build_ident.clone(),
                                         component: pkg_name.component.clone(),
-                                        requires_build_from_source: component
-                                            != *pkg_name_component,
-                                    });
+                                        requires_build_from_source,
+                                    };
+
+                                    if requires_build_from_source {
+                                        // _replace_ any existing entry, which
+                                        // might have
+                                        // requires_build_from_source == false,
+                                        // so it now is true.
+                                        located_builds.replace(new_entry);
+                                    } else {
+                                        // _insert_ to not overwrite any
+                                        // existing entry that might have
+                                        // requires_build_from_source == true.
+                                        located_builds.insert(new_entry);
+                                    }
                                 }
                             } else {
-                                located_builds.push(LocatedBuildIdentWithComponent {
+                                located_builds.insert(LocatedBuildIdentWithComponent {
                                     ident: located_build_ident,
                                     component: SyntheticComponent::Base,
                                     requires_build_from_source: false,
@@ -538,21 +580,21 @@ impl SpkProvider {
     fn pkg_request_to_known_dependencies(&self, pkg_request: &PkgRequest) -> KnownDependencies {
         let mut components = pkg_request.pkg.components.iter().peekable();
         let iter = if components.peek().is_some() {
-            itertools::Either::Right(components.cloned().map(SyntheticComponent::Actual))
+            itertools::Either::Right(components.cloned())
         } else {
             itertools::Either::Left(
                 // A request with no components is assumed to be a request for
-                // the run (or source) component.
+                // the default_for_run (or source) component.
                 if pkg_request
                     .pkg
                     .build
                     .as_ref()
                     .map(|build| build.is_source())
                     .unwrap_or(false)
                 {
-                    std::iter::once(SyntheticComponent::Actual(Component::Source))
+                    std::iter::once(Component::Source)
                 } else {
-                    std::iter::once(SyntheticComponent::Actual(Component::Run))
+                    std::iter::once(Component::default_for_run())
                 },
             )
         };
@@ -566,12 +608,14 @@ impl SpkProvider {
                     .intern_package_name(ResolvoPackageName::PkgNameBufWithComponent(
                         PkgNameBufWithComponent {
                             name: pkg_request.pkg.name().to_owned(),
-                            component,
+                            component: SyntheticComponent::Actual(component.clone()),
                         },
                     ));
+            let mut pkg_request_with_component = pkg_request.clone();
+            pkg_request_with_component.pkg.components = BTreeSet::from_iter([component]);
             let dep_vs = self.pool.intern_version_set(
                 dep_name,
-                RequestVS::SpkRequest(Request::Pkg(pkg_request.clone())),
+                RequestVS::SpkRequest(Request::Pkg(pkg_request_with_component)),
             );
             match pkg_request.inclusion_policy {
                 spk_schema::ident::InclusionPolicy::Always => {
@@ -761,7 +805,7 @@ impl SpkProvider {
 
         // If neither build has a key, both packages failed to load?
         // Add debug assert to see if this ever happens.
-        debug_assert!(false, "builds without keys");
+        debug_assert!(false, "builds without keys {a:?} {b:?}");
 
         a.1.ident.cmp(&b.1.ident)
     }
@@ -885,6 +929,22 @@ impl DependencyProvider for SpkProvider {
                         // a candidate. Source builds that can't be built from
                         // source are filtered out in `get_candidates`.
                         if located_build_ident_with_component.requires_build_from_source {
+                            // However, building from source is not a suitable
+                            // candidate for a request for a specific component
+                            // of an existing build, such as when finding the
+                            // members of the :all component of a build.
+                            if pkg_request
+                                .pkg
+                                .build
+                                .as_ref()
+                                .is_some_and(|b| b.is_buildid())
+                            {
+                                if inverse {
+                                    selected.push(*candidate);
+                                }
+                                continue;
+                            }
+
                             if !inverse {
                                 selected.push(*candidate);
                             }

crates/spk-solve/src/solver_test.rs

@@ -235,7 +235,8 @@ async fn test_solver_package_with_no_recipe_and_impossible_initial_checks(
     let res = run_and_print_resolve_for_tests(&mut solver).await;
     if cfg!(feature = "migration-to-components") {
         match res {
-            Err(Error::InitialRequestsContainImpossibleError(_)) => {
+            Err(Error::InitialRequestsContainImpossibleError(_))
+            | Err(Error::FailedToResolve(_)) => {
                 // Success, when the 'migration-to-components' feature
                 // is enabled because the initial checks for
                 // impossible requests fail because the package does
@@ -332,7 +333,11 @@ async fn test_solver_package_with_no_recipe_from_cmd_line_and_impossible_initial
         // :build and a :run component to pass and it only has a :run
         // component
         assert!(
-            matches!(res, Err(Error::InitialRequestsContainImpossibleError(_))),
+            matches!(
+                res,
+                Err(Error::InitialRequestsContainImpossibleError(_))
+                    | Err(Error::FailedToResolve(_))
+            ),
             "'{res:?}' should be a Error::String('Initial requests contain 1 impossible request.')",
         );
     } else {