From 4439e83be5639ec40cd38ab8404cc57dce66d3b5 Mon Sep 17 00:00:00 2001 From: TimJentzsch Date: Tue, 3 Dec 2024 13:55:06 +0100 Subject: [PATCH] Rework binary detection (#169) # Objective Rework the detection of binaries again, aligning it closer to how Cargo works and enabling several new features / fixing several bugs. - Support multiple binaries in a single package - Support running examples (closes #69) - Fix detection of `target` folder location (closes #145) With this PR, we can now just run `bevy run --example breakout web` in the Bevy repo and it works without any additional configuration! # Solution The code previously made the wrong assumption that we want to run packages with a binary target. Instead, we want to run the binary targets itself. A package can even have multiple of them. Additionally, examples need to be considered as binary targets. The `--example` argument now needs to be factored into the detection algorithm and the different path for examples must be considered in the bundling step. ## Note for Reviewers The crux of the changes is contained in `run/mod.rs` in the `select_run_binary` function. This implements the new algorithm to select the correct binary to run. In `serve.rs`, the `index.html` file now needs to be generated dynamically, in order to pick the path to the correct binary. Because the `index.html` needs to be a _static_ string, it needs to be leaked to create a static reference. --- Cargo.lock | 1 - Cargo.toml | 5 - src/build/args.rs | 5 + src/build/mod.rs | 14 ++- src/external_cli/cargo/metadata.rs | 14 +++ src/external_cli/cargo/mod.rs | 8 ++ src/external_cli/wasm_bindgen.rs | 37 ++---- src/lib.rs | 1 - src/manifest.rs | 24 ---- src/run/args.rs | 5 + src/run/mod.rs | 185 ++++++++++++++++++++--------- src/run/serve.rs | 53 ++++++--- 12 files changed, 227 insertions(+), 125 deletions(-) delete mode 100644 src/manifest.rs diff --git a/Cargo.lock b/Cargo.lock index 69ccdf3..a933dfa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -497,7 +497,6 @@ dependencies = [ "semver", "serde", "serde_json", - "toml_edit", "webbrowser", ] diff --git a/Cargo.toml b/Cargo.toml index 0300397..36ae76d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,11 +33,6 @@ serde_json = "1.0.128" reqwest = { features = ["blocking", "json"], version = "0.12.7" } regex = "1.10.6" -# Understanding Cargo.toml -toml_edit = { version = "0.22.21", default-features = false, features = [ - "parse", -] } - # Understanding package versions semver = { version = "1.0.23", features = ["serde"] } diff --git a/src/build/args.rs b/src/build/args.rs index 5fc7c06..06efcad 100644 --- a/src/build/args.rs +++ b/src/build/args.rs @@ -24,6 +24,11 @@ impl BuildArgs { self.cargo_args.compilation_args.profile() } + /// The targeted platform. + pub(crate) fn target(&self) -> Option { + self.cargo_args.compilation_args.target(self.is_web()) + } + /// Generate arguments to forward to `cargo build`. pub(crate) fn cargo_args_builder(&self) -> ArgBuilder { self.cargo_args.args_builder(self.is_web()) diff --git a/src/build/mod.rs b/src/build/mod.rs index 3a6d991..55f4ceb 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -1,6 +1,6 @@ use crate::{ external_cli::{cargo, rustup, wasm_bindgen, CommandHelpers}, - manifest::package_name, + run::select_run_binary, }; pub use self::args::BuildArgs; @@ -13,11 +13,21 @@ pub fn build(args: &BuildArgs) -> anyhow::Result<()> { if args.is_web() { ensure_web_setup()?; + let metadata = cargo::metadata::metadata_with_args(["--no-deps"])?; + println!("Compiling to WebAssembly..."); cargo::build::command().args(cargo_args).ensure_status()?; println!("Bundling JavaScript bindings..."); - wasm_bindgen::bundle(&package_name()?, args.profile())?; + let bin_target = select_run_binary( + &metadata, + args.cargo_args.package_args.package.as_deref(), + args.cargo_args.target_args.bin.as_deref(), + args.cargo_args.target_args.example.as_deref(), + args.target().as_deref(), + args.profile(), + )?; + wasm_bindgen::bundle(&bin_target)?; } else { cargo::build::command().args(cargo_args).ensure_status()?; } diff --git a/src/external_cli/cargo/metadata.rs b/src/external_cli/cargo/metadata.rs index af69695..9b800e6 100644 --- a/src/external_cli/cargo/metadata.rs +++ b/src/external_cli/cargo/metadata.rs @@ -83,6 +83,20 @@ impl Package { .any(|target_kind| *target_kind == TargetKind::Bin) }) } + + /// An iterator over all binary targets contained in this package. + pub fn bin_targets(&self) -> impl Iterator { + self.targets + .iter() + .filter(|target| target.kind.iter().any(|kind| *kind == TargetKind::Bin)) + } + + /// An iterator over all example targets contained in this package. + pub fn example_targets(&self) -> impl Iterator { + self.targets + .iter() + .filter(|target| target.kind.iter().any(|kind| *kind == TargetKind::Example)) + } } #[derive(Debug, Deserialize)] diff --git a/src/external_cli/cargo/mod.rs b/src/external_cli/cargo/mod.rs index 1199d23..a128864 100644 --- a/src/external_cli/cargo/mod.rs +++ b/src/external_cli/cargo/mod.rs @@ -80,6 +80,14 @@ impl CargoCompilationArgs { } } + pub(crate) fn target(&self, is_web: bool) -> Option { + if is_web { + Some("wasm32-unknown-unknown".to_string()) + } else { + self.target.clone() + } + } + pub(crate) fn args_builder(&self, is_web: bool) -> ArgBuilder { // web takes precedence over --target let target = if is_web { diff --git a/src/external_cli/wasm_bindgen.rs b/src/external_cli/wasm_bindgen.rs index 46738a1..088bbbd 100644 --- a/src/external_cli/wasm_bindgen.rs +++ b/src/external_cli/wasm_bindgen.rs @@ -1,40 +1,29 @@ -use std::{path::Path, process::Command}; +use std::process::Command; + +use crate::{external_cli::CommandHelpers, run::BinTarget}; use super::arg_builder::ArgBuilder; pub(crate) const PACKAGE: &str = "wasm-bindgen-cli"; pub(crate) const PROGRAM: &str = "wasm-bindgen"; -/// Determine the path to the folder where the Wasm build artifacts are stored. -pub(crate) fn get_target_folder(profile: &str) -> String { - format!("target/wasm32-unknown-unknown/{profile}") -} - /// Bundle the Wasm build for the web. -pub(crate) fn bundle(package_name: &str, profile: &str) -> anyhow::Result<()> { - let target_folder = get_target_folder(profile); +pub(crate) fn bundle(bin_target: &BinTarget) -> anyhow::Result<()> { + let original_wasm = bin_target + .artifact_directory + .clone() + .join(format!("{}.wasm", bin_target.bin_name)); - let status = Command::new(PROGRAM) + Command::new(PROGRAM) .args( ArgBuilder::new() .arg("--no-typescript") - .add_with_value("--out-name", "bevy_app") - .add_with_value("--out-dir", &target_folder) + .add_with_value("--out-name", &bin_target.bin_name) + .add_with_value("--out-dir", bin_target.artifact_directory.to_string_lossy()) .add_with_value("--target", "web") - .arg(format!("{target_folder}/{package_name}.wasm")), + .arg(original_wasm.to_string_lossy()), ) - .status()?; + .ensure_status()?; - anyhow::ensure!(status.success(), "Failed to bundle project for the web."); Ok(()) } - -/// Determine if a file path in the target folder is an artifact generated by wasm-bindgen. -pub(crate) fn is_bindgen_artifact(path: &Path) -> bool { - // The JS interface wrapping the WASM binary - let js_path = Path::new("bevy_app.js"); - // The WASM bindgen - let wasm_path = Path::new("bevy_app_bg.wasm"); - - path == js_path || path == wasm_path -} diff --git a/src/lib.rs b/src/lib.rs index fd57a5b..c56abc8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3,6 +3,5 @@ pub mod build; pub mod external_cli; pub mod lint; -pub mod manifest; pub mod run; pub mod template; diff --git a/src/manifest.rs b/src/manifest.rs deleted file mode 100644 index 3d08c0e..0000000 --- a/src/manifest.rs +++ /dev/null @@ -1,24 +0,0 @@ -use std::{fs::File, io::Read as _}; - -use toml_edit::{DocumentMut, Item, Value}; - -/// Get the contents of the manifest file. -fn get_cargo_toml(folder_name: &str) -> anyhow::Result { - let mut file = File::open(format!("{folder_name}/Cargo.toml"))?; - - let mut content = String::new(); - file.read_to_string(&mut content)?; - - Ok(content.parse()?) -} - -/// Determine the name of the cargo package. -pub(crate) fn package_name() -> anyhow::Result { - let cargo_toml = get_cargo_toml("./")?; - - if let Item::Value(Value::String(name)) = &cargo_toml["package"]["name"] { - Ok(name.value().clone()) - } else { - Err(anyhow::anyhow!("No package name defined in Cargo.toml")) - } -} diff --git a/src/run/args.rs b/src/run/args.rs index 4723330..b358f81 100644 --- a/src/run/args.rs +++ b/src/run/args.rs @@ -24,6 +24,11 @@ impl RunArgs { self.cargo_args.compilation_args.profile() } + /// The targeted platform. + pub(crate) fn target(&self) -> Option { + self.cargo_args.compilation_args.target(self.is_web()) + } + /// Generate arguments for `cargo`. pub(crate) fn cargo_args_builder(&self) -> ArgBuilder { self.cargo_args.args_builder(self.is_web()) diff --git a/src/run/mod.rs b/src/run/mod.rs index 369427e..a202ef2 100644 --- a/src/run/mod.rs +++ b/src/run/mod.rs @@ -1,13 +1,11 @@ +use std::path::PathBuf; + use args::RunSubcommands; use crate::{ build::ensure_web_setup, external_cli::{ - cargo::{ - self, - metadata::{Metadata, Package}, - run::CargoRunArgs, - }, + cargo::{self, metadata::Metadata}, wasm_bindgen, CommandHelpers, }, }; @@ -30,10 +28,15 @@ pub fn run(args: &RunArgs) -> anyhow::Result<()> { cargo::build::command().args(cargo_args).ensure_status()?; println!("Bundling JavaScript bindings..."); - let package_name = select_run_package(&metadata, &args.cargo_args)? - .name - .clone(); - wasm_bindgen::bundle(&package_name, args.profile())?; + let bin_target = select_run_binary( + &metadata, + args.cargo_args.package_args.package.as_deref(), + args.cargo_args.target_args.bin.as_deref(), + args.cargo_args.target_args.example.as_deref(), + args.target().as_deref(), + args.profile(), + )?; + wasm_bindgen::bundle(&bin_target)?; let port = web_args.port; let url = format!("http://localhost:{port}"); @@ -50,7 +53,7 @@ pub fn run(args: &RunArgs) -> anyhow::Result<()> { println!("Open your app at <{url}>!"); } - serve::serve(port, args.profile())?; + serve::serve(bin_target, port)?; } else { // For native builds, wrap `cargo run` cargo::run::command().args(cargo_args).ensure_status()?; @@ -59,60 +62,134 @@ pub fn run(args: &RunArgs) -> anyhow::Result<()> { Ok(()) } -/// Determine which package should be run. +#[derive(Debug, Clone)] +pub(crate) struct BinTarget { + /// The path to the directory in `target` which contains the binary. + pub(crate) artifact_directory: PathBuf, + /// The name of the binary (without any extensions). + pub(crate) bin_name: String, +} + +/// Determine which binary target should be run. /// -/// We first take a look at the `--bin` and `--package` args. -/// If they are not defined, we try to determine the package automatically with the information -/// provided by cargo metadata. -/// We first look for the `default_run` definition and otherwise check if there is only a single -/// binary package that could be run. -fn select_run_package<'a>( - metadata: &'a Metadata, - args: &CargoRunArgs, -) -> anyhow::Result<&'a Package> { - let package_name = if let Some(bin) = &args.target_args.bin { - bin.clone() - } else if let Some(package) = &args.package_args.package { - package.clone() - } else { - // Try to determine the run package automatically - let default_runs: Vec<_> = metadata +/// The `--package` arg narrows down the search space to the given package, +/// while the `--bin` and `--example` args determine the binary target within the selected packages. +/// +/// If the search couldn't be narrowed down to a single binary, +/// the `default_run` option is taken into account. +/// +/// The path to the compiled binary is determined via the compilation target and profile. +pub(crate) fn select_run_binary( + metadata: &Metadata, + package_name: Option<&str>, + bin_name: Option<&str>, + example_name: Option<&str>, + compile_target: Option<&str>, + compile_profile: &str, +) -> anyhow::Result { + // Determine which packages the binary could be in + let packages = if let Some(package_name) = package_name { + let package = metadata .packages .iter() - .filter_map(|package| package.default_run.clone()) + .find(|package| package.name == *package_name) + .ok_or_else(|| anyhow::anyhow!("Failed to find package {package_name}"))?; + vec![package] + } else { + metadata.packages.iter().collect() + }; + + let mut is_example = false; + + let target = if let Some(bin_name) = bin_name { + // The user specified a concrete binary + let bins: Vec<_> = packages + .iter() + .flat_map(|package| { + package + .bin_targets() + .filter(|target| target.name == *bin_name) + }) + .collect(); + + if bins.is_empty() { + anyhow::bail!("No binary with name {bin_name} available!"); + } else if bins.len() > 1 { + anyhow::bail!("Multiple binaries with name {bin_name} available!"); + } + + bins[0] + } else if let Some(example_name) = example_name { + // The user specified a concrete example + let examples: Vec<_> = packages + .iter() + .flat_map(|package| { + package + .example_targets() + .filter(|target| target.name == *example_name) + }) + .collect(); + + if examples.is_empty() { + anyhow::bail!("No example with name {example_name} available!"); + } else if examples.len() > 1 { + anyhow::bail!("Multiple examples with name {example_name} available!"); + } + + is_example = true; + examples[0] + } else { + // Nothing concrete specified, try to pick one automatically + + // If there is only one binary, pick that one + let bins: Vec<_> = packages + .iter() + .flat_map(|package| package.bin_targets()) .collect(); - anyhow::ensure!(default_runs.len() <= 1, "More than one default run target"); - if let Some(default_run) = default_runs.into_iter().next() { - default_run + if bins.is_empty() { + anyhow::bail!("No binaries available!"); + } else if bins.len() == 1 { + bins[0] } else { - // If there is only one package with binary target, use that - let bin_packages: Vec<_> = metadata - .packages + // Otherwise, check if there is a default run target defined + let default_runs: Vec<_> = packages .iter() - .filter(|package| package.has_bin()) + .filter_map(|package| package.default_run.as_ref()) .collect(); - anyhow::ensure!( - bin_packages.len() <= 1, - "Multiple binary targets found: {}\nPlease select one with the `--bin` argument", - bin_packages - .iter() - .map(|package| package.name.clone()) - .collect::>() - .join(", ") - ); - - anyhow::ensure!(bin_packages.len() == 1, "No binary target found"); - bin_packages[0].name.clone() + + if default_runs.is_empty() { + anyhow::bail!("There are multiple binaries available, try specifying one with --bin or define `default_run` in the Cargo.toml"); + } else if default_runs.len() > 1 { + anyhow::bail!( + "Found multiple `default_run` definitions, I don't know which one to pick!" + ); + } else { + let default_run = default_runs[0]; + bins.iter() + .find(|bin| bin.name == *default_run) + .ok_or_else(|| { + anyhow::anyhow!("Didn't find `default_run` binary {default_run}") + })? + } } }; - match metadata - .packages - .iter() - .find(|package| package.name == package_name) - { - Some(package) => Ok(package), - None => Err(anyhow::anyhow!("Didn't find package {package_name}")), + // Assemble the path where the binary will be put + let mut artifact_directory = metadata.target_directory.clone(); + + if let Some(target) = compile_target { + artifact_directory.push(target); } + + artifact_directory.push(compile_profile); + + if is_example { + artifact_directory.push("examples"); + } + + Ok(BinTarget { + bin_name: target.name.clone(), + artifact_directory, + }) } diff --git a/src/run/serve.rs b/src/run/serve.rs index 9f2752a..5ca5343 100644 --- a/src/run/serve.rs +++ b/src/run/serve.rs @@ -2,15 +2,10 @@ use actix_web::{rt, web, App, HttpResponse, HttpServer, Responder}; use std::path::Path; -use crate::external_cli::wasm_bindgen; - -/// If the user didn't provide an `index.html`, serve a default one. -async fn serve_default_index() -> impl Responder { - let content = include_str!(concat!( - env!("CARGO_MANIFEST_DIR"), - "/assets/web/index.html" - )); +use super::BinTarget; +/// Serve a static HTML file with the given content. +async fn serve_static_html(content: &'static str) -> impl Responder { // Build the HTTP response with appropriate headers to serve the content as a file HttpResponse::Ok() .insert_header(( @@ -20,19 +15,49 @@ async fn serve_default_index() -> impl Responder { .body(content) } +/// Create the default `index.html` if the user didn't provide one. +fn default_index(bin_target: &BinTarget) -> &'static str { + let template = include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/assets/web/index.html" + )); + + // Insert correct path to JS bindings + let index = template.replace( + "./build/bevy_app.js", + format!("./build/{}.js", bin_target.bin_name).as_str(), + ); + + // Only static strings can be served in the web app, + // so we leak the string memory to convert it to a static reference. + // PERF: This is assumed to be used only once and is needed for the rest of the app running + // time, making the memory leak acceptable. + Box::leak(index.into_boxed_str()) +} + /// Launch a web server running the Bevy app. -pub(crate) fn serve(port: u16, profile: &str) -> anyhow::Result<()> { - let profile = profile.to_string(); +pub(crate) fn serve(bin_target: BinTarget, port: u16) -> anyhow::Result<()> { + let index_html = default_index(&bin_target); rt::System::new().block_on( HttpServer::new(move || { let mut app = App::new(); + let bin_target = bin_target.clone(); // Serve the build artifacts at the `/build/*` route - // A custom `index.html` will have to call `/build/bevy_app.js` + // A custom `index.html` will have to call `/build/{bin_name}.js` app = app.service( - actix_files::Files::new("/build", wasm_bindgen::get_target_folder(&profile)) - .path_filter(|path, _| wasm_bindgen::is_bindgen_artifact(path)), + actix_files::Files::new("/build", bin_target.artifact_directory.clone()) + // This potentially includes artifacts which we will not need, + // but we can't add the bin name to the check due to lifetime requirements + .path_filter(move |path, _| { + path.file_stem().is_some_and(|stem| { + // Using `.starts_with` instead of equality, because of the `_bg` suffix + // of the WASM bindings + stem.to_string_lossy().starts_with(&bin_target.bin_name) + }) && (path.extension().is_some_and(|ext| ext == "js") + || path.extension().is_some_and(|ext| ext == "wasm")) + }), ); // If the app has an assets folder, serve it under `/assets` @@ -45,7 +70,7 @@ pub(crate) fn serve(port: u16, profile: &str) -> anyhow::Result<()> { app = app.service(actix_files::Files::new("/", "./web").index_file("index.html")); } else { // If the user doesn't provide a custom web setup, serve a default `index.html` - app = app.route("/", web::get().to(serve_default_index)) + app = app.route("/", web::get().to(|| serve_static_html(index_html))) } app