From 1fdb43f61f3c93ae4c4698ff31a62376e22341a3 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Sun, 10 Apr 2022 13:34:41 -0700 Subject: [PATCH 01/12] Fix typo in doc comment Signed-off-by: Ryan Bottriell Co-authored-by: jrray --- src/api/test_spec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api/test_spec.rs b/src/api/test_spec.rs index e955a07cf2..798e890ea1 100644 --- a/src/api/test_spec.rs +++ b/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, From b8253775a793387e6400c13118cf14783e84127b Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Sun, 10 Apr 2022 13:35:15 -0700 Subject: [PATCH 02/12] Fix typo in doc comment Signed-off-by: Ryan Bottriell Co-authored-by: jrray --- src/solve/solver.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/solve/solver.rs b/src/solve/solver.rs index a066d5da18..7f26346438 100644 --- a/src/solve/solver.rs +++ b/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); From a831057b8295360674e2c63b421475150ae9f68f Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 11 Apr 2022 17:43:27 -0700 Subject: [PATCH 03/12] Skip deprecating already deprecated packages Signed-off-by: Ryan Bottriell --- src/cli/cmd_deprecate.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cli/cmd_deprecate.rs b/src/cli/cmd_deprecate.rs index c68ff3bf15..59c32f1ebb 100644 --- a/src/cli/cmd_deprecate.rs +++ b/src/cli/cmd_deprecate.rs @@ -52,6 +52,10 @@ impl Deprecate { let ident = spk::api::parse_ident(name)?; for (repo_name, repo) in repos.iter() { let mut spec = repo.read_spec(&ident)?; + if spec.deprecated { + tracing::warn!(repo=%repo_name, "no change {} (already deprecated)", spk::io::format_ident(&ident)); + continue; + } spec.deprecated = true; repo.force_publish_spec(spec)?; tracing::info!(repo=%repo_name, "deprecated {}", spk::io::format_ident(&ident)); From ca4467aa3380e7c9f34cebd258ba7b6dfd5c4ca8 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 11 Apr 2022 17:44:42 -0700 Subject: [PATCH 04/12] Output solve to stdout for explain command Signed-off-by: Ryan Bottriell --- src/cli/cmd_explain.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cli/cmd_explain.rs b/src/cli/cmd_explain.rs index 1a4690db27..c5ff49ced3 100644 --- a/src/cli/cmd_explain.rs +++ b/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) } } From 028670d737b44694a7ce1a1e467a6a1cd7ae5227 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 11 Apr 2022 17:48:14 -0700 Subject: [PATCH 05/12] Warn instead of masking the real error on failed export Signed-off-by: Ryan Bottriell --- src/cli/cmd_export.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cli/cmd_export.rs b/src/cli/cmd_export.rs index 379c827802..5b608d7ef9 100644 --- a/src/cli/cmd_export.rs +++ b/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); From cfdd507a60966396cee9a2dd954060352265e43d Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 11 Apr 2022 17:52:52 -0700 Subject: [PATCH 06/12] Reduce boilerplate with anyhow::bail Signed-off-by: Ryan Bottriell --- src/cli/cmd_render.rs | 4 ++-- src/cli/cmd_view.rs | 4 ++-- src/cli/flags.rs | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/cli/cmd_render.rs b/src/cli/cmd_render.rs index 34d94d2341..0fe6a97f4a 100644 --- a/src/cli/cmd_render.rs +++ b/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()?; diff --git a/src/cli/cmd_view.rs b/src/cli/cmd_view.rs index ce8bcdc86c..c188b7f41f 100644 --- a/src/cli/cmd_view.rs +++ b/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(); diff --git a/src/cli/flags.rs b/src/cli/flags.rs index aad6adf2ac..e7494436ab 100644 --- a/src/cli/flags.rs +++ b/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 - )) + ) } }; From fd63a847b1356fb54c73b313abf972a560879843 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 11 Apr 2022 17:53:42 -0700 Subject: [PATCH 07/12] Remove unneeded python file Signed-off-by: Ryan Bottriell --- src/cli/run.rs | 98 -------------------------------------------------- 1 file changed, 98 deletions(-) delete mode 100644 src/cli/run.rs diff --git a/src/cli/run.rs b/src/cli/run.rs deleted file mode 100644 index b3def91b26..0000000000 --- a/src/cli/run.rs +++ /dev/null @@ -1,98 +0,0 @@ -# Copyright (c) 2021 Sony Pictures Imageworks, et al. -# SPDX-License-Identifier: Apache-2.0 -# https://github.com/imageworks/spk - -from typing import Sequence -import sys -import traceback - -import sentry_sdk -from colorama import Fore - -import spk - -from ._args import ( - parse_args, - configure_logging, - configure_sentry, - configure_spops, - spops, -) - - -def main() -> None: - - code = run(sys.argv[1:]) - sentry_sdk.flush() - sys.exit(code) - - -def run(argv: Sequence[str]) -> int: - - try: - configure_sentry() - except Exception as e: - print(f"failed to initialize sentry: {e}", file=sys.stderr) - - try: - args = parse_args(argv) - except SystemExit as e: - return e.code - - configure_logging(args) - configure_spops() - - if spops is not None: - spops.count("spk.run_count", command=args.command) - - with sentry_sdk.configure_scope() as scope: - scope.set_extra("command", args.command) - scope.set_extra("argv", sys.argv) - - try: - sys.stdout.reconfigure(encoding="utf-8") # type: ignore - except AttributeError: - # stdio might not be a real terminal, but that's okay - pass - - try: - - if spops is not None: - with spops.timer("spk.run_time", command=args.command): - args.func(args) - else: - args.func(args) - - except KeyboardInterrupt: - pass - - except SystemExit as e: - return e.code - - except Exception as e: - _capture_if_relevant(e) - if spops is not None: - spops.count("spk.error_count", command=args.command) - print(f"{spk.io.format_error(e)}", file=sys.stderr) - if args.verbose > 2: - print(f"{Fore.RED}{traceback.format_exc()}{Fore.RESET}", file=sys.stderr) - return 1 - - return 0 - - -def _capture_if_relevant(err: Exception) -> None: - - if isinstance(err, spk.storage.PackageNotFoundError): - return - if isinstance(err, spk.storage.VersionExistsError): - return - if isinstance(err, spk.NoEnvironmentError): - return - if isinstance(err, spk.build.BuildError): - return - if isinstance(err, spk.solve.SolverError): - return - if isinstance(err, RuntimeError): # likely from spkrs - return - sentry_sdk.capture_exception(err) From e751b6a4e9558f096fda44f5173d1ae46c7bfd8e Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 11 Apr 2022 17:55:15 -0700 Subject: [PATCH 08/12] Use explicit arc::clone for clarity Signed-off-by: Ryan Bottriell --- src/solve/graph.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/solve/graph.rs b/src/solve/graph.rs index 0679fce237..caa2c15da1 100644 --- a/src/solve/graph.rs +++ b/src/solve/graph.rs @@ -1203,11 +1203,11 @@ impl StepBack { pub fn new(cause: impl Into, to: &Arc) -> Self { StepBack { cause: cause.into(), - destination: to.clone(), + destination: Arc::clone(to), } } fn apply(&self, _base: &State) -> Arc { - self.destination.clone() + Arc::clone(&self.destination) } } From 43ed079bb68bf2baebec6120cea31f86a817271c Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Mon, 11 Apr 2022 17:56:56 -0700 Subject: [PATCH 09/12] Fix typo in error message Signed-off-by: Ryan Bottriell Co-authored-by: jrray --- src/solve/solver.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/solve/solver.rs b/src/solve/solver.rs index aaae15764d..41f67bc2cf 100644 --- a/src/solve/solver.rs +++ b/src/solve/solver.rs @@ -445,7 +445,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() From dbde7749520e1190eeafa540eae1caf98924f117 Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Tue, 12 Apr 2022 08:11:14 -0700 Subject: [PATCH 10/12] Try to fail less often in the middle of a deprecate operation Signed-off-by: Ryan Bottriell --- src/cli/cmd_deprecate.rs | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/cli/cmd_deprecate.rs b/src/cli/cmd_deprecate.rs index 59c32f1ebb..8428e5599d 100644 --- a/src/cli/cmd_deprecate.rs +++ b/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 { - 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,16 +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)?; - if spec.deprecated { - tracing::warn!(repo=%repo_name, "no change {} (already deprecated)", spk::io::format_ident(&ident)); - continue; - } - 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) } } From b5ac8ca23c09e1e44ddf1ae81c602435bbd8573d Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Tue, 12 Apr 2022 08:19:05 -0700 Subject: [PATCH 11/12] Describe why we allow large enum variant Signed-off-by: Ryan Bottriell --- src/cli/flags.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cli/flags.rs b/src/cli/flags.rs index e7494436ab..4ce3b9b9e6 100644 --- a/src/cli/flags.rs +++ b/src/cli/flags.rs @@ -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 From 251f17c0e6dc5212a9ee4897a418d0f3dcda80ca Mon Sep 17 00:00:00 2001 From: Ryan Bottriell Date: Tue, 12 Apr 2022 17:47:33 -0700 Subject: [PATCH 12/12] Update src/api/request.rs Signed-off-by: Ryan Bottriell Co-authored-by: jrray --- src/api/ident.rs | 1 - src/api/request.rs | 1 + src/api/spec.rs | 1 - 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/api/ident.rs b/src/api/ident.rs index fcaa63a1d1..de1c18c3a9 100644 --- a/src/api/ident.rs +++ b/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 { diff --git a/src/api/request.rs b/src/api/request.rs index 90b8b35e36..d44d4f47aa 100644 --- a/src/api/request.rs +++ b/src/api/request.rs @@ -591,6 +591,7 @@ impl PkgRequest { } } + // TODO: change parameter to `pkg: Ident` pub fn from_ident(pkg: &Ident) -> Self { Self::from(pkg.clone()) } diff --git a/src/api/spec.rs b/src/api/spec.rs index ef12a0eb98..0f178f5e2e 100644 --- a/src/api/spec.rs +++ b/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)