From 59728c523f8798d93b040ffeee541df8c2fd948a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Mon, 19 Aug 2024 08:49:05 +0200 Subject: [PATCH] Some xtask/metadata cleanups (#1965) * Clean up almost all clippy violations * Remove redundant variable from context * Do not clone configs * Do not collect all config symbols into a vec needlessly * Do not allocate so many strings --- esp-hal/build.rs | 11 +++---- esp-metadata/src/lib.rs | 49 ++++++++++++---------------- resources/index.html.jinja | 2 +- xtask/src/main.rs | 66 ++++++++++++++++++-------------------- 4 files changed, 59 insertions(+), 69 deletions(-) diff --git a/esp-hal/build.rs b/esp-hal/build.rs index 53d292770..f9aa7fb1e 100644 --- a/esp-hal/build.rs +++ b/esp-hal/build.rs @@ -67,9 +67,9 @@ fn main() -> Result<(), Box> { config.define_symbols(); #[allow(unused_mut)] - let mut config_symbols = config.all(); + let mut config_symbols = config.all().collect::>(); #[cfg(feature = "flip-link")] - config_symbols.push("flip-link".to_owned()); + config_symbols.push("flip-link"); // Place all linker scripts in `OUT_DIR`, and instruct Cargo how to find these // files: @@ -135,7 +135,7 @@ fn main() -> Result<(), Box> { // Helper Functions fn copy_dir_all( - config_symbols: &Vec, + config_symbols: &[&str], src: impl AsRef, dst: impl AsRef, ) -> std::io::Result<()> { @@ -162,7 +162,7 @@ fn copy_dir_all( /// A naive pre-processor for linker scripts fn preprocess_file( - config: &[String], + config: &[&str], src: impl AsRef, dst: impl AsRef, ) -> std::io::Result<()> { @@ -176,8 +176,7 @@ fn preprocess_file( let line = line?; let trimmed = line.trim(); - if let Some(stripped) = trimmed.strip_prefix("#IF ") { - let condition = stripped.to_string(); + if let Some(condition) = trimmed.strip_prefix("#IF ") { let should_take = take.iter().all(|v| *v); let should_take = should_take && config.contains(&condition); take.push(should_take); diff --git a/esp-metadata/src/lib.rs b/esp-metadata/src/lib.rs index 7a5b94327..ea077cddd 100644 --- a/esp-metadata/src/lib.rs +++ b/esp-metadata/src/lib.rs @@ -34,6 +34,7 @@ lazy_static::lazy_static! { strum::Display, strum::EnumIter, strum::EnumString, + strum::AsRefStr, )] #[serde(rename_all = "lowercase")] #[strum(serialize_all = "lowercase")] @@ -58,6 +59,7 @@ pub enum Arch { strum::Display, strum::EnumIter, strum::EnumString, + strum::AsRefStr, )] pub enum Cores { /// Single CPU core @@ -84,6 +86,7 @@ pub enum Cores { strum::Display, strum::EnumIter, strum::EnumString, + strum::AsRefStr, clap::ValueEnum, )] #[serde(rename_all = "kebab-case")] @@ -172,15 +175,15 @@ pub struct Config { impl Config { /// The configuration for the specified chip. - pub fn for_chip(chip: &Chip) -> Self { + pub fn for_chip(chip: &Chip) -> &Self { match chip { - Chip::Esp32 => ESP32_CFG.clone(), - Chip::Esp32c2 => ESP32C2_CFG.clone(), - Chip::Esp32c3 => ESP32C3_CFG.clone(), - Chip::Esp32c6 => ESP32C6_CFG.clone(), - Chip::Esp32h2 => ESP32H2_CFG.clone(), - Chip::Esp32s2 => ESP32S2_CFG.clone(), - Chip::Esp32s3 => ESP32S3_CFG.clone(), + Chip::Esp32 => &ESP32_CFG, + Chip::Esp32c2 => &ESP32C2_CFG, + Chip::Esp32c3 => &ESP32C3_CFG, + Chip::Esp32c6 => &ESP32C6_CFG, + Chip::Esp32h2 => &ESP32H2_CFG, + Chip::Esp32s2 => &ESP32S2_CFG, + Chip::Esp32s3 => &ESP32S3_CFG, } } @@ -210,36 +213,26 @@ impl Config { } /// All configuration values for the device. - pub fn all(&self) -> Vec { + pub fn all(&self) -> impl Iterator + '_ { [ - vec![ - self.device.name.clone(), - self.device.arch.to_string(), - self.device.cores.to_string(), - ], - self.device.peripherals.clone(), - self.device.symbols.clone(), + self.device.name.as_str(), + self.device.arch.as_ref(), + self.device.cores.as_ref(), ] - .concat() + .into_iter() + .chain(self.device.peripherals.iter().map(|s| s.as_str())) + .chain(self.device.symbols.iter().map(|s| s.as_str())) } /// Does the configuration contain `item`? - pub fn contains(&self, item: &String) -> bool { - self.all().contains(item) + pub fn contains(&self, item: &str) -> bool { + self.all().any(|i| i == item) } /// Define all symbols for a given configuration. pub fn define_symbols(&self) { // Define all necessary configuration symbols for the configured device: - println!("cargo:rustc-cfg={}", self.name()); - println!("cargo:rustc-cfg={}", self.arch()); - println!("cargo:rustc-cfg={}", self.cores()); - - for peripheral in self.peripherals() { - println!("cargo:rustc-cfg={peripheral}"); - } - - for symbol in self.symbols() { + for symbol in self.all() { println!("cargo:rustc-cfg={symbol}"); } } diff --git a/resources/index.html.jinja b/resources/index.html.jinja index c0cce6f81..0a7d5506b 100644 --- a/resources/index.html.jinja +++ b/resources/index.html.jinja @@ -106,7 +106,7 @@ {{ meta.chip_pretty }} - {{ meta.description }} + {{ meta.name }} (targeting {{ meta.chip_pretty }}) {{ meta.version }} {%- endfor %} diff --git a/xtask/src/main.rs b/xtask/src/main.rs index db10638f7..bbec3a47e 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -5,7 +5,7 @@ use std::{ process::Command, }; -use anyhow::{bail, Context as _, Result}; +use anyhow::{bail, ensure, Context as _, Result}; use clap::{Args, Parser}; use esp_metadata::{Arch, Chip, Config}; use minijinja::Value; @@ -214,7 +214,7 @@ fn examples(workspace: &Path, mut args: ExampleArgs, action: CargoAction) -> Res .collect::>(); // Sort all examples by name: - examples.sort_by(|a, b| a.name().cmp(&b.name())); + examples.sort_by_key(|a| a.name()); // Execute the specified action: match action { @@ -225,12 +225,12 @@ fn examples(workspace: &Path, mut args: ExampleArgs, action: CargoAction) -> Res fn build_examples(args: ExampleArgs, examples: Vec, package_path: &Path) -> Result<()> { // Determine the appropriate build target for the given package and chip: - let target = target_triple(&args.package, &args.chip)?; + let target = target_triple(args.package, &args.chip)?; if let Some(example) = examples.iter().find(|ex| Some(ex.name()) == args.example) { // Attempt to build only the specified example: xtask::execute_app( - &package_path, + package_path, args.chip, target, example, @@ -244,7 +244,7 @@ fn build_examples(args: ExampleArgs, examples: Vec, package_path: &Pat // Attempt to build each supported example, with all required features enabled: examples.iter().try_for_each(|example| { xtask::execute_app( - &package_path, + package_path, args.chip, target, example, @@ -257,16 +257,16 @@ fn build_examples(args: ExampleArgs, examples: Vec, package_path: &Pat fn run_example(args: ExampleArgs, examples: Vec, package_path: &Path) -> Result<()> { // Determine the appropriate build target for the given package and chip: - let target = target_triple(&args.package, &args.chip)?; + let target = target_triple(args.package, &args.chip)?; // Filter the examples down to only the binary we're interested in, assuming it // actually supports the specified chip: if let Some(example) = examples.iter().find(|ex| Some(ex.name()) == args.example) { xtask::execute_app( - &package_path, + package_path, args.chip, target, - &example, + example, CargoAction::Run, 1, ) @@ -280,7 +280,7 @@ fn tests(workspace: &Path, args: TestArgs, action: CargoAction) -> Result<()> { let package_path = xtask::windows_safe_path(&workspace.join("hil-test")); // Determine the appropriate build target for the given package and chip: - let target = target_triple(&Package::HilTest, &args.chip)?; + let target = target_triple(Package::HilTest, &args.chip)?; // Load all tests which support the specified chip and parse their metadata: let mut tests = xtask::load_examples(&package_path.join("tests"))? @@ -289,7 +289,7 @@ fn tests(workspace: &Path, args: TestArgs, action: CargoAction) -> Result<()> { .collect::>(); // Sort all tests by name: - tests.sort_by(|a, b| a.name().cmp(&b.name())); + tests.sort_by_key(|a| a.name()); // Execute the specified action: if let Some(test) = tests.iter().find(|test| Some(test.name()) == args.test) { @@ -297,7 +297,7 @@ fn tests(workspace: &Path, args: TestArgs, action: CargoAction) -> Result<()> { &package_path, args.chip, target, - &test, + test, action, args.repeat.unwrap_or(1), ) @@ -373,7 +373,7 @@ fn build_documentation_for_package( validate_package_chip(&package, chip)?; // Determine the appropriate build target for the given package and chip: - let target = target_triple(&package, &chip)?; + let target = target_triple(package, chip)?; // Build the documentation for the specified package, targeting the // specified chip: @@ -405,7 +405,6 @@ fn build_documentation_for_package( chip => chip.to_string(), chip_pretty => chip.pretty_name(), package => package.to_string().replace('-', "_"), - description => format!("{} (targeting {})", package, chip.pretty_name()), }); } @@ -490,7 +489,7 @@ fn lint_packages(workspace: &Path, args: LintPackagesArgs) -> Result<()> { // is *some* overlap) for chip in &args.chips { - let device = Config::for_chip(&chip); + let device = Config::for_chip(chip); match package { Package::EspBacktrace => { @@ -509,13 +508,13 @@ fn lint_packages(workspace: &Path, args: LintPackagesArgs) -> Result<()> { let mut features = format!("--features={chip},ci"); // Cover all esp-hal features where a device is supported - if device.contains(&"usb0".to_owned()) { + if device.contains("usb0") { features.push_str(",usb-otg") } - if device.contains(&"bt".to_owned()) { + if device.contains("bt") { features.push_str(",bluetooth") } - if device.contains(&"psram".to_owned()) { + if device.contains("psram") { // TODO this doesn't test octal psram as it would require a separate build features.push_str(",psram-4m,psram-80mhz") } @@ -545,7 +544,7 @@ fn lint_packages(workspace: &Path, args: LintPackagesArgs) -> Result<()> { } Package::EspIeee802154 => { - if device.contains(&"ieee802154".to_owned()) { + if device.contains("ieee802154") { lint_package( &path, &[ @@ -557,7 +556,7 @@ fn lint_packages(workspace: &Path, args: LintPackagesArgs) -> Result<()> { } } Package::EspLpHal => { - if device.contains(&"lp_core".to_owned()) { + if device.contains("lp_core") { lint_package( &path, &[ @@ -569,7 +568,7 @@ fn lint_packages(workspace: &Path, args: LintPackagesArgs) -> Result<()> { } } Package::EspHalSmartled => { - if device.contains(&"rmt".to_owned()) { + if device.contains("rmt") { lint_package( &path, &[ @@ -615,14 +614,14 @@ fn lint_packages(workspace: &Path, args: LintPackagesArgs) -> Result<()> { Package::EspWifi => { let mut features = format!("--features={chip},async,ps-min-modem,defmt"); - if device.contains(&"wifi".to_owned()) { + if device.contains("wifi") { features .push_str(",wifi-default,esp-now,embedded-svc,embassy-net,dump-packets") } - if device.contains(&"bt".to_owned()) { + if device.contains("bt") { features.push_str(",ble") } - if device.contains(&"coex".to_owned()) { + if device.contains("coex") { features.push_str(",coex") } lint_package( @@ -679,7 +678,7 @@ fn lint_package(path: &Path, args: &[&str]) -> Result<()> { .arg("warnings") .build(); - xtask::cargo::run(&cargo_args, &path) + xtask::cargo::run(&cargo_args, path) } fn run_elfs(args: RunElfArgs) -> Result<()> { @@ -728,7 +727,7 @@ fn run_doctests(workspace: &Path, args: ExampleArgs) -> Result<()> { let package_path = xtask::windows_safe_path(&workspace.join(&package_name)); // Determine the appropriate build target for the given package and chip: - let target = target_triple(&args.package, &args.chip)?; + let target = target_triple(args.package, &args.chip)?; let features = vec![args.chip.to_string()]; // Build up an array of command-line arguments to pass to `cargo`: @@ -753,8 +752,8 @@ fn run_doctests(workspace: &Path, args: ExampleArgs) -> Result<()> { // ---------------------------------------------------------------------------- // Helper Functions -fn target_triple<'a>(package: &'a Package, chip: &'a Chip) -> Result<&'a str> { - if *package == Package::EspLpHal { +fn target_triple(package: Package, chip: &Chip) -> Result<&str> { + if package == Package::EspLpHal { chip.lp_target() } else { Ok(chip.target()) @@ -762,13 +761,12 @@ fn target_triple<'a>(package: &'a Package, chip: &'a Chip) -> Result<&'a str> { } fn validate_package_chip(package: &Package, chip: &Chip) -> Result<()> { - if *package == Package::EspLpHal && !chip.has_lp_core() { - bail!( - "Invalid chip provided for package '{}': '{}'", - package, - chip - ); - } + ensure!( + *package != Package::EspLpHal || chip.has_lp_core(), + "Invalid chip provided for package '{}': '{}'", + package, + chip + ); Ok(()) }