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

@@ -67,9 +67,9 @@ fn main() -> Result<(), Box<dyn Error>> {
     config.define_symbols();
 
     #[allow(unused_mut)]
-    let mut config_symbols = config.all();
+    let mut config_symbols = config.all().collect::<Vec<_>>();
     #[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<dyn Error>> {
 // Helper Functions
 
 fn copy_dir_all(
-    config_symbols: &Vec<String>,
+    config_symbols: &[&str],
     src: impl AsRef<Path>,
     dst: impl AsRef<Path>,
 ) -> 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<Path>,
     dst: impl AsRef<Path>,
 ) -> 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 ") {
+        if let Some(condition) = trimmed.strip_prefix("#IF ") {
-            let condition = stripped.to_string();
             let should_take = take.iter().all(|v| *v);
             let should_take = should_take && config.contains(&condition);
             take.push(should_take);

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::Esp32 => &ESP32_CFG,
-            Chip::Esp32c2 => ESP32C2_CFG.clone(),
+            Chip::Esp32c2 => &ESP32C2_CFG,
-            Chip::Esp32c3 => ESP32C3_CFG.clone(),
+            Chip::Esp32c3 => &ESP32C3_CFG,
-            Chip::Esp32c6 => ESP32C6_CFG.clone(),
+            Chip::Esp32c6 => &ESP32C6_CFG,
-            Chip::Esp32h2 => ESP32H2_CFG.clone(),
+            Chip::Esp32h2 => &ESP32H2_CFG,
-            Chip::Esp32s2 => ESP32S2_CFG.clone(),
+            Chip::Esp32s2 => &ESP32S2_CFG,
-            Chip::Esp32s3 => ESP32S3_CFG.clone(),
+            Chip::Esp32s3 => &ESP32S3_CFG,
         }
     }
 
@@ -210,36 +213,26 @@ impl Config {
     }
 
     /// All configuration values for the device.
-    pub fn all(&self) -> Vec<String> {
+    pub fn all(&self) -> impl Iterator<Item = &str> + '_ {
         [
-            vec![
+            self.device.name.as_str(),
-                self.device.name.clone(),
+            self.device.arch.as_ref(),
-                self.device.arch.to_string(),
+            self.device.cores.as_ref(),
-                self.device.cores.to_string(),
-            ],
-            self.device.peripherals.clone(),
-            self.device.symbols.clone(),
         ]
-        .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 {
+    pub fn contains(&self, item: &str) -> bool {
-        self.all().contains(item)
+        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());
+        for symbol in self.all() {
-        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() {
             println!("cargo:rustc-cfg={symbol}");
         }
     }

resources/index.html.jinja

@@ -106,7 +106,7 @@
           {{ meta.chip_pretty }}
         </a>
       </span>
-      <span class="crate-description">{{ meta.description }}</span>
+      <span class="crate-description">{{ meta.name }} (targeting {{ meta.chip_pretty }})</span>
       <span class="crate-version">{{ meta.version }}</span>
     </div>
     {%- endfor %}

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::<Vec<_>>();
 
     // 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<Metadata>, 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<Metadata>, 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<Metadata>, package_path: &Pat
 
 fn run_example(args: ExampleArgs, examples: Vec<Metadata>, 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::<Vec<_>>();
 
     // 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> {
+fn target_triple(package: Package, chip: &Chip) -> Result<&str> {
-    if *package == Package::EspLpHal {
+    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() {
+    ensure!(
-        bail!(
+        *package != Package::EspLpHal || chip.has_lp_core(),
-            "Invalid chip provided for package '{}': '{}'",
+        "Invalid chip provided for package '{}': '{}'",
-            package,
+        package,
-            chip
+        chip
-        );
+    );
-    }
 
     Ok(())
 }