Fix requests for :all not filtering invalid versions

Messed up the logic when checking :all cases. Add test for this case.

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

Author: J Robert Ray

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

@@ -679,6 +679,7 @@ impl DependencyProvider for SpkProvider {
                     let compatible = pkg_request
                         .is_version_applicable(located_build_ident_with_component.ident.version());
                     if compatible.is_ok() {
+                        tracing::trace!(pkg_request = %pkg_request.pkg, version = %located_build_ident_with_component.ident.version(), "version applicable");
                         let is_source =
                             located_build_ident_with_component.ident.build().is_source();
 
@@ -710,52 +711,57 @@ impl DependencyProvider for SpkProvider {
                         // Only select All component for requests of All
                         // component.
                         if located_build_ident_with_component.component.is_all() {
-                            if pkg_request.pkg.components.contains(&Component::All) ^ inverse {
-                                selected.push(*candidate);
-                            }
-                            continue;
-                        } else
-                        // Only the All component can satisfy requests for All.
-                        if pkg_request.pkg.components.contains(&Component::All) {
-                            if inverse {
-                                selected.push(*candidate);
-                            }
-                            continue;
-                        }
-
-                        // Only the x component can satisfy requests for x.
-                        let mut at_least_one_request_matched_this_solvable = None;
-                        for component in pkg_request.pkg.components.iter() {
-                            if component.is_all() {
-                                continue;
-                            }
-                            if component == &located_build_ident_with_component.component {
-                                at_least_one_request_matched_this_solvable = Some(true);
-                                break;
-                            } else {
-                                at_least_one_request_matched_this_solvable = Some(false);
-                            }
-                        }
-
-                        match at_least_one_request_matched_this_solvable {
-                            Some(true) => {
+                            // This can disqualify but not qualify; version
+                            // compatibility check is still required.
+                            if !pkg_request.pkg.components.contains(&Component::All) {
                                 if inverse {
-                                    continue;
+                                    selected.push(*candidate);
                                 }
+                                continue;
                             }
-                            Some(false) => {
-                                // The request is for specific components but
-                                // this solvable doesn't match any of them.
+                        } else {
+                            // Only the All component can satisfy requests for All.
+                            if pkg_request.pkg.components.contains(&Component::All) {
                                 if inverse {
                                     selected.push(*candidate);
+                                }
+                                continue;
+                            }
+
+                            // Only the x component can satisfy requests for x.
+                            let mut at_least_one_request_matched_this_solvable = None;
+                            for component in pkg_request.pkg.components.iter() {
+                                if component.is_all() {
                                     continue;
                                 }
+                                if component == &located_build_ident_with_component.component {
+                                    at_least_one_request_matched_this_solvable = Some(true);
+                                    break;
+                                } else {
+                                    at_least_one_request_matched_this_solvable = Some(false);
+                                }
                             }
-                            None => {
-                                // TODO: if at_least_one_request_matched_this_solvable
-                                // is None it means the request didn't specify a
-                                // component. Decide which specific component this
-                                // should match.
+
+                            match at_least_one_request_matched_this_solvable {
+                                Some(true) => {
+                                    if inverse {
+                                        continue;
+                                    }
+                                }
+                                Some(false) => {
+                                    // The request is for specific components but
+                                    // this solvable doesn't match any of them.
+                                    if inverse {
+                                        selected.push(*candidate);
+                                        continue;
+                                    }
+                                }
+                                None => {
+                                    // TODO: if at_least_one_request_matched_this_solvable
+                                    // is None it means the request didn't specify a
+                                    // component. Decide which specific component this
+                                    // should match.
+                                }
                             }
                         }
 
@@ -772,6 +778,7 @@ impl DependencyProvider for SpkProvider {
                             .await
                         {
                             if pkg_request.is_satisfied_by(&package).is_ok() ^ inverse {
+                                tracing::trace!(pkg_request = %pkg_request.pkg, package = %package.ident(), %inverse, "satisfied by");
                                 selected.push(*candidate);
                             }
                         } else if inverse {

crates/spk-solve/src/solver_test.rs

@@ -45,12 +45,15 @@ fn solver() -> Solver {
 /// of resolved components, or the specific build of the package.
 macro_rules! assert_resolved {
     ($solution:ident, $pkg:literal, $version:literal) => {
-        assert_resolved!($solution, $pkg, $version, "wrong package version was resolved")
+        assert_resolved!($solution, $pkg, version = $version, "wrong package version was resolved")
+    };
+    ($solution:ident, $pkg:literal, version = $version:expr) => {
+        assert_resolved!($solution, $pkg, version = $version, "wrong package version was resolved")
     };
     ($solution:ident, $pkg:literal, $version:literal, $message:literal) => {
         assert_resolved!($solution, $pkg, version = $version, $message)
     };
-    ($solution:ident, $pkg:literal, version = $version:literal, $message:literal) => {{
+    ($solution:ident, $pkg:literal, version = $version:expr, $message:literal) => {{
         let pkg = $solution
             .get($pkg)
             .expect("expected package to be in solution");
@@ -2912,3 +2915,30 @@ async fn test_version_number_masking(
     );
     assert_ne!(resolved.spec.ident().build(), &Build::Source);
 }
+
+#[rstest]
+#[case::og(og_solver())]
+#[case::cdcl(cdcl_solver())]
+#[tokio::test]
+async fn request_for_all_component_picks_correct_version(
+    #[case] mut solver: SolverImpl,
+    #[values("1.0.0", "2.0.0", "3.0.0")] version: &str,
+) {
+    // A request for :all component still controls for version compatibility
+
+    let repo = make_repo!(
+        [
+            { "pkg": "mypkg/1.0.0" },
+            { "pkg": "mypkg/2.0.0" },
+            { "pkg": "mypkg/3.0.0" },
+        ]
+    );
+    let repo = Arc::new(repo);
+
+    solver.add_repository(repo);
+    let request_str = format!("mypkg:all/{version}");
+    solver.add_request(request!(request_str));
+
+    let solution = run_and_print_resolve_for_tests(&mut solver).await.unwrap();
+    assert_resolved!(solution, "mypkg", version = version);
+}