From 526f43136dbbd08da83d2a44a68fa537c385d9c3 Mon Sep 17 00:00:00 2001 From: Ian Chamberlain Date: Sat, 1 Jun 2024 18:43:45 -0400 Subject: [PATCH] Refactor everything to use the new configs Build all the artifacts, then run if needed. It basically works! There's a couple of changes in behavior but overall I think this gets us much closer to the goal. --- src/command.rs | 83 ++++++++++++--------- src/config.rs | 198 ++++++++++++++++++++++++++++++++----------------- src/lib.rs | 77 ++----------------- src/main.rs | 7 +- 4 files changed, 189 insertions(+), 176 deletions(-) diff --git a/src/command.rs b/src/command.rs index 02f998f..b0aeeca 100644 --- a/src/command.rs +++ b/src/command.rs @@ -3,10 +3,11 @@ use std::io::Read; use std::process::Stdio; use std::sync::OnceLock; -use cargo_metadata::Message; +use cargo_metadata::{Message, Metadata}; use clap::{Args, Parser, Subcommand}; -use crate::{build_3dsx, build_smdh, cargo, get_metadata, link, print_command, CTRConfig}; +use crate::config; +use crate::{build_3dsx, build_smdh, cargo, link, print_command, CTRConfig}; #[derive(Parser, Debug)] #[command(name = "cargo", bin_name = "cargo")] @@ -295,23 +296,45 @@ impl CargoCmd { /// /// - `cargo 3ds build` and other "build" commands will use their callbacks to build the final `.3dsx` file and link it. /// - `cargo 3ds new` and other generic commands will use their callbacks to make 3ds-specific changes to the environment. - pub fn run_callback(&self, messages: &[Message]) { - // Process the metadata only for commands that have it/use it - let config = if self.should_build_3dsx() { - eprintln!("Getting metadata"); + pub fn run_callback(&self, messages: &[Message], metadata: &Metadata) { + // We perform a lookup by package so that we only build the 3dsx when the + // artifact was actually produced *by* that package. Otherwise, we'd end + // up building each 3dsx once for every package in the workspace. + let cargo_3ds_by_package = config::Cargo3DS::from_metadata(metadata); - Some(get_metadata(messages)) - } else { - None - }; + let configs: Vec<_> = messages + .iter() + .filter_map(|m| match m { + Message::CompilerArtifact(artifact) => cargo_3ds_by_package + .get(&artifact.package_id)? + .artifact_config(metadata, artifact), + _ => None, + }) + .collect(); + + // First, run all the build callbacks for each target (i.e. build its .3dsx) + for config in &configs { + match self { + Self::Build(cmd) => cmd.callback(config), + Self::Run(cmd) => cmd.build_args.callback(config), + Self::Test(cmd) => cmd.run_args.build_args.callback(config), + _ => {} + } + } - // Run callback only for commands that use it + // For run + test, we can only run one of the targets. Error if more or less + // than one executable was built, otherwise run the callback for the first target. match self { - Self::Build(cmd) => cmd.callback(&config), - Self::Run(cmd) => cmd.callback(&config), - Self::Test(cmd) => cmd.callback(&config), + Self::Run(_) | Self::Test(Test { no_run: false, .. }) if configs.len() != 1 => { + let names: Vec<_> = configs.iter().map(|c| c.name.clone()).collect(); + eprintln!("Error: expected exactly one executable to run, got {names:?}"); + // TODO: should we exit nonzero here maybe? + } + Self::Run(cmd) => cmd.callback(&configs[0]), + Self::Test(cmd) => cmd.callback(&configs[0]), + // New is a special case where we always want to run its callback Self::New(cmd) => cmd.callback(), - _ => (), + _ => {} } } } @@ -346,14 +369,12 @@ impl Build { /// Callback for `cargo 3ds build`. /// /// This callback handles building the application as a `.3dsx` file. - fn callback(&self, config: &Option) { - if let Some(config) = config { - eprintln!("Building smdh: {}", config.path_smdh().display()); - build_smdh(config, self.verbose); + fn callback(&self, config: &CTRConfig) { + eprintln!("Building smdh: {}", config.path_smdh().display()); + build_smdh(config, self.verbose); - eprintln!("Building 3dsx: {}", config.path_3dsx().display()); - build_3dsx(config, self.verbose); - } + eprintln!("Building 3dsx: {}", config.path_3dsx().display()); + build_3dsx(config, self.verbose); } } @@ -402,15 +423,10 @@ impl Run { /// Callback for `cargo 3ds run`. /// /// This callback handles launching the application via `3dslink`. - fn callback(&self, config: &Option) { - // Run the normal "build" callback - self.build_args.callback(config); - + fn callback(&self, config: &CTRConfig) { if !self.use_custom_runner() { - if let Some(cfg) = config { - eprintln!("Running 3dslink"); - link(cfg, self, self.build_args.verbose); - } + eprintln!("Running 3dslink"); + link(config, self, self.build_args.verbose); } } @@ -461,11 +477,8 @@ impl Test { /// Callback for `cargo 3ds test`. /// /// This callback handles launching the application via `3dslink`. - fn callback(&self, config: &Option) { - if self.no_run { - // If the tests don't have to run, use the "build" callback - self.run_args.build_args.callback(config); - } else { + fn callback(&self, config: &CTRConfig) { + if !self.no_run { // If the tests have to run, use the "run" callback self.run_args.callback(config); } diff --git a/src/config.rs b/src/config.rs index 5f27374..7a29b41 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,28 +1,37 @@ -// Beginning sketch of +//! This module defines the in-memory representation of the `cargo-3ds` section +//! of metadata in `Cargo.toml`. use std::collections::hash_map::Entry; use std::collections::HashMap; -use std::path::PathBuf; +use std::env; -use cargo_metadata::{Metadata, PackageId}; +use cargo_metadata::camino::Utf8PathBuf; +use cargo_metadata::{Artifact, Metadata, PackageId}; use serde::Deserialize; +use crate::CTRConfig; + /// The `cargo-3ds` section of a `Cargo.toml` file for a single package. #[derive(Debug, Deserialize, Default, PartialEq, Eq)] pub struct Cargo3DS { /// The default configuration for all targets in the package. These values /// will be used if a target does not have its own values specified. + /// // It might be nice to use a `#[serde(default)]` attribute here, but it doesn't // work with `flatten`: https://github.com/serde-rs/serde/issues/1879 #[serde(flatten)] pub default: TargetMetadata, /// Configuration for each example target in the package. - #[serde(default)] + #[serde(default, rename = "example")] pub examples: HashMap, + /// Configuration for each binary target in the package. + #[serde(default, rename = "bin")] + pub bins: HashMap, + /// Configuration for each integration test target in the package. - #[serde(default)] + #[serde(default, rename = "test")] pub tests: HashMap, /// Configuration for the lib test executable (a.k.a unit tests). @@ -52,6 +61,8 @@ impl Cargo3DS { .clone_from(&package.description); } + // TODO copy authors. Maybe we should do a ", " join of all authors? + if let Some(package_meta) = package .metadata @@ -67,25 +78,83 @@ impl Cargo3DS { result } + /// Walk the list of provided messages and return a [`CTRConfig`] for each + /// executable artifact that was built (e.g. an example, a test, or the lib tests). + pub fn artifact_config(&self, metadata: &Metadata, artifact: &Artifact) -> Option { + let package = &metadata[&artifact.package_id]; + let target = &artifact.target; + let profile = &artifact.profile; + let mut target_name = target.name.clone(); + + let mut metadata = None; + for kind in &target.kind { + metadata = match kind.as_str() { + "lib" | "rlib" | "dylib" | "bin" if profile.test => { + target_name = format!("{target_name} tests"); + self.lib.as_ref() + } + "example" => { + target_name = format!("{target_name} - {} example", package.name); + self.examples.get(&target_name) + } + "test" => self.tests.get(&target_name), + "bin" => self.bins.get(&target_name), + _ => continue, + }; + + break; + } + + let target_metadata = metadata.unwrap_or(&self.default); + + // TODO: restore old behavior of trying ./icon.png if it exists + let icon_path = target_metadata + .icon + .as_ref() + .and_then(|path| Some(package.manifest_path.parent()?.join(path))) + .unwrap_or_else(|| { + let devkitpro_dir = Utf8PathBuf::from(&env::var("DEVKITPRO").unwrap()); + devkitpro_dir.join("libctru").join("default_icon.png") + }); + + let author = target_metadata + .author + .clone() + .unwrap_or_else(|| String::from("Unspecified Author")); + + let description = target_metadata + .description + .clone() + .unwrap_or_else(|| String::from("Homebrew Application")); + + let executable = artifact.executable.clone()?; + + Some(CTRConfig { + name: target_name, + author, + description, + icon: icon_path.into(), + target_path: executable.into(), + cargo_manifest_path: package.manifest_path.clone().into(), + }) + } + /// Merge another [`Cargo3DS`] into this one. Each target specified by the given /// configuration may set options to overide the current configuration. fn merge(&mut self, other: Self) { self.default.merge(other.default); - for (name, example) in other.examples { - match self.examples.entry(name) { - Entry::Occupied(mut t) => t.get_mut().merge(example), - Entry::Vacant(t) => { - t.insert(example); - } - } - } - - for (name, test) in other.tests { - match self.tests.entry(name) { - Entry::Occupied(mut t) => t.get_mut().merge(test), - Entry::Vacant(t) => { - t.insert(test); + for (self_targets, other_targets) in [ + (&mut self.bins, other.bins), + (&mut self.examples, other.examples), + (&mut self.tests, other.tests), + ] { + for (name, target) in other_targets { + match self_targets.entry(name) { + Entry::Occupied(mut t) => t.get_mut().merge(target), + Entry::Vacant(t) => { + t.insert(target); + } } } } @@ -100,17 +169,22 @@ impl Cargo3DS { } } -#[derive(Debug, Deserialize, PartialEq, Eq)] +// TODO: maybe this should just *be* CTRConfig? It might not be necessary to do the +// translation between them if we just deserialize directly into CTRConfig. +#[derive(Default, Debug, Deserialize, PartialEq, Eq)] pub struct TargetMetadata { - /// The path to the icon file for the target, relative to `Cargo.toml`. - pub icon: Option, + /// The path to the icon file for the executable, relative to `Cargo.toml`. + pub icon: Option, - /// The path to the ROMFS directory for the target, relative to `Cargo.toml`. + /// The path to the ROMFS directory for the executable, relative to `Cargo.toml`. #[serde(alias = "romfs-dir")] - pub romfs_dir: Option, + pub romfs_dir: Option, - /// A short description of the target, used in the homebrew menu. + /// A short description of the executable, used in the homebrew menu. pub description: Option, + + /// The author of the executable, used in the homebrew menu. + pub author: Option, } impl TargetMetadata { @@ -120,16 +194,6 @@ impl TargetMetadata { } } -impl Default for TargetMetadata { - fn default() -> Self { - Self { - icon: Some(PathBuf::from("icon.png")), - romfs_dir: Some(PathBuf::from("romfs")), - description: None, - } - } -} - #[cfg(test)] mod tests { use toml::toml; @@ -141,12 +205,12 @@ mod tests { let value = toml! { romfs_dir = "my_romfs" - examples.example1.icon = "example1.png" - examples.example1.romfs-dir = "example1-romfs" + example.example1.icon = "example1.png" + example.example1.romfs-dir = "example1-romfs" - examples.example2.icon = "example2.png" + example.example2.icon = "example2.png" - tests.test1.romfs_dir = "test1-romfs" + test.test1.romfs_dir = "test1-romfs" lib.icon = "lib.png" lib.romfs_dir = "lib-romfs" @@ -157,9 +221,9 @@ mod tests { assert_eq!( config.default, TargetMetadata { + romfs_dir: Some(Utf8PathBuf::from("my_romfs")), icon: None, - romfs_dir: Some(PathBuf::from("my_romfs")), - description: None, + ..Default::default() } ); @@ -169,17 +233,16 @@ mod tests { ( String::from("example1"), TargetMetadata { - icon: Some(PathBuf::from("example1.png")), - romfs_dir: Some(PathBuf::from("example1-romfs")), - description: None, + icon: Some(Utf8PathBuf::from("example1.png")), + romfs_dir: Some(Utf8PathBuf::from("example1-romfs")), + ..Default::default() } ), ( String::from("example2"), TargetMetadata { - icon: Some(PathBuf::from("example2.png")), - romfs_dir: None, - description: None, + icon: Some(Utf8PathBuf::from("example2.png")), + ..Default::default() } ), ]) @@ -190,9 +253,8 @@ mod tests { HashMap::from_iter([( String::from("test1"), TargetMetadata { - icon: None, - romfs_dir: Some(PathBuf::from("test1-romfs")), - description: None, + romfs_dir: Some(Utf8PathBuf::from("test1-romfs")), + ..Default::default() } )]) ); @@ -200,9 +262,9 @@ mod tests { assert_eq!( config.lib, Some(TargetMetadata { - icon: Some(PathBuf::from("lib.png")), - romfs_dir: Some(PathBuf::from("lib-romfs")), - description: None, + icon: Some(Utf8PathBuf::from("lib.png")), + romfs_dir: Some(Utf8PathBuf::from("lib-romfs")), + ..Default::default() }) ); } @@ -221,7 +283,7 @@ mod tests { // Assert that at least one package has the configured value for its romfs_dir. config .values() - .find(|cfg| cfg.default.romfs_dir == Some(PathBuf::from("examples/romfs"))) + .find(|cfg| cfg.default.romfs_dir == Some(Utf8PathBuf::from("examples/romfs"))) .unwrap(); } @@ -232,10 +294,12 @@ mod tests { let first: Cargo3DS = toml! { romfs_dir = "first_romfs" - examples.example1.icon = "example1.png" - examples.example2.icon = "example2.png" + bin.cool-bin.icon = "cool.png" - tests.test1.romfs_dir = "test1-romfs" + example.example1.icon = "example1.png" + example.example2.icon = "example2.png" + + test.test1.romfs_dir = "test1-romfs" lib.romfs_dir = "lib-romfs" } @@ -243,8 +307,8 @@ mod tests { .unwrap(); let next: Cargo3DS = toml! { - examples.example1.romfs-dir = "example-dir" - tests.test1.romfs_dir = "test1-next-romfs" + example.example1.romfs-dir = "example-dir" + test.test1.romfs_dir = "test1-next-romfs" lib.romfs_dir = "lib-override" } .try_into() @@ -253,25 +317,23 @@ mod tests { config.merge(first); config.merge(next); - let mut expected: Cargo3DS = toml! { + let expected: Cargo3DS = toml! { romfs_dir = "first_romfs" - examples.example1.icon = "example1.png" - examples.example1.romfs-dir = "example-dir" + bin.cool-bin.icon = "cool.png" + + example.example1.icon = "example1.png" + example.example1.romfs-dir = "example-dir" - examples.example2.icon = "example2.png" + example.example2.icon = "example2.png" - tests.test1.romfs_dir = "test1-next-romfs" + test.test1.romfs_dir = "test1-next-romfs" lib.romfs_dir = "lib-override" } .try_into() .unwrap(); - // Serde parsing won't set the default but since we started from a default - // we can just set it in the expected struct here. - expected.default.icon = Some(PathBuf::from("icon.png")); - assert_eq!(config, expected); } } diff --git a/src/lib.rs b/src/lib.rs index 4d5b47a..5be588b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,14 +1,16 @@ pub mod command; + +mod config; mod graph; use core::fmt; use std::ffi::OsStr; use std::io::{BufRead, BufReader}; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::process::{Command, ExitStatus, Stdio}; use std::{env, io, process}; -use cargo_metadata::{Message, MetadataCommand}; +use cargo_metadata::Message; use command::{Input, Test}; use rustc_version::Channel; use semver::Version; @@ -244,75 +246,6 @@ pub fn check_rust_version() { } } -/// Parses messages returned by "build" cargo commands (such as `cargo 3ds build` or `cargo 3ds run`). -/// The returned [`CTRConfig`] is then used for further building in and execution -/// in [`build_smdh`], [`build_3dsx`], and [`link`]. -pub fn get_metadata(messages: &[Message]) -> CTRConfig { - let metadata = MetadataCommand::new() - .no_deps() - .exec() - .expect("Failed to get cargo metadata"); - - let mut package = None; - let mut artifact = None; - - // Extract the final built executable. We may want to fail in cases where - // multiple executables, or none, were built? - for message in messages.iter().rev() { - if let Message::CompilerArtifact(art) = message { - if art.executable.is_some() { - package = Some(metadata[&art.package_id].clone()); - artifact = Some(art.clone()); - - break; - } - } - } - if package.is_none() || artifact.is_none() { - eprintln!("No executable found from build command output!"); - process::exit(1); - } - - let (package, artifact) = (package.unwrap(), artifact.unwrap()); - - let mut icon = String::from("./icon.png"); - - if !Path::new(&icon).exists() { - icon = format!( - "{}/libctru/default_icon.png", - env::var("DEVKITPRO").unwrap() - ); - } - - // for now assume a single "kind" since we only support one output artifact - let name = match artifact.target.kind[0].as_ref() { - "bin" | "lib" | "rlib" | "dylib" if artifact.target.test => { - format!("{} tests", artifact.target.name) - } - "example" => { - format!("{} - {} example", artifact.target.name, package.name) - } - _ => artifact.target.name, - }; - - let author = match package.authors.as_slice() { - [name, ..] => name.clone(), - [] => String::from("Unspecified Author"), // as standard with the devkitPRO toolchain - }; - - CTRConfig { - name, - author, - description: package - .description - .clone() - .unwrap_or_else(|| String::from("Homebrew Application")), - icon, - target_path: artifact.executable.unwrap().into(), - cargo_manifest_path: package.manifest_path.into(), - } -} - /// Builds the smdh using `smdhtool`. /// This will fail if `smdhtool` is not within the running directory or in a directory found in $PATH pub fn build_smdh(config: &CTRConfig, verbose: bool) { @@ -437,7 +370,7 @@ pub fn get_romfs_path(config: &CTRConfig) -> (PathBuf, bool) { (romfs_path, is_default) } -#[derive(Default)] +#[derive(Default, Debug)] pub struct CTRConfig { name: String, author: String, diff --git a/src/main.rs b/src/main.rs index 022e635..0955dfe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -17,11 +17,16 @@ fn main() { } }; + let metadata = cargo_metadata::MetadataCommand::new() + .no_deps() + .exec() + .unwrap(); + let (status, messages) = run_cargo(&input, message_format); if !status.success() { process::exit(status.code().unwrap_or(1)); } - input.cmd.run_callback(&messages); + input.cmd.run_callback(&messages, &metadata); }