From 9eeac56277bb6bff885807428a3ef2e53c284aa8 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Wed, 20 Nov 2019 19:46:26 +0900 Subject: [PATCH] Fix handling of ctrl-c --- src/main.rs | 100 ++++++++++--------------------------------------- src/restore.rs | 90 ++++++++++++++++++++++++++++++++++++++++++++ tests/test.rs | 15 ++++++++ 3 files changed, 124 insertions(+), 81 deletions(-) create mode 100644 src/restore.rs diff --git a/src/main.rs b/src/main.rs index 0b0a2c41..c2d8abf4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,17 +9,9 @@ mod cli; mod manifest; mod metadata; mod process; +mod restore; -use std::{ - env, - ffi::OsString, - fs, - path::{Path, PathBuf}, - sync::{ - atomic::{AtomicBool, Ordering::SeqCst}, - Arc, Mutex, - }, -}; +use std::{env, ffi::OsString, fs, path::Path}; use anyhow::{bail, Context, Error}; @@ -28,6 +20,7 @@ use crate::{ manifest::{find_root_manifest_for_wd, Manifest}, metadata::{Metadata, Package}, process::ProcessBuilder, + restore::Restore, }; type Result = std::result::Result; @@ -64,6 +57,8 @@ fn exec_on_workspace(args: &Args, current_manifest: &Manifest, metadata: &Metada line.arg(color.as_str()); } + let restore = Restore::new(args); + if args.workspace { for spec in &args.exclude { if !metadata.packages.iter().any(|package| package.name == *spec) { @@ -79,7 +74,7 @@ fn exec_on_workspace(args: &Args, current_manifest: &Manifest, metadata: &Metada for package in metadata.packages.iter().filter(|package| !args.exclude.contains(&package.name)) { - exec_on_package(args, package, &line)?; + exec_on_package(args, package, &line, &restore)?; } } else if !args.package.is_empty() { for spec in &args.package { @@ -91,23 +86,28 @@ fn exec_on_workspace(args: &Args, current_manifest: &Manifest, metadata: &Metada for package in metadata.packages.iter().filter(|package| args.package.contains(&package.name)) { - exec_on_package(args, package, &line)?; + exec_on_package(args, package, &line, &restore)?; } } else if current_manifest.is_virtual() { for package in &metadata.packages { - exec_on_package(args, package, &line)?; + exec_on_package(args, package, &line, &restore)?; } } else if !current_manifest.is_virtual() { let current_package = current_manifest.package_name(); let package = metadata.packages.iter().find(|package| package.name == *current_package).unwrap(); - exec_on_package(args, package, &line)?; + exec_on_package(args, package, &line, &restore)?; } Ok(()) } -fn exec_on_package(args: &Args, package: &Package, line: &ProcessBuilder) -> Result<()> { +fn exec_on_package( + args: &Args, + package: &Package, + line: &ProcessBuilder, + restore: &Restore, +) -> Result<()> { let manifest = Manifest::new(&package.manifest_path)?; if args.ignore_private && manifest.is_private() { @@ -118,7 +118,7 @@ fn exec_on_package(args: &Args, package: &Package, line: &ProcessBuilder) -> Res line.arg("--manifest-path"); line.arg(&package.manifest_path); - no_dev_deps(args, package, &manifest, &line)?; + no_dev_deps(args, package, &manifest, &line, restore)?; } Ok(()) @@ -129,71 +129,11 @@ fn no_dev_deps( package: &Package, manifest: &Manifest, line: &ProcessBuilder, + restore: &Restore, ) -> Result<()> { - struct Restore { - manifest: String, - manifest_path: PathBuf, - color: Option, - restore: AtomicBool, - done: AtomicBool, - res: Arc>>>, - } - - impl Restore { - #[allow(clippy::type_complexity)] - fn new( - args: &Args, - manifest: &Manifest, - ) -> (Bomb, Arc, Arc>>>) { - let res = Arc::new(Mutex::new(Some(Ok(())))); - - let bomb = Arc::new(Self { - manifest: manifest.raw.to_string(), - manifest_path: manifest.path.to_path_buf(), - color: args.color, - // if `--remove-dev-deps` flag is off, restore manifest file. - restore: AtomicBool::new(args.no_dev_deps && !args.remove_dev_deps), - done: AtomicBool::new(false), - res: res.clone(), - }); - - (Bomb(bomb.clone()), bomb, res) - } - - fn restore_dev_deps(&self) { - if self.restore.load(SeqCst) { - let res = fs::write(&self.manifest_path, &self.manifest).with_context(|| { - format!("failed to restore manifest file: {}", self.manifest_path.display()) - }); - - if self.done.load(SeqCst) { - *self.res.lock().unwrap() = Some(res); - } else if let Err(e) = res { - error!(self.color, "{:#}", e); - } - - self.restore.store(false, SeqCst); - } - } - } - - struct Bomb(Arc); - - impl Drop for Bomb { - fn drop(&mut self) { - self.0.restore_dev_deps(); - } - } - if args.no_dev_deps || args.remove_dev_deps { let new = manifest.remove_dev_deps()?; - let (bomb, restore, res) = Restore::new(args, manifest); - - ctrlc::set_handler(move || { - restore.restore_dev_deps(); - std::process::exit(0) - }) - .unwrap(); + let mut handle = restore.set_manifest(&manifest); fs::write(&package.manifest_path, new).with_context(|| { format!("failed to update manifest file: {}", package.manifest_path.display()) @@ -203,9 +143,7 @@ fn no_dev_deps( each_feature(args, package, line)?; } - bomb.0.done.store(true, SeqCst); - drop(bomb); - res.lock().unwrap().take().unwrap()?; + handle.done()?; } else if args.subcommand.is_some() { each_feature(args, package, line)?; } diff --git a/src/restore.rs b/src/restore.rs new file mode 100644 index 00000000..c7c89808 --- /dev/null +++ b/src/restore.rs @@ -0,0 +1,90 @@ +use std::{ + fs, mem, + path::PathBuf, + sync::{Arc, Mutex}, +}; + +use anyhow::Context; + +use crate::{ + cli::{Args, Coloring}, + manifest::Manifest, + Result, +}; + +pub(crate) struct Restore { + color: Option, + restore_flag: bool, + + current: Mutex>, +} + +struct Current { + manifest: String, + manifest_path: PathBuf, + // A flag that indicates a restore is needed. + restore_flag: bool, +} + +impl Restore { + pub(crate) fn new(args: &Args) -> Arc { + let this = Arc::new(Self { + color: args.color, + // if `--remove-dev-deps` flag is off, restore manifest file. + restore_flag: args.no_dev_deps && !args.remove_dev_deps, + current: Mutex::new(None), + }); + + let x = this.clone(); + ctrlc::set_handler(move || { + if let Err(e) = x.restore_dev_deps() { + error!(x.color, "{:#}", e); + std::process::exit(1) + } + std::process::exit(0) + }) + .unwrap(); + + this + } + + pub(crate) fn set_manifest(&self, manifest: &Manifest) -> Handle<'_> { + *self.current.lock().unwrap() = Some(Current { + manifest: manifest.raw.to_string(), + manifest_path: manifest.path.to_path_buf(), + restore_flag: self.restore_flag, + }); + + Handle(Some(self)) + } + + fn restore_dev_deps(&self) -> Result<()> { + let mut current = self.current.lock().unwrap(); + if let Some(current) = &mut *current { + if mem::replace(&mut current.restore_flag, false) { + fs::write(¤t.manifest_path, ¤t.manifest).with_context(|| { + format!("failed to restore manifest file: {}", current.manifest_path.display()) + })?; + } + } + Ok(()) + } +} + +pub(crate) struct Handle<'a>(Option<&'a Restore>); + +impl Handle<'_> { + pub(crate) fn done(&mut self) -> Result<()> { + self.0.take().unwrap().restore_dev_deps() + } +} + +impl Drop for Handle<'_> { + fn drop(&mut self) { + if let Some(this) = &self.0 { + if let Err(e) = this.restore_dev_deps() { + error!(this.color, "{:#}", e); + } + } + } +} diff --git a/tests/test.rs b/tests/test.rs index fd10aaca..143066d7 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -393,6 +393,21 @@ fn test_no_dev_deps() { )); } +#[test] +fn test_no_dev_deps_all() { + let output = cargo_hack() + .args(&["hack", "check", "--no-dev-deps", "--all"]) + .current_dir(test_dir("tests/fixtures/real")) + .output() + .unwrap(); + + output + .assert_success() + .assert_stderr_contains(&format!( + "`--no-dev-deps` flag removes dev-dependencies from real `Cargo.toml` while cargo-hack is running and restores it when finished", + )); +} + #[test] fn test_no_dev_deps_with_devs() { for flag in &[