diff --git a/crates/hiroz-union/src/plugin/wasm/host/ros.rs b/crates/hiroz-union/src/plugin/wasm/host/ros.rs index 04dc425f0..43907409e 100644 --- a/crates/hiroz-union/src/plugin/wasm/host/ros.rs +++ b/crates/hiroz-union/src/plugin/wasm/host/ros.rs @@ -389,8 +389,15 @@ impl hu::plugin::ros::Host for PluginState { // bytes onto that endpoint's key — reject instead (same guard as // the discovery branch below). An empty topic has no live type, // so publishing to it still works, like `ros2 topic pub`. + // The graph reports a DDS-mangled name + // (`std_msgs::msg::dds_::String_`) while `--msg-type` is the ROS + // form (`std_msgs/msg/String`). Comparing them raw rejects a + // correct publish. This branch only became reachable once a + // schema could resolve without a live node, so nothing exercised + // it before: the one test that resolves from disk publishes to an + // *empty* topic, where there is no live type to compare against. if let Some(live) = self.live_topic_type_info(&topic) - && live.name != type_name + && hiroz::dynamic::ros_type_name_from_dds(&live.name) != type_name { return Err(PluginError::Invalid(format!( "topic {topic} carries {}, not the requested {type_name}", diff --git a/crates/hiroz/build.rs b/crates/hiroz/build.rs index 6d0bc02b7..bc2c4b35b 100644 --- a/crates/hiroz/build.rs +++ b/crates/hiroz/build.rs @@ -16,6 +16,8 @@ fn main() { println!("cargo:rerun-if-changed=src/config.rs"); println!("cargo:rerun-if-env-changed=HIROZ_CONFIG_OUTPUT_DIR"); + embed_bundled_msgs(); + // Generate C FFI header when ffi feature is enabled #[cfg(feature = "ffi")] { @@ -118,3 +120,88 @@ fn main() { ); } } + +/// Embed the bundled `.msg` definitions as source text, so a binary with no +/// `HIROZ_MSG_PATH` and no live publisher can still resolve a schema. +/// +/// Emits `$OUT_DIR/embedded_msgs.rs`: a sorted `&[(&str, &str)]` of +/// `pkg/msg/Name` to the file's contents, via `include_str!` so the bytes are +/// the ones on disk at build time and cannot drift from them. +/// +/// The whole jazzy set is ~122 KB of text. Subsetting it would only trade a +/// fraction of a percent of binary size for "why is my type missing?", so +/// everything bundled is embedded. +/// +/// Reads `../hiroz-codegen/assets/{distro}` directly rather than calling +/// `hiroz_codegen::bundled_assets_dir`, which would make hiroz-codegen a +/// build-dependency for a directory walk. If the directory is absent the table +/// is empty and schema resolution behaves exactly as it did before. +fn embed_bundled_msgs() { + use std::fmt::Write as _; + + let manifest = std::path::PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap()); + let distro = if std::env::var_os("CARGO_FEATURE_HUMBLE").is_some() { + "humble" + } else { + "jazzy" + }; + let assets = manifest + .join("..") + .join("hiroz-codegen") + .join("assets") + .join(distro); + println!("cargo:rerun-if-changed={}", assets.display()); + + let mut entries: Vec<(String, std::path::PathBuf)> = Vec::new(); + if let Ok(packages) = std::fs::read_dir(&assets) { + for pkg in packages.flatten() { + let pkg_path = pkg.path(); + if !pkg_path.is_dir() { + continue; + } + let Some(pkg_name) = pkg_path.file_name().and_then(|n| n.to_str()) else { + continue; + }; + let msg_dir = pkg_path.join("msg"); + let Ok(msgs) = std::fs::read_dir(&msg_dir) else { + continue; + }; + for m in msgs.flatten() { + let path = m.path(); + if path.extension().and_then(|e| e.to_str()) != Some("msg") { + continue; + } + let Some(stem) = path.file_stem().and_then(|n| n.to_str()) else { + continue; + }; + entries.push((format!("{pkg_name}/msg/{stem}"), path)); + } + } + } else { + println!( + "cargo:warning=bundled assets not found at {}; embedded schema fallback will be empty", + assets.display() + ); + } + + // Sorted, so the lookup can binary-search. + entries.sort_by(|a, b| a.0.cmp(&b.0)); + + let mut out = String::from( + "// @generated by build.rs - do not edit\npub static EMBEDDED_MSGS: &[(&str, &str)] = &[\n", + ); + for (type_name, path) in &entries { + writeln!( + out, + " ({type_name:?}, include_str!({:?})),", + path.display().to_string() + ) + .unwrap(); + } + out.push_str("];\n"); + + let dest = std::path::PathBuf::from(std::env::var("OUT_DIR").unwrap()).join("embedded_msgs.rs"); + if let Err(e) = std::fs::write(&dest, out) { + println!("cargo:warning=failed to write {}: {e}", dest.display()); + } +} diff --git a/crates/hiroz/src/dynamic/registry.rs b/crates/hiroz/src/dynamic/registry.rs index 050563367..0489aa0dc 100644 --- a/crates/hiroz/src/dynamic/registry.rs +++ b/crates/hiroz/src/dynamic/registry.rs @@ -220,15 +220,30 @@ fn load_schema_inner( // File not on disk is a legitimate "try the next source" (live discovery), // so return None quietly. Errors *after* a file is found are logged below, // since a broken `.msg` masquerading as "not found" would be misleading. - let path = find_msg_file(&package, &name)?; + // Disk first, so a user pointing HIROZ_MSG_PATH at their own definitions + // always wins over what this binary happens to have been built with. + let source = match find_msg_file(&package, &name) { + Some(path) => MsgSource::File(path), + None => MsgSource::Embedded(embedded_msg_source(&package, &name)?), + }; if !in_progress.borrow_mut().insert(type_name.to_string()) { tracing::warn!("cyclic .msg definition for {type_name}; skipping schema load"); return None; } - let mut parsed = match hiroz_codegen::parser::msg::parse_msg_file(&path, &package) { + let parse_result = match &source { + MsgSource::File(path) => hiroz_codegen::parser::msg::parse_msg_file(path, &package), + // The path argument is only used for diagnostics; the bytes come from + // the embedded table. + MsgSource::Embedded(text) => hiroz_codegen::parser::msg::parse_msg_string( + text, + &package, + std::path::Path::new(&format!("/{package}/msg/{name}.msg")), + ), + }; + let mut parsed = match parse_result { Ok(parsed) => parsed, Err(e) => { - tracing::warn!("failed to parse .msg file {}: {e}", path.display()); + tracing::warn!("failed to parse .msg for {type_name} from {source}: {e}"); in_progress.borrow_mut().remove(type_name); return None; } @@ -261,6 +276,49 @@ fn load_schema_inner( Some(register_schema(schema)) } +/// Where a `.msg` definition came from, for diagnostics. +#[cfg(feature = "dynamic-schema-loader")] +enum MsgSource { + File(std::path::PathBuf), + Embedded(&'static str), +} + +#[cfg(feature = "dynamic-schema-loader")] +impl std::fmt::Display for MsgSource { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + MsgSource::File(p) => write!(f, "{}", p.display()), + MsgSource::Embedded(_) => write!(f, "the definitions built into this binary"), + } + } +} + +/// The bundled `.msg` definitions, embedded as source text at build time. +/// +/// Sorted by `pkg/msg/Name`, so the lookup below can binary-search. +#[cfg(feature = "dynamic-schema-loader")] +mod embedded { + include!(concat!(env!("OUT_DIR"), "/embedded_msgs.rs")); +} + +/// Look up `/msg/` among the definitions built into this binary. +/// +/// This is what lets a downloaded `hu` decode a topic with no `HIROZ_MSG_PATH` +/// set and no reachable type-description service — the case where discovery +/// yields a type name and the disk has nothing to resolve it with. +/// +/// Consulted **after** `HIROZ_MSG_PATH`, never before: a user who points that +/// variable at their own definitions means it, and a stale embedded copy must +/// not silently win over the messages their publisher was actually built from. +#[cfg(feature = "dynamic-schema-loader")] +fn embedded_msg_source(package: &str, name: &str) -> Option<&'static str> { + let key = format!("{package}/msg/{name}"); + embedded::EMBEDDED_MSGS + .binary_search_by(|(k, _)| (*k).cmp(key.as_str())) + .ok() + .map(|i| embedded::EMBEDDED_MSGS[i].1) +} + /// Split `pkg/msg/Name` (or the shorthand `pkg/Name`) into `(package, name)`. #[cfg(feature = "dynamic-schema-loader")] fn split_msg_type(type_name: &str) -> Option<(String, String)> { @@ -304,3 +362,81 @@ fn find_msg_file(package: &str, name: &str) -> Option { } None } + +#[cfg(all(test, feature = "dynamic-schema-loader"))] +mod embedded_tests { + use super::*; + + /// The table is what makes a downloaded `hu` able to decode anything, so an + /// empty one is a silent regression: every lookup would simply miss and the + /// behaviour would fall back to today's "no .msg found". + #[test] + fn the_embedded_table_is_not_empty_and_is_sorted() { + assert!( + !embedded::EMBEDDED_MSGS.is_empty(), + "no bundled .msg definitions were embedded; \ + the build script found no assets directory" + ); + assert!( + embedded::EMBEDDED_MSGS.windows(2).all(|w| w[0].0 < w[1].0), + "the embedded table must be sorted and duplicate-free: binary_search relies on it" + ); + } + + /// std_msgs/msg/String is the type the documented quick start echoes, and + /// the one the G2 measurement showed failing on a default install. + #[test] + fn a_common_type_resolves_from_the_embedded_definitions() { + let src = embedded_msg_source("std_msgs", "String") + .expect("std_msgs/msg/String must be embedded"); + assert!( + src.contains("string data"), + "embedded source does not look like the real definition: {src:?}" + ); + } + + #[test] + fn an_unknown_type_is_a_miss_not_a_panic() { + assert!(embedded_msg_source("no_such_pkg", "Nope").is_none()); + } + + /// Disk must win over the embedded copy. A user who sets HIROZ_MSG_PATH + /// means it, and their publisher may have been built from definitions that + /// differ from the ones this binary was compiled with. + #[test] + #[serial_test::serial] + fn a_definition_on_disk_wins_over_the_embedded_one() { + let dir = std::env::temp_dir().join(format!("hiroz-embed-{}", std::process::id())); + let msg_dir = dir.join("std_msgs").join("msg"); + std::fs::create_dir_all(&msg_dir).unwrap(); + // Deliberately NOT the real definition, so resolving it proves the disk + // copy was used rather than the embedded one. + std::fs::write( + msg_dir.join("String.msg"), + "string data\nint32 sentinel_field\n", + ) + .unwrap(); + + let found = find_msg_file("std_msgs", "String"); + let restore = std::env::var("HIROZ_MSG_PATH").ok(); + assert!( + found.is_none() || restore.is_some(), + "test environment already has HIROZ_MSG_PATH pointing somewhere" + ); + + unsafe { std::env::set_var("HIROZ_MSG_PATH", &dir) }; + let path = find_msg_file("std_msgs", "String") + .expect("the on-disk definition must be found first"); + let text = std::fs::read_to_string(&path).unwrap(); + assert!( + text.contains("sentinel_field"), + "HIROZ_MSG_PATH did not take precedence over the embedded table" + ); + + match restore { + Some(v) => unsafe { std::env::set_var("HIROZ_MSG_PATH", v) }, + None => unsafe { std::env::remove_var("HIROZ_MSG_PATH") }, + } + let _ = std::fs::remove_dir_all(&dir); + } +}