From cd414d4c2ef03fbced9cbccacc7d0a972210310d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Kongstad?= Date: Thu, 27 Jul 2023 14:25:33 +0200 Subject: [PATCH] aconfig: improve error messages Propagate anyhow errors to main.rs and commands.rs to improve the error message. As an example. instead of just "bad flag declaration: exactly one bug required", aconfig will now print the following. ---- 8< ---- Error: failed to create cache Caused by: 0: failed to parse build/make/tools/aconfig/tests/test.aconfig 1: bad flag declaration: missing description ---- >8 ---- Error messages can be improved further by including additional information in the protos.rs error cases. This will be handled in a follow-up CL. Bug: 290300657 Test: manual: introduce error in an aconfig file and run `m all_aconfig_declarations` Test: atest aconfig.test Change-Id: Id278f4877e5794b95913ae8ba0ca3ee211293f38 --- tools/aconfig/src/commands.rs | 29 ++++++++++++++++------- tools/aconfig/src/main.rs | 44 +++++++++++++++++++++-------------- 2 files changed, 47 insertions(+), 26 deletions(-) diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index 4f0a706dc0..bd09e24ef6 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -35,8 +35,15 @@ pub struct Input { impl Input { fn try_parse_flags(&mut self) -> Result { let mut buffer = Vec::new(); - self.reader.read_to_end(&mut buffer)?; + self.reader + .read_to_end(&mut buffer) + .with_context(|| format!("failed to read {}", self.source))?; crate::protos::parsed_flags::try_from_binary_proto(&buffer) + .with_context(|| self.error_context()) + } + + fn error_context(&self) -> String { + format!("failed to parse {}", self.source) } } @@ -53,20 +60,23 @@ pub fn parse_flags(package: &str, declarations: Vec, values: Vec) for mut input in declarations { let mut contents = String::new(); - input.reader.read_to_string(&mut contents)?; + input + .reader + .read_to_string(&mut contents) + .with_context(|| format!("failed to read {}", input.source))?; let flag_declarations = crate::protos::flag_declarations::try_from_text_proto(&contents) - .with_context(|| format!("Failed to parse {}", input.source))?; + .with_context(|| input.error_context())?; ensure!( package == flag_declarations.package(), - "Failed to parse {}: expected package {}, got {}", + "failed to parse {}: expected package {}, got {}", input.source, package, flag_declarations.package() ); for mut flag_declaration in flag_declarations.flag.into_iter() { crate::protos::flag_declaration::verify_fields(&flag_declaration) - .with_context(|| format!("Failed to parse {}", input.source))?; + .with_context(|| input.error_context())?; // create ParsedFlag using FlagDeclaration and default values let mut parsed_flag = ProtoParsedFlag::new(); @@ -101,12 +111,15 @@ pub fn parse_flags(package: &str, declarations: Vec, values: Vec) for mut input in values { let mut contents = String::new(); - input.reader.read_to_string(&mut contents)?; + input + .reader + .read_to_string(&mut contents) + .with_context(|| format!("failed to read {}", input.source))?; let flag_values = crate::protos::flag_values::try_from_text_proto(&contents) - .with_context(|| format!("Failed to parse {}", input.source))?; + .with_context(|| input.error_context())?; for flag_value in flag_values.flag_value.into_iter() { crate::protos::flag_value::verify_fields(&flag_value) - .with_context(|| format!("Failed to parse {}", input.source))?; + .with_context(|| input.error_context())?; let Some(parsed_flag) = parsed_flags .parsed_flag diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index 151cbe8f2a..920b7610da 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -16,7 +16,7 @@ //! `aconfig` is a build time tool to manage build time configurations, such as feature flags. -use anyhow::{anyhow, bail, ensure, Result}; +use anyhow::{anyhow, bail, Context, Result}; use clap::{builder::ArgAction, builder::EnumValueParser, Arg, ArgMatches, Command}; use core::any::Any; use std::fs; @@ -129,26 +129,27 @@ fn open_single_file(matches: &ArgMatches, arg_name: &str) -> Result { } fn write_output_file_realtive_to_dir(root: &Path, output_file: &OutputFile) -> Result<()> { - ensure!( - root.is_dir(), - "output directory {} does not exist or is not a directory", - root.display() - ); let path = root.join(output_file.path.clone()); let parent = path .parent() .ok_or(anyhow!("unable to locate parent of output file {}", path.display()))?; - fs::create_dir_all(parent)?; - let mut file = fs::File::create(path)?; - file.write_all(&output_file.contents)?; + fs::create_dir_all(parent) + .with_context(|| format!("failed to create directory {}", parent.display()))?; + let mut file = fs::File::create(path.clone()) + .with_context(|| format!("failed to open {}", path.display()))?; + file.write_all(&output_file.contents) + .with_context(|| format!("failed to write to {}", path.display()))?; Ok(()) } fn write_output_to_file_or_stdout(path: &str, data: &[u8]) -> Result<()> { if path == "-" { - io::stdout().write_all(data)?; + io::stdout().write_all(data).context("failed to write to stdout")?; } else { - fs::File::create(path)?.write_all(data)?; + fs::File::create(path) + .with_context(|| format!("failed to open {}", path))? + .write_all(data) + .with_context(|| format!("failed to write to {}", path))?; } Ok(()) } @@ -160,14 +161,16 @@ fn main() -> Result<()> { let package = get_required_arg::(sub_matches, "package")?; let declarations = open_zero_or_more_files(sub_matches, "declarations")?; let values = open_zero_or_more_files(sub_matches, "values")?; - let output = commands::parse_flags(package, declarations, values)?; + let output = commands::parse_flags(package, declarations, values) + .context("failed to create cache")?; let path = get_required_arg::(sub_matches, "cache")?; write_output_to_file_or_stdout(path, &output)?; } Some(("create-java-lib", sub_matches)) => { let cache = open_single_file(sub_matches, "cache")?; let mode = get_required_arg::(sub_matches, "mode")?; - let generated_files = commands::create_java_lib(cache, *mode)?; + let generated_files = + commands::create_java_lib(cache, *mode).context("failed to create java lib")?; let dir = PathBuf::from(get_required_arg::(sub_matches, "out")?); generated_files .iter() @@ -176,7 +179,8 @@ fn main() -> Result<()> { Some(("create-cpp-lib", sub_matches)) => { let cache = open_single_file(sub_matches, "cache")?; let mode = get_required_arg::(sub_matches, "mode")?; - let generated_files = commands::create_cpp_lib(cache, *mode)?; + let generated_files = + commands::create_cpp_lib(cache, *mode).context("failed to create cpp lib")?; let dir = PathBuf::from(get_required_arg::(sub_matches, "out")?); generated_files .iter() @@ -185,25 +189,29 @@ fn main() -> Result<()> { Some(("create-rust-lib", sub_matches)) => { let cache = open_single_file(sub_matches, "cache")?; let mode = get_required_arg::(sub_matches, "mode")?; - let generated_file = commands::create_rust_lib(cache, *mode)?; + let generated_file = + commands::create_rust_lib(cache, *mode).context("failed to create rust lib")?; let dir = PathBuf::from(get_required_arg::(sub_matches, "out")?); write_output_file_realtive_to_dir(&dir, &generated_file)?; } Some(("create-device-config-defaults", sub_matches)) => { let cache = open_single_file(sub_matches, "cache")?; - let output = commands::create_device_config_defaults(cache)?; + let output = commands::create_device_config_defaults(cache) + .context("failed to create device config defaults")?; let path = get_required_arg::(sub_matches, "out")?; write_output_to_file_or_stdout(path, &output)?; } Some(("create-device-config-sysprops", sub_matches)) => { let cache = open_single_file(sub_matches, "cache")?; - let output = commands::create_device_config_sysprops(cache)?; + let output = commands::create_device_config_sysprops(cache) + .context("failed to create device config sysprops")?; let path = get_required_arg::(sub_matches, "out")?; write_output_to_file_or_stdout(path, &output)?; } Some(("dump", sub_matches)) => { let input = open_zero_or_more_files(sub_matches, "cache")?; - let format = get_required_arg::(sub_matches, "format")?; + let format = get_required_arg::(sub_matches, "format") + .context("failed to dump previously parsed flags")?; let output = commands::dump_parsed_flags(input, *format)?; let path = get_required_arg::(sub_matches, "out")?; write_output_to_file_or_stdout(path, &output)?;