Bugfix: get_unresolved_requests() and incompatible requests (#1138)

Removes unwrap, and changes function to return a Result for better error handling

Signed-off-by: David Gilligan-Cook <[email protected]>

Author: dcookspi

crates/spk-cli/group1/src/cmd_bake.rs

@@ -11,6 +11,10 @@ use spk_schema::ident::RequestedBy;
 use spk_schema::Package;
 use spk_solve::solution::{get_spfs_layers_to_packages, LayerPackageAndComponents, PackageSource};
 
+#[cfg(test)]
+#[path = "./cmd_bake_test.rs"]
+mod cmd_bake_test;
+
 // Verbosity level above which repo and component names will be
 // included in the package display values.
 const NO_VERBOSITY: u8 = 0;

crates/spk-cli/group1/src/cmd_bake_test.rs

@@ -0,0 +1,107 @@
+// Copyright (c) Contributors to the SPK project.
+// SPDX-License-Identifier: Apache-2.0
+// https://github.com/spkenv/spk
+
+use clap::Parser;
+use rstest::rstest;
+use spfs::config::Remote;
+use spfs::RemoteAddress;
+use spk_cli_common::Run;
+use spk_solve::{recipe, spec, Component};
+use spk_storage::fixtures::{empty_layer_digest, spfs_runtime, spfsrepo};
+
+use super::Bake;
+
+#[derive(Parser)]
+struct Opt {
+    #[clap(flatten)]
+    bake: Bake,
+}
+
+#[rstest]
+#[tokio::test]
+async fn test_bake() {
+    // Test the bake command runs
+    let mut rt = spfs_runtime().await;
+    let remote_repo = spfsrepo().await;
+
+    // Populate the "origin" repo with one package.
+    // The "local" repo is empty.
+    rt.add_remote_repo(
+        "origin",
+        Remote::Address(RemoteAddress {
+            address: remote_repo.address().clone(),
+        }),
+    )
+    .unwrap();
+
+    let recipe = recipe!({"pkg": "my-pkg/1.0.1"});
+    remote_repo.publish_recipe(&recipe).await.unwrap();
+    let spec = spec!({"pkg": "my-pkg/1.0.1/ZPGKGOTY"});
+    remote_repo
+        .publish_package(
+            &spec,
+            &vec![(Component::Run, empty_layer_digest())]
+                .into_iter()
+                .collect(),
+        )
+        .await
+        .unwrap();
+
+    // Test a basic bake
+    let mut opt = Opt::try_parse_from(["bake", "--no-runtime", "my-pkg:run"]).unwrap();
+    let result = opt.bake.run().await.unwrap();
+    assert_eq!(result, 0);
+}
+
+#[rstest]
+#[tokio::test]
+async fn test_bake_incompatible_merged_request() {
+    // Test bake with an incompatible set of requests
+    let mut rt = spfs_runtime().await;
+    let remote_repo = spfsrepo().await;
+
+    // Populate the "origin" repo with one package.
+    // The "local" repo is empty.
+    rt.add_remote_repo(
+        "origin",
+        Remote::Address(RemoteAddress {
+            address: remote_repo.address().clone(),
+        }),
+    )
+    .unwrap();
+
+    let recipe = recipe!({"pkg": "my-pkg/1.0.33+r.1"});
+    remote_repo.publish_recipe(&recipe).await.unwrap();
+    let spec = spec!({"pkg": "my-pkg/1.0.33+r.1/ZPGKGOTY"});
+    remote_repo
+        .publish_package(
+            &spec,
+            &vec![(Component::Run, empty_layer_digest())]
+                .into_iter()
+                .collect(),
+        )
+        .await
+        .unwrap();
+
+    // Test bake command with 2 incompatible requests. This should
+    // not panic, it should error out
+    let mut opt = Opt::try_parse_from([
+        "bake",
+        "--no-runtime",
+        "my-pkg:run/==1.0.33+r.1/ZPGKGOTY",
+        "my-pkg:run/=1.0.99",
+    ])
+    .unwrap();
+    let result = opt.bake.run().await;
+    println!("bake run result: {result:?}");
+
+    match result {
+        Err(err) => {
+            println!("Bake errored with: {err}");
+        }
+        Ok(_value) => {
+            panic!("Incompatible requests for same package should cause bake to error");
+        }
+    }
+}

crates/spk-solve/crates/graph/src/graph.rs

@@ -1374,22 +1374,20 @@ impl State {
 
     /// Get a mapping of pkg name -> merged request for the unresolved
     /// PkgRequests in this state
-    pub fn get_unresolved_requests(&self) -> &HashMap<PkgNameBuf, PkgRequest> {
-        self.cached_unresolved_pkg_requests.get_or_init(|| {
+    pub fn get_unresolved_requests(&self) -> Result<&HashMap<PkgNameBuf, PkgRequest>> {
+        self.cached_unresolved_pkg_requests.get_or_try_init(|| {
             let mut unresolved: HashMap<PkgNameBuf, PkgRequest> = HashMap::new();
 
             for req in self.pkg_requests.iter() {
                 if unresolved.contains_key(&req.pkg.name) {
                     continue;
                 }
                 if self.get_current_resolve(&req.pkg.name).is_err() {
-                    unresolved.insert(
-                        req.pkg.name.clone(),
-                        self.get_merged_request(&req.pkg.name).unwrap(),
-                    );
+                    let merged = self.get_merged_request(&req.pkg.name)?;
+                    unresolved.insert(req.pkg.name.clone(), merged);
                 }
             }
-            unresolved
+            Ok(unresolved)
         })
     }
 

crates/spk-solve/src/io.rs

@@ -243,9 +243,9 @@ where
         }
     }
 
-    fn show_unresolved_requests(&self, state: &Arc<State>) {
+    fn show_unresolved_requests(&self, state: &Arc<State>) -> Result<()> {
         let unresolved_requests = state
-            .get_unresolved_requests()
+            .get_unresolved_requests()?
             .iter()
             .map(|(n, r)| {
                 r.format_request(
@@ -264,6 +264,7 @@ where
             unresolved_requests.join("\n  "),
             unresolved_requests.len()
         );
+        Ok(())
     }
 
     fn show_var_requests(&self, state: &Arc<State>) {
@@ -290,11 +291,12 @@ where
         );
     }
 
-    fn show_state(&self, state: &Arc<State>) {
+    fn show_state(&self, state: &Arc<State>) -> Result<()> {
         self.show_resolved_packages(state);
-        self.show_unresolved_requests(state);
+        self.show_unresolved_requests(state)?;
         self.show_var_requests(state);
         self.show_options(state);
+        Ok(())
     }
 
     fn show_full_menu(&self, prompt_prefix: &str) {
@@ -343,10 +345,10 @@ where
                 match selection {
                     '?' => self.show_full_menu(&prompt_prefix),
                     'r' => self.show_resolved_packages(state),
-                    'u' => self.show_unresolved_requests(state),
+                    'u' => self.show_unresolved_requests(state)?,
                     'v' => self.show_var_requests(state),
                     'o' => self.show_options(state),
-                    's' | 'a' => self.show_state(state),
+                    's' | 'a' => self.show_state(state)?,
                     'c' => {
                         self.remove_step_and_stop_setting();
                         break;

crates/spk-solve/src/solver.rs

@@ -555,7 +555,7 @@ impl Solver {
                 let builds_with_impossible_requests = if self.impossible_checks.use_in_build_keys {
                     let impossible_check_start = Instant::now();
                     let start_number = self.request_validator.num_build_specs_read();
-                    let unresolved = node.state.get_unresolved_requests();
+                    let unresolved = node.state.get_unresolved_requests()?;
 
                     let problematic_builds = self
                         .check_builds_for_impossible_requests(unresolved, builds.clone())
@@ -608,7 +608,7 @@ impl Solver {
                                     // are used to check the new requests this
                                     // build would add, if it was used to
                                     // resolve the current request.
-                                    let unresolved = node.state.get_unresolved_requests();
+                                    let unresolved = node.state.get_unresolved_requests()?;
                                     let compat = self
                                         .check_requirements_for_impossible_requests(
                                             &spec, unresolved,
@@ -844,7 +844,7 @@ impl Solver {
 
         let tasks = FuturesUnordered::new();
 
-        let initial_requests = initial_state.get_unresolved_requests();
+        let initial_requests = initial_state.get_unresolved_requests()?;
         for (count, req) in initial_requests.values().enumerate() {
             // Have to make a dummy spec for an "initialrequest"
             // package to interact with the request_validator's

cspell.json

@@ -785,6 +785,7 @@
     "YYYNNNNNNN",
     "YYYNNNYYYY",
     "ZLMZGDCVUOL",
+    "ZPGKGOTY",
     "ZWOIF"
   ],
   "enabled": true