Skip to content

Commit

Permalink
Some xtask/metadata cleanups (esp-rs#1965)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@bugadani
bugadani authored Aug 19, 2024
1 parent f95ab0d commit 59728c5
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 69 deletions.
11 changes: 5 additions & 6 deletions esp-hal/build.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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<()> {
Expand All @@ -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<()> {
Expand All @@ -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);
Expand Down
49 changes: 21 additions & 28 deletions esp-metadata/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ lazy_static::lazy_static! {
strum::Display,
strum::EnumIter,
strum::EnumString,
strum::AsRefStr,
)]
#[serde(rename_all = "lowercase")]
#[strum(serialize_all = "lowercase")]
Expand All @@ -58,6 +59,7 @@ pub enum Arch {
strum::Display,
strum::EnumIter,
strum::EnumString,
strum::AsRefStr,
)]
pub enum Cores {
/// Single CPU core
Expand All @@ -84,6 +86,7 @@ pub enum Cores {
strum::Display,
strum::EnumIter,
strum::EnumString,
strum::AsRefStr,
clap::ValueEnum,
)]
#[serde(rename_all = "kebab-case")]
Expand Down Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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.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}");
}
}
Expand Down
2 changes: 1 addition & 1 deletion resources/index.html.jinja
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
Expand Down
66 changes: 32 additions & 34 deletions xtask/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
)
Expand All @@ -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"))?
Expand All @@ -289,15 +289,15 @@ 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) {
xtask::execute_app(
&package_path,
args.chip,
target,
&test,
test,
action,
args.repeat.unwrap_or(1),
)
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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()),
});
}

Expand Down Expand Up @@ -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 => {
Expand All @@ -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")
}
Expand Down Expand Up @@ -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,
&[
Expand All @@ -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,
&[
Expand All @@ -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,
&[
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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<()> {
Expand Down Expand Up @@ -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`:
Expand All @@ -753,22 +752,21 @@ 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())
}
}

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(())
}
Expand Down

0 comments on commit 59728c5

Please sign in to comment.