From 6672f1fb3a42ef4c67a4b6f49e96691e63273aa1 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 20 Dec 2024 15:10:17 -0800 Subject: [PATCH] [2/n] [omicron-package] better error messages around target (#7287) * If no target is specified, don't print a confusing `the name '{name}' is reserved` message. * For `target delete`, if removing the file failed, print the corresponding error message. Depends on #7285. --- package/src/bin/omicron-package.rs | 49 +++++++++++++++++++----------- package/src/config.rs | 25 +++++++-------- package/src/target.rs | 8 +++++ 3 files changed, 51 insertions(+), 31 deletions(-) diff --git a/package/src/bin/omicron-package.rs b/package/src/bin/omicron-package.rs index f4bda47e2c..2cb0512169 100644 --- a/package/src/bin/omicron-package.rs +++ b/package/src/bin/omicron-package.rs @@ -12,7 +12,7 @@ use illumos_utils::{zfs, zone}; use indicatif::{MultiProgress, ProgressBar, ProgressStyle}; use omicron_package::cargo_plan::build_cargo_plan; use omicron_package::config::{Config, ConfigArgs}; -use omicron_package::target::KnownTarget; +use omicron_package::target::{target_command_help, KnownTarget}; use omicron_package::{parse, BuildCommand, DeployCommand, TargetCommand}; use omicron_zone_package::config::Config as PackageConfig; use omicron_zone_package::package::{Package, PackageOutput, PackageSource}; @@ -157,7 +157,7 @@ async fn do_list_outputs( async fn do_target( artifact_dir: &Utf8Path, - name: &str, + name: Option<&str>, subcommand: &TargetCommand, ) -> Result<()> { let target_dir = artifact_dir.join("target"); @@ -180,7 +180,7 @@ async fn do_target( clickhouse_topology.clone(), )?; - let path = get_single_target(&target_dir, name).await?; + let (name, path) = get_required_named_target(&target_dir, name)?; tokio::fs::write(&path, Target::from(target).to_string()) .await .with_context(|| { @@ -215,31 +215,46 @@ async fn do_target( } } TargetCommand::Set => { - let _ = get_single_target(&target_dir, name).await?; + let (name, _) = get_required_named_target(&target_dir, name)?; replace_active_link(&name, &target_dir).await?; println!("Set build target '{name}' as active"); } TargetCommand::Delete => { - let path = get_single_target(&target_dir, name).await?; - tokio::fs::remove_file(&path).await?; + let (name, path) = get_required_named_target(&target_dir, name)?; + tokio::fs::remove_file(&path).await.with_context(|| { + format!("failed to remove target file {}", path) + })?; println!("Removed build target '{name}'"); } }; Ok(()) } -async fn get_single_target( +/// Get the path to a named target as required by the `target` subcommand. +/// +/// This function bans `active` as a target name, as it is reserved for the +/// active target. +fn get_required_named_target( target_dir: impl AsRef, - name: &str, -) -> Result { - if name == Config::ACTIVE { - bail!( - "The name '{name}' is reserved, please try another (e.g. 'default')\n\ - Usage: '{} -t target ...'", - env::current_exe().unwrap().display(), - ); + name: Option<&str>, +) -> Result<(&str, Utf8PathBuf)> { + match name { + Some(name) if name == Config::ACTIVE => { + bail!( + "the name '{name}' is reserved, please try another (e.g. 'default')\n\ + Usage: {} ...", + target_command_help(""), + ); + } + Some(name) => Ok((name, target_dir.as_ref().join(name))), + None => { + bail!( + "a target name is required for this operation (e.g. 'default')\n\ + Usage: {} ...", + target_command_help(""), + ); + } } - Ok(target_dir.as_ref().join(name)) } async fn replace_active_link( @@ -887,7 +902,7 @@ async fn main() -> Result<()> { SubCommand::Build(BuildCommand::Target { subcommand }) => { do_target( &args.artifact_dir, - &args.config_args.target, + args.config_args.target.as_deref(), &subcommand, ) .await?; diff --git a/package/src/config.rs b/package/src/config.rs index f80bd36057..800af7a0de 100644 --- a/package/src/config.rs +++ b/package/src/config.rs @@ -11,21 +11,15 @@ use omicron_zone_package::{ target::Target, }; use slog::{debug, Logger}; -use std::{ - collections::BTreeMap, env, io::Write, str::FromStr, time::Duration, -}; +use std::{collections::BTreeMap, io::Write, str::FromStr, time::Duration}; -use crate::target::KnownTarget; +use crate::target::{target_command_help, KnownTarget}; #[derive(Debug, Args)] pub struct ConfigArgs { /// The name of the build target to use for this command - #[clap( - short, - long, - default_value_t = Config::ACTIVE.to_string(), - )] - pub target: String, + #[clap(short, long)] + pub target: Option, /// Skip confirmation prompt for destructive operations #[clap(short, long, action, default_value_t = false)] @@ -78,14 +72,17 @@ impl Config { args: &ConfigArgs, artifact_dir: &Utf8Path, ) -> Result { + // Within this path, the target is expected to be set. + let target = args.target.as_deref().unwrap_or(Self::ACTIVE); + let target_help_str = || -> String { format!( - "Try calling: '{} -t default target create' to create a new build target", - env::current_exe().unwrap().display() + "Try calling: '{} target create' to create a new build target", + target_command_help("default"), ) }; - let target_path = artifact_dir.join("target").join(&args.target); + let target_path = artifact_dir.join("target").join(target); let raw_target = std::fs::read_to_string(&target_path).inspect_err(|_| { eprintln!( @@ -103,7 +100,7 @@ impl Config { ); })? .into(); - debug!(log, "target[{}]: {:?}", args.target, target); + debug!(log, "target[{}]: {:?}", target, target); Ok(Config { log: log.clone(), diff --git a/package/src/target.rs b/package/src/target.rs index 6a6cbd32d8..d56f7e87c5 100644 --- a/package/src/target.rs +++ b/package/src/target.rs @@ -200,3 +200,11 @@ impl std::str::FromStr for KnownTarget { ) } } + +/// Generate a command to build a target, for use in usage strings. +pub fn target_command_help(target_name: &str) -> String { + format!( + "{} -t {target_name} target", + std::env::current_exe().unwrap().display(), + ) +}