From b7f7a55b4262c1da91a659dcf0d8915948b27791 Mon Sep 17 00:00:00 2001 From: Clement Rey Date: Tue, 19 Dec 2023 19:50:06 +0100 Subject: [PATCH] `DataLoader`s 6: first-class support for `NotSupported` (#4565) Introduce a first-class protocol for `DataLoader`s -- whether they are builtin, custom, or external -- to announce that they don't support loading a given piece of data. This fixes one of the oldest issue when loading files in Rerun: loading unsupported data would always bottom out in either the image or websocket paths, leading to unrelated errors very confusing to users. The loading process is now a two-pass process: 1. Dispatch the data to be loaded to all loaders, which will respond ASAP whether they can do it or not. 1.1. If at least one compatible loader is found, go to 2 1.2. Otherwise, fail early with a nice error message for the end user 2. Dispatch the actual data loading. This has important/subtle ramifications on the threading model, but other than that is what you'd expect. Checks: - [x] Native: CLI examples/assets/* - [x] Native: CLI examples/assets - [x] Native: CLI examples/assets/* containing unsupported files - [x] Native: CLI examples/assets containing unsupported files - [x] Native: File>Open examples/assets/* - [x] Native: File>Open examples/assets - [x] Native: File>Open examples/assets/* containing unsupported files - [x] Native: File>Open examples/assets containing unsupported files - [x] Native: Drag-n-drop examples/assets/* - [x] Native: Drag-n-drop examples/assets - [x] Native: Drag-n-drop examples/assets/* containing unsupported files - [x] Native: Drag-n-drop examples/assets containing unsupported files - [x] Web: File>Open examples/assets/* - [x] Web: Drag-n-drop examples/assets/* - [x] Web: File>Open examples/assets/* containing unsupported files - [x] Web: Drag-n-drop examples/assets/* containing unsupported files --- Part of a series of PRs to make it possible to load _any_ file from the local filesystem, by any means, on web and native: - #4516 - #4517 - #4518 - #4519 - #4520 - #4521 - #4565 - #4566 - #4567 --- Cargo.lock | 1 + .../src/data_loader/loader_archetype.rs | 9 +- .../src/data_loader/loader_directory.rs | 40 +++-- .../src/data_loader/loader_external.rs | 59 ++++++- .../src/data_loader/loader_rrd.rs | 18 ++- crates/re_data_source/src/data_loader/mod.rs | 17 +- crates/re_data_source/src/lib.rs | 6 +- crates/re_data_source/src/load_file.rs | 152 ++++++++++++------ crates/rerun/src/lib.rs | 4 + examples/cpp/external_data_loader/main.cpp | 7 +- examples/python/external_data_loader/main.py | 7 +- examples/rust/external_data_loader/Cargo.toml | 1 + .../rust/external_data_loader/src/main.rs | 22 +-- rerun_cpp/src/rerun.hpp | 5 + rerun_py/rerun_sdk/rerun/__init__.py | 9 ++ 15 files changed, 256 insertions(+), 101 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3e4c59ddaf6a..d59adb46abc2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5517,6 +5517,7 @@ dependencies = [ name = "rerun-loader-rust-file" version = "0.12.0-alpha.1+dev" dependencies = [ + "anyhow", "argh", "rerun", ] diff --git a/crates/re_data_source/src/data_loader/loader_archetype.rs b/crates/re_data_source/src/data_loader/loader_archetype.rs index f903e78b9dff..8c4a92276a86 100644 --- a/crates/re_data_source/src/data_loader/loader_archetype.rs +++ b/crates/re_data_source/src/data_loader/loader_archetype.rs @@ -26,7 +26,7 @@ impl DataLoader for ArchetypeLoader { use anyhow::Context as _; if filepath.is_dir() { - return Ok(()); // simply not interested + return Err(crate::DataLoaderError::Incompatible(filepath.clone())); } re_tracing::profile_function!(filepath.display().to_string()); @@ -45,6 +45,11 @@ impl DataLoader for ArchetypeLoader { contents: std::borrow::Cow<'_, [u8]>, tx: std::sync::mpsc::Sender, ) -> Result<(), crate::DataLoaderError> { + let extension = crate::extension(&filepath); + if !crate::is_supported_file_extension(&extension) { + return Err(crate::DataLoaderError::Incompatible(filepath.clone())); + } + re_tracing::profile_function!(filepath.display().to_string()); let entity_path = EntityPath::from_file_path(&filepath); @@ -76,8 +81,6 @@ impl DataLoader for ArchetypeLoader { } } - let extension = crate::extension(&filepath); - let mut rows = Vec::new(); if crate::SUPPORTED_IMAGE_EXTENSIONS.contains(&extension.as_str()) { diff --git a/crates/re_data_source/src/data_loader/loader_directory.rs b/crates/re_data_source/src/data_loader/loader_directory.rs index bcca69919278..1e09e6854ee0 100644 --- a/crates/re_data_source/src/data_loader/loader_directory.rs +++ b/crates/re_data_source/src/data_loader/loader_directory.rs @@ -21,7 +21,7 @@ impl crate::DataLoader for DirectoryLoader { tx: std::sync::mpsc::Sender, ) -> Result<(), crate::DataLoaderError> { if dirpath.is_file() { - return Ok(()); // simply not interested + return Err(crate::DataLoaderError::Incompatible(dirpath.clone())); } re_tracing::profile_function!(dirpath.display().to_string()); @@ -43,22 +43,28 @@ impl crate::DataLoader for DirectoryLoader { let filepath = filepath.to_owned(); let tx = tx.clone(); - // NOTE: spawn is fine, this whole function is native-only. - rayon::spawn(move || { - let data = match crate::load_file::load(&store_id, &filepath, false, None) { - Ok(data) => data, - Err(err) => { - re_log::error!(?filepath, %err, "Failed to load directory entry"); - return; - } - }; + // NOTE(1): `spawn` is fine, this whole function is native-only. + // NOTE(2): this must spawned on a dedicated thread to avoid a deadlock! + // `load` will spawn a bunch of loaders on the common rayon thread pool and wait for + // their response via channels: we cannot be waiting for these responses on the + // common rayon thread pool. + _ = std::thread::Builder::new() + .name(format!("load_dir_entry({filepath:?})")) + .spawn(move || { + let data = match crate::load_file::load(&store_id, &filepath, None) { + Ok(data) => data, + Err(err) => { + re_log::error!(?filepath, %err, "Failed to load directory entry"); + return; + } + }; - for datum in data { - if tx.send(datum).is_err() { - break; + for datum in data { + if tx.send(datum).is_err() { + break; + } } - } - }); + }); } } @@ -69,11 +75,11 @@ impl crate::DataLoader for DirectoryLoader { fn load_from_file_contents( &self, _store_id: re_log_types::StoreId, - _path: std::path::PathBuf, + path: std::path::PathBuf, _contents: std::borrow::Cow<'_, [u8]>, _tx: std::sync::mpsc::Sender, ) -> Result<(), crate::DataLoaderError> { // TODO(cmc): This could make sense to implement for e.g. archive formats (zip, tar, …) - Ok(()) // simply not interested + Err(crate::DataLoaderError::Incompatible(path)) } } diff --git a/crates/re_data_source/src/data_loader/loader_external.rs b/crates/re_data_source/src/data_loader/loader_external.rs index 715164b8de00..a8aa304eeb64 100644 --- a/crates/re_data_source/src/data_loader/loader_external.rs +++ b/crates/re_data_source/src/data_loader/loader_external.rs @@ -2,10 +2,17 @@ use std::io::Read; use once_cell::sync::Lazy; +// --- + /// To register a new external data loader, simply add an executable in your $PATH whose name /// starts with this prefix. pub const EXTERNAL_DATA_LOADER_PREFIX: &str = "rerun-loader-"; +/// When an external [`crate::DataLoader`] is asked to load some data that it doesn't know +/// how to load, it should exit with this exit code. +// NOTE: Always keep in sync with other languages. +pub const EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE: i32 = 66; + /// Keeps track of the paths all external executable [`crate::DataLoader`]s. /// /// Lazy initialized the first time a file is opened by running a full scan of the `$PATH`. @@ -78,14 +85,18 @@ impl crate::DataLoader for ExternalLoader { re_tracing::profile_function!(filepath.display().to_string()); + #[derive(PartialEq, Eq)] + struct CompatibleLoaderFound; + let (tx_feedback, rx_feedback) = std::sync::mpsc::channel::(); + for exe in EXTERNAL_LOADER_PATHS.iter() { let store_id = store_id.clone(); let filepath = filepath.clone(); let tx = tx.clone(); + let tx_feedback = tx_feedback.clone(); - // NOTE: spawn is fine, the entire loader is native-only. rayon::spawn(move || { - re_tracing::profile_function!(); + re_tracing::profile_function!(exe.to_string_lossy()); let child = Command::new(exe) .arg(filepath.clone()) @@ -119,14 +130,28 @@ impl crate::DataLoader for ExternalLoader { let stdout = std::io::BufReader::new(stdout); match re_log_encoding::decoder::Decoder::new(version_policy, stdout) { Ok(decoder) => { - decode_and_stream(&filepath, &tx, decoder); + let filepath = filepath.clone(); + let tx = tx.clone(); + // NOTE: This is completely IO bound, it must run on a dedicated thread, not the shared + // rayon thread pool. + if let Err(err) = std::thread::Builder::new() + .name(format!("decode_and_stream({filepath:?})")) + .spawn({ + let filepath = filepath.clone(); + move || { + decode_and_stream(&filepath, &tx, decoder); + } + }) + { + re_log::error!(?filepath, loader = ?exe, %err, "Failed to open spawn IO thread"); + return; + } } Err(re_log_encoding::decoder::DecodeError::Read(_)) => { // The child was not interested in that file and left without logging // anything. // That's fine, we just need to make sure to check its exit status further // down, still. - return; } Err(err) => { re_log::error!(?filepath, loader = ?exe, %err, "Failed to decode external loader's output"); @@ -142,15 +167,35 @@ impl crate::DataLoader for ExternalLoader { } }; - if !status.success() { + // NOTE: We assume that plugins are compatible until proven otherwise. + let is_compatible = + status.code() != Some(crate::EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE); + + if is_compatible && !status.success() { let mut stderr = std::io::BufReader::new(stderr); let mut reason = String::new(); stderr.read_to_string(&mut reason).ok(); re_log::error!(?filepath, loader = ?exe, %reason, "Failed to execute external loader"); } + + if is_compatible { + re_log::debug!(loader = ?exe, ?filepath, "compatible external loader found"); + tx_feedback.send(CompatibleLoaderFound).ok(); + } }); } + re_tracing::profile_wait!("compatible_loader"); + + drop(tx_feedback); + + let any_compatible_loader = rx_feedback.recv() == Ok(CompatibleLoaderFound); + if !any_compatible_loader { + // NOTE: The only way to get here is if all loaders closed then sending end of the + // channel without sending anything, i.e. none of them are compatible. + return Err(crate::DataLoaderError::Incompatible(filepath.clone())); + } + Ok(()) } @@ -158,13 +203,13 @@ impl crate::DataLoader for ExternalLoader { fn load_from_file_contents( &self, _store_id: re_log_types::StoreId, - _path: std::path::PathBuf, + path: std::path::PathBuf, _contents: std::borrow::Cow<'_, [u8]>, _tx: std::sync::mpsc::Sender, ) -> Result<(), crate::DataLoaderError> { // TODO(cmc): You could imagine a world where plugins can be streamed rrd data via their // standard input… but today is not world. - Ok(()) // simply not interested + Err(crate::DataLoaderError::Incompatible(path)) } } diff --git a/crates/re_data_source/src/data_loader/loader_rrd.rs b/crates/re_data_source/src/data_loader/loader_rrd.rs index f04b487638a5..a40f46850963 100644 --- a/crates/re_data_source/src/data_loader/loader_rrd.rs +++ b/crates/re_data_source/src/data_loader/loader_rrd.rs @@ -25,7 +25,7 @@ impl crate::DataLoader for RrdLoader { let extension = crate::extension(&filepath); if extension != "rrd" { - return Ok(()); // simply not interested + return Err(crate::DataLoaderError::Incompatible(filepath.clone())); } re_log::debug!( @@ -40,7 +40,17 @@ impl crate::DataLoader for RrdLoader { let file = std::io::BufReader::new(file); let decoder = re_log_encoding::decoder::Decoder::new(version_policy, file)?; - decode_and_stream(&filepath, &tx, decoder); + + // NOTE: This is IO bound, it must run on a dedicated thread, not the shared rayon thread pool. + std::thread::Builder::new() + .name(format!("decode_and_stream({filepath:?})")) + .spawn({ + let filepath = filepath.clone(); + move || { + decode_and_stream(&filepath, &tx, decoder); + } + }) + .with_context(|| format!("Failed to open spawn IO thread for {filepath:?}"))?; Ok(()) } @@ -57,7 +67,7 @@ impl crate::DataLoader for RrdLoader { let extension = crate::extension(&filepath); if extension != "rrd" { - return Ok(()); // simply not interested + return Err(crate::DataLoaderError::Incompatible(filepath)); } let version_policy = re_log_encoding::decoder::VersionPolicy::Warn; @@ -71,7 +81,9 @@ impl crate::DataLoader for RrdLoader { _ => return Err(err.into()), }, }; + decode_and_stream(&filepath, &tx, decoder); + Ok(()) } } diff --git a/crates/re_data_source/src/data_loader/mod.rs b/crates/re_data_source/src/data_loader/mod.rs index 5d7c239488fd..469e22b26181 100644 --- a/crates/re_data_source/src/data_loader/mod.rs +++ b/crates/re_data_source/src/data_loader/mod.rs @@ -31,6 +31,10 @@ use re_log_types::{ArrowMsg, DataRow, LogMsg}; /// - [`DirectoryLoader`] for recursively loading folders. /// - [`ExternalLoader`], which looks for user-defined data loaders in $PATH. /// +/// ## Registering custom loaders +/// +/// TODO(cmc): web guide in upcoming PR +/// /// ## Execution /// /// **All** registered [`DataLoader`]s get called when a user tries to open a file, unconditionally. @@ -131,6 +135,9 @@ pub enum DataLoaderError { #[error(transparent)] Decode(#[from] re_log_encoding::decoder::DecodeError), + #[error("No data-loader support for {0:?}")] + Incompatible(std::path::PathBuf), + #[error(transparent)] Other(#[from] anyhow::Error), } @@ -144,6 +151,11 @@ impl DataLoaderError { _ => false, } } + + #[inline] + pub fn is_incompatible(&self) -> bool { + matches!(self, Self::Incompatible { .. }) + } } /// What [`DataLoader`]s load. @@ -234,9 +246,8 @@ pub use self::loader_archetype::ArchetypeLoader; pub use self::loader_directory::DirectoryLoader; pub use self::loader_rrd::RrdLoader; -#[cfg(not(target_arch = "wasm32"))] -pub(crate) use self::loader_external::EXTERNAL_LOADER_PATHS; #[cfg(not(target_arch = "wasm32"))] pub use self::loader_external::{ - iter_external_loaders, ExternalLoader, EXTERNAL_DATA_LOADER_PREFIX, + iter_external_loaders, ExternalLoader, EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE, + EXTERNAL_DATA_LOADER_PREFIX, }; diff --git a/crates/re_data_source/src/lib.rs b/crates/re_data_source/src/lib.rs index 3cfa30a8dcf8..111072cb6336 100644 --- a/crates/re_data_source/src/lib.rs +++ b/crates/re_data_source/src/lib.rs @@ -23,7 +23,10 @@ pub use self::load_file::{extension, load_from_file_contents}; pub use self::web_sockets::connect_to_ws_url; #[cfg(not(target_arch = "wasm32"))] -pub use self::data_loader::{iter_external_loaders, ExternalLoader}; +pub use self::data_loader::{ + iter_external_loaders, ExternalLoader, EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE, + EXTERNAL_DATA_LOADER_PREFIX, +}; #[cfg(not(target_arch = "wasm32"))] pub use self::load_file::load_from_path; @@ -63,6 +66,7 @@ pub fn supported_extensions() -> impl Iterator { .iter() .chain(SUPPORTED_IMAGE_EXTENSIONS) .chain(SUPPORTED_MESH_EXTENSIONS) + .chain(SUPPORTED_POINT_CLOUD_EXTENSIONS) .chain(SUPPORTED_TEXT_EXTENSIONS) .copied() } diff --git a/crates/re_data_source/src/load_file.rs b/crates/re_data_source/src/load_file.rs index 1c0e7e2736bb..6ac771e76c42 100644 --- a/crates/re_data_source/src/load_file.rs +++ b/crates/re_data_source/src/load_file.rs @@ -1,5 +1,4 @@ use std::borrow::Cow; -use std::sync::Arc; use re_log_types::{FileSource, LogMsg}; use re_smart_channel::Sender; @@ -42,7 +41,7 @@ pub fn load_from_path( } } - let data = load(store_id, path, path.is_dir(), None)?; + let data = load(store_id, path, None)?; send(store_id, data, tx); Ok(()) @@ -75,7 +74,7 @@ pub fn load_from_file_contents( } } - let data = load(store_id, filepath, false, Some(contents))?; + let data = load(store_id, filepath, Some(contents))?; send(store_id, data, tx); Ok(()) @@ -135,52 +134,46 @@ pub(crate) fn prepare_store_info( /// Loads the data at `path` using all available [`crate::DataLoader`]s. /// -/// Returns a channel with all the [`LoadedData`]: +/// On success, returns a channel with all the [`LoadedData`]: /// - On native, this is filled asynchronously from other threads. /// - On wasm, this is pre-filled synchronously. -#[cfg_attr(target_arch = "wasm32", allow(clippy::needless_pass_by_value))] +/// +/// There is only one way this function can return an error: not a single [`crate::DataLoader`] +/// (whether it is builtin, custom or external) was capable of loading the data, in which case +/// [`DataLoaderError::Incompatible`] will be returned. +#[cfg(not(target_arch = "wasm32"))] pub(crate) fn load( store_id: &re_log_types::StoreId, path: &std::path::Path, - is_dir: bool, contents: Option>, ) -> Result, DataLoaderError> { - #[cfg(target_arch = "wasm32")] - let has_external_loaders = false; - #[cfg(not(target_arch = "wasm32"))] - let has_external_loaders = !crate::data_loader::EXTERNAL_LOADER_PATHS.is_empty(); - - let extension = extension(path); - let is_builtin = is_associated_with_builtin_loader(path, is_dir); - - // If there are no external loaders registered (which is always the case on wasm) and we don't - // have a builtin loader for it, then we know for a fact that we won't be able to load it. - if !is_builtin && !has_external_loaders { - return if extension.is_empty() { - Err(anyhow::anyhow!("files without extensions (file.XXX) are not supported").into()) - } else { - Err(anyhow::anyhow!(".{extension} files are not supported").into()) - }; - } + re_tracing::profile_function!(path.display().to_string()); // On native we run loaders in parallel so this needs to become static. - #[cfg(not(target_arch = "wasm32"))] - let contents: Option>> = - contents.map(|contents| Arc::new(Cow::Owned(contents.into_owned()))); + let contents: Option>> = + contents.map(|contents| std::sync::Arc::new(Cow::Owned(contents.into_owned()))); let rx_loader = { let (tx_loader, rx_loader) = std::sync::mpsc::channel(); - for loader in crate::iter_loaders() { - let loader = Arc::clone(&loader); - let store_id = store_id.clone(); - let tx_loader = tx_loader.clone(); - let path = path.to_owned(); + let any_compatible_loader = { + #[derive(PartialEq, Eq)] + struct CompatibleLoaderFound; + let (tx_feedback, rx_feedback) = std::sync::mpsc::channel::(); + + for loader in crate::iter_loaders() { + let loader = std::sync::Arc::clone(&loader); - #[cfg(not(target_arch = "wasm32"))] - spawn({ + let store_id = store_id.clone(); + let path = path.to_owned(); let contents = contents.clone(); // arc - move || { + + let tx_loader = tx_loader.clone(); + let tx_feedback = tx_feedback.clone(); + + rayon::spawn(move || { + re_tracing::profile_scope!("inner", loader.name()); + if let Some(contents) = contents.as_deref() { let contents = Cow::Borrowed(contents.as_ref()); @@ -190,36 +183,95 @@ pub(crate) fn load( contents, tx_loader, ) { - re_log::error!(?path, loader = loader.name(), %err, "Failed to load data from file"); + if err.is_incompatible() { + return; + } + re_log::error!(?path, loader = loader.name(), %err, "Failed to load data"); } } else if let Err(err) = loader.load_from_path(store_id, path.clone(), tx_loader) { + if err.is_incompatible() { + return; + } re_log::error!(?path, loader = loader.name(), %err, "Failed to load data from file"); } - } - }); - #[cfg(target_arch = "wasm32")] - spawn(|| { - if let Some(contents) = contents.as_deref() { - let contents = Cow::Borrowed(contents); + re_log::debug!(loader = loader.name(), ?path, "compatible loader found"); + tx_feedback.send(CompatibleLoaderFound).ok(); + }); + } + + re_tracing::profile_wait!("compatible_loader"); - if let Err(err) = - loader.load_from_file_contents(store_id, path.clone(), contents, tx_loader) - { - re_log::error!(?path, loader = loader.name(), %err, "Failed to load data from file"); + drop(tx_feedback); + + rx_feedback.recv() == Ok(CompatibleLoaderFound) + }; + + // Implicitly closing `tx_loader`! + + any_compatible_loader.then_some(rx_loader) + }; + + if let Some(rx_loader) = rx_loader { + Ok(rx_loader) + } else { + Err(DataLoaderError::Incompatible(path.to_owned())) + } +} + +/// Loads the data at `path` using all available [`crate::DataLoader`]s. +/// +/// On success, returns a channel (pre-filled synchronously) with all the [`LoadedData`]. +/// +/// There is only one way this function can return an error: not a single [`crate::DataLoader`] +/// (whether it is builtin, custom or external) was capable of loading the data, in which case +/// [`DataLoaderError::Incompatible`] will be returned. +#[cfg(target_arch = "wasm32")] +#[allow(clippy::needless_pass_by_value)] +pub(crate) fn load( + store_id: &re_log_types::StoreId, + path: &std::path::Path, + contents: Option>, +) -> Result, DataLoaderError> { + re_tracing::profile_function!(path.display().to_string()); + + let rx_loader = { + let (tx_loader, rx_loader) = std::sync::mpsc::channel(); + + let any_compatible_loader = crate::iter_loaders().map(|loader| { + if let Some(contents) = contents.as_deref() { + let store_id = store_id.clone(); + let tx_loader = tx_loader.clone(); + let path = path.to_owned(); + let contents = Cow::Borrowed(contents); + + if let Err(err) = loader.load_from_file_contents(store_id, path.clone(), contents, tx_loader) { + if err.is_incompatible() { + return false; } + re_log::error!(?path, loader = loader.name(), %err, "Failed to load data from file"); } - }); - } + + true + } else { + false + } + }) + .reduce(|any_compatible, is_compatible| any_compatible || is_compatible) + .unwrap_or(false); // Implicitly closing `tx_loader`! - rx_loader + any_compatible_loader.then_some(rx_loader) }; - Ok(rx_loader) + if let Some(rx_loader) = rx_loader { + Ok(rx_loader) + } else { + Err(DataLoaderError::Incompatible(path.to_owned())) + } } /// Forwards the data in `rx_loader` to `tx`, taking care of necessary conversions, if any. @@ -231,6 +283,8 @@ pub(crate) fn send( tx: &Sender, ) { spawn({ + re_tracing::profile_function!(); + let tx = tx.clone(); let store_id = store_id.clone(); move || { diff --git a/crates/rerun/src/lib.rs b/crates/rerun/src/lib.rs index 99c2244b608c..ac0e51421e9a 100644 --- a/crates/rerun/src/lib.rs +++ b/crates/rerun/src/lib.rs @@ -137,6 +137,10 @@ pub use re_data_store::external::re_arrow_store::{ DataStore, StoreDiff, StoreDiffKind, StoreEvent, StoreGeneration, StoreSubscriber, }; +pub use re_data_source::{ + EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE, EXTERNAL_DATA_LOADER_PREFIX, +}; + /// Re-exports of other crates. pub mod external { pub use anyhow; diff --git a/examples/cpp/external_data_loader/main.cpp b/examples/cpp/external_data_loader/main.cpp index bffcf5f7af49..24b538a0bf71 100644 --- a/examples/cpp/external_data_loader/main.cpp +++ b/examples/cpp/external_data_loader/main.cpp @@ -59,10 +59,9 @@ int main(int argc, char* argv[]) { bool is_file = std::filesystem::is_regular_file(filepath); bool is_cpp_file = std::filesystem::path(filepath).extension().string() == ".cpp"; - // We're not interested: just exit silently. - // Don't return an error, as that would show up to the end user in the Rerun Viewer! - if (!(is_file && is_cpp_file)) { - return 0; + // Inform the Rerun Viewer that we do not support that kind of file. + if (!is_file || is_cpp_file) { + return rerun::EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE; } std::ifstream file(filepath); diff --git a/examples/python/external_data_loader/main.py b/examples/python/external_data_loader/main.py index 19363fe09529..1ddb5d7d78bc 100755 --- a/examples/python/external_data_loader/main.py +++ b/examples/python/external_data_loader/main.py @@ -31,10 +31,9 @@ def main() -> None: is_file = os.path.isfile(args.filepath) is_python_file = os.path.splitext(args.filepath)[1].lower() == ".py" - # We're not interested: just exit silently. - # Don't return an error, as that would show up to the end user in the Rerun Viewer! - if not (is_file and is_python_file): - return + # Inform the Rerun Viewer that we do not support that kind of file. + if not is_file or not is_python_file: + exit(rr.EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE) rr.init("rerun_example_external_data_loader", recording_id=args.recording_id) # The most important part of this: log to standard output so the Rerun Viewer can ingest it! diff --git a/examples/rust/external_data_loader/Cargo.toml b/examples/rust/external_data_loader/Cargo.toml index a03f442605fc..f78c60312b43 100644 --- a/examples/rust/external_data_loader/Cargo.toml +++ b/examples/rust/external_data_loader/Cargo.toml @@ -9,4 +9,5 @@ publish = false [dependencies] rerun = { path = "../../../crates/rerun" } +anyhow = "1.0" argh = "0.1" diff --git a/examples/rust/external_data_loader/src/main.rs b/examples/rust/external_data_loader/src/main.rs index 6f04184cd25a..2afebc073440 100644 --- a/examples/rust/external_data_loader/src/main.rs +++ b/examples/rust/external_data_loader/src/main.rs @@ -1,6 +1,8 @@ //! Example of an external data-loader executable plugin for the Rerun Viewer. -use rerun::{external::re_data_source::extension, MediaType}; +use rerun::{ + external::re_data_source::extension, MediaType, EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE, +}; // The Rerun Viewer will always pass these two pieces of information: // 1. The path to be loaded, as a positional arg. @@ -25,18 +27,21 @@ struct Args { recording_id: Option, } -fn main() -> Result<(), Box> { +fn main() -> anyhow::Result<()> { let args: Args = argh::from_env(); let is_file = args.filepath.is_file(); let is_rust_file = extension(&args.filepath) == "rs"; - // We're not interested: just exit silently. - // Don't return an error, as that would show up to the end user in the Rerun Viewer! - if !(is_file && is_rust_file) { - return Ok(()); + // Inform the Rerun Viewer that we do not support that kind of file. + if !is_file || !is_rust_file { + #[allow(clippy::exit)] + std::process::exit(EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE); } + let body = std::fs::read_to_string(&args.filepath)?; + let text = format!("## Some Rust code\n```rust\n{body}\n```\n"); + let rec = { let mut rec = rerun::RecordingStreamBuilder::new("rerun_example_external_data_loader"); if let Some(recording_id) = args.recording_id { @@ -47,13 +52,10 @@ fn main() -> Result<(), Box> { rec.stdout()? }; - let body = std::fs::read_to_string(&args.filepath)?; - let text = format!("## Some Rust code\n```rust\n{body}\n```\n"); - rec.log_timeless( rerun::EntityPath::from_file_path(&args.filepath), &rerun::TextDocument::new(text).with_media_type(MediaType::MARKDOWN), )?; - Ok(()) + Ok::<_, anyhow::Error>(()) } diff --git a/rerun_cpp/src/rerun.hpp b/rerun_cpp/src/rerun.hpp index d5c2da90ecff..7ebd2635ffbc 100644 --- a/rerun_cpp/src/rerun.hpp +++ b/rerun_cpp/src/rerun.hpp @@ -18,6 +18,11 @@ /// All Rerun C++ types and functions are in the `rerun` namespace or one of its nested namespaces. namespace rerun { + /// When an external [`DataLoader`] is asked to load some data that it doesn't know how to load, it + /// should exit with this exit code. + // NOTE: Always keep in sync with other languages. + const int EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE = 66; + // Archetypes are the quick-and-easy default way of logging data to Rerun. // Make them available in the rerun namespace. using namespace archetypes; diff --git a/rerun_py/rerun_sdk/rerun/__init__.py b/rerun_py/rerun_sdk/rerun/__init__.py index c055123c6613..c6244024af2e 100644 --- a/rerun_py/rerun_sdk/rerun/__init__.py +++ b/rerun_py/rerun_sdk/rerun/__init__.py @@ -170,6 +170,7 @@ # UTILITIES __all__ += [ + "EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE", "cleanup_if_forked_child", "init", "new_recording", @@ -182,6 +183,14 @@ ] +# NOTE: Always keep in sync with other languages. +EXTERNAL_DATA_LOADER_INCOMPATIBLE_EXIT_CODE = 66 +""" +When an external `DataLoader` is asked to load some data that it doesn't know how to load, it +should exit with this exit code. +""" + + def _init_recording_stream() -> None: # Inject all relevant methods into the `RecordingStream` class. # We need to do this from here to avoid circular import issues.