Merge remote-tracking branch 'origin/master' into 283-package-opt-ove…

…rride

src/api/ident.rs

@@ -60,7 +60,6 @@ impl std::fmt::Display for Ident {
 }
 
 impl Ident {
-
     /// Return true if this identifier is for a source package.
     pub fn is_source(&self) -> bool {
         match &self.build {

src/api/request.rs

@@ -597,6 +597,7 @@ impl PkgRequest {
         }
     }
 
+    // TODO: change parameter to `pkg: Ident`
     pub fn from_ident(pkg: &Ident) -> Self {
         Self::from(pkg.clone())
     }

src/api/spec.rs

@@ -64,7 +64,6 @@ pub struct Spec {
 }
 
 impl Spec {
-
     /// Return the full set of resolved build options using the given ones.
     pub fn resolve_all_options(&self, given: &OptionMap) -> OptionMap {
         self.build.resolve_all_options(Some(self.pkg.name()), given)

src/api/test_spec.rs

@@ -24,7 +24,7 @@ impl std::fmt::Display for TestStage {
         f.write_str(
             // Note that we need `TestStage::to_string` to produce
             // these exact values in order to match correctly with
-            // the spelling on the in the package yaml.
+            // the spelling in the package yaml.
             match self {
                 TestStage::Build => BUILD_NAME,
                 TestStage::Install => INSTALL_NAME,

src/cli/cmd_deprecate.rs

@@ -1,6 +1,7 @@
 // Copyright (c) 2022 Sony Pictures Imageworks, et al.
 // SPDX-License-Identifier: Apache-2.0
 // https://github.com/imageworks/spk
+use std::sync::Arc;
 
 use anyhow::Result;
 use clap::Args;
@@ -33,7 +34,12 @@ pub struct Deprecate {
 /// Runs make-source and then make-binary
 impl Deprecate {
     pub fn run(&self) -> Result<i32> {
-        let repos = self.repos.get_repos(None)?;
+        let repos: Vec<_> = self
+            .repos
+            .get_repos(None)?
+            .into_iter()
+            .map(|(name, repo)| (name, Arc::new(repo)))
+            .collect();
         if repos.is_empty() {
             eprintln!(
                 "{}",
@@ -43,6 +49,11 @@ impl Deprecate {
             return Ok(1);
         }
 
+        // find and load everything that we want to deprecate first
+        // to avoid doing some deprecations and then failing in the
+        // middle of the operation. This is still not properly atomic
+        // but avoids the simple failure cases
+        let mut to_deprecate = Vec::new();
         for name in self.packages.iter() {
             if !name.contains('/') {
                 tracing::error!("Must provide a version number: {name}/???");
@@ -51,12 +62,21 @@ impl Deprecate {
             }
             let ident = spk::api::parse_ident(name)?;
             for (repo_name, repo) in repos.iter() {
-                let mut spec = repo.read_spec(&ident)?;
-                spec.deprecated = true;
-                repo.force_publish_spec(spec)?;
-                tracing::info!(repo=%repo_name, "deprecated {}", spk::io::format_ident(&ident));
+                let spec = repo.read_spec(&ident)?;
+                to_deprecate.push((spec, repo_name.clone(), Arc::clone(repo)));
             }
         }
+
+        for (mut spec, repo_name, repo) in to_deprecate.into_iter() {
+            let fmt = spk::io::format_ident(&spec.pkg);
+            if spec.deprecated {
+                tracing::warn!(repo=%repo_name, "no change  {} (already deprecated)", fmt);
+                continue;
+            }
+            spec.deprecated = true;
+            repo.force_publish_spec(spec)?;
+            tracing::info!(repo=%repo_name, "deprecated {}", fmt);
+        }
         Ok(0)
     }
 }

src/cli/cmd_explain.rs

@@ -41,7 +41,7 @@ impl Explain {
 
         let solution = spk::io::run_and_print_resolve(&solver, self.verbose + 1)?;
 
-        eprintln!("{}", spk::io::format_solution(&solution, self.verbose + 1));
+        println!("{}", spk::io::format_solution(&solution, self.verbose + 1));
         Ok(0)
     }
 }

src/cli/cmd_export.rs

@@ -47,7 +47,9 @@ impl Export {
             tracing::warn!("version number when exporting from the local repository");
         }
         if res.is_err() {
-            std::fs::remove_file(&filename)?;
+            if let Err(err) = std::fs::remove_file(&filename) {
+                tracing::warn!(?err, path=?filename, "failed to clean up incomplete archive");
+            }
         }
         res?;
         println!("{}: {:?}", "Created".green(), filename);

src/cli/cmd_render.rs

@@ -3,7 +3,7 @@
 // https://github.com/imageworks/spk
 use std::path::PathBuf;
 
-use anyhow::{anyhow, Context, Result};
+use anyhow::{bail, Context, Result};
 use clap::Args;
 
 use super::flags;
@@ -50,7 +50,7 @@ impl Render {
             .next()
             .is_some()
         {
-            return Err(anyhow!("Output directory does not appear to be empty"));
+            bail!("Output directory does not appear to be empty");
         }
 
         let path = self.target.canonicalize()?;

src/cli/cmd_view.rs

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 // https://github.com/imageworks/spk
 
-use anyhow::{anyhow, Context, Result};
+use anyhow::{bail, Context, Result};
 use clap::Args;
 use colored::Colorize;
 
@@ -36,7 +36,7 @@ impl View {
         solver.add_request(request.clone());
         let request = match request {
             spk::api::Request::Pkg(pkg) => pkg,
-            _ => return Err(anyhow!("Not a package request: {request:?}")),
+            _ => bail!("Not a package request: {request:?}"),
         };
 
         let mut runtime = solver.run();

src/cli/flags.rs

@@ -4,7 +4,7 @@
 
 use std::{collections::HashMap, str::FromStr};
 
-use anyhow::{anyhow, Context, Result};
+use anyhow::{anyhow, bail, Context, Result};
 use clap::Args;
 
 #[cfg(test)]
@@ -172,9 +172,9 @@ impl Requests {
                         continue;
                     }
                     _ => {
-                        return Err(anyhow!(
+                        bail!(
                         "Unsupported stage '{stage}', can only be empty or 'source' in this context"
-                    ));
+                    );
                     }
                 }
             }
@@ -259,10 +259,10 @@ impl Requests {
                 }
                 serde_yaml::Value::Mapping(m) => m,
                 _ => {
-                    return Err(anyhow!(
+                    bail!(
                         "Invalid request, expected either a string or a mapping, got: {:?}",
                         value
-                    ))
+                    )
                 }
             };
 
@@ -303,6 +303,9 @@ pub fn parse_stage_specifier(
 }
 
 /// The result of the [`find_package_spec`] function.
+// We are okay with the large variant here because it's specifically
+// used as the positive result of the function, with the others simply
+// denoting unique error cases.
 #[allow(clippy::large_enum_variant)]
 pub enum FindPackageSpecResult {
     /// A non-ambiguous package spec file was found

src/solve/graph.rs

@@ -1025,11 +1025,11 @@ impl StepBack {
     pub fn new(cause: impl Into<String>, to: &Arc<State>) -> Self {
         StepBack {
             cause: cause.into(),
-            destination: to.clone(),
+            destination: Arc::clone(to),
         }
     }
 
     pub fn apply(&self, _base: &State) -> Arc<State> {
-        self.destination.clone()
+        Arc::clone(&self.destination)
     }
 }

src/solve/solver.rs

@@ -248,7 +248,7 @@ impl Solver {
         Ok(api::Compatibility::Compatible)
     }
 
-    /// Put this solver back into it's default state
+    /// Put this solver back into its default state
     pub fn reset(&mut self) {
         self.repos.truncate(0);
         self.initial_state_builders.truncate(0);
@@ -385,7 +385,7 @@ impl SolverRuntime {
         let current_node = self
             .current_node
             .as_ref()
-            .ok_or_else(|| Error::String("Solver runtime as not been consumed".into()))?;
+            .ok_or_else(|| Error::String("Solver runtime has not been consumed".into()))?;
         let current_node_lock = current_node.read().unwrap();
 
         let is_dead = current_node_lock.state.id()