From ad26453e468035191484ca923378944b0869f9c5 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 16 Oct 2023 15:16:54 +0200 Subject: [PATCH 1/2] Replace `--strict` flag with `RERUN_PANIC_ON_WARN` env-var `--strict` was an ill-chosen name, as it clashes with the SDK-side strict-modes for user errors. By making it an environment variable it is now also works anywhere `setup_native_logging` is called. --- crates/re_log/src/setup.rs | 45 ++++++++++++++++++++++++++++++++ crates/rerun/src/run.rs | 43 +----------------------------- docs/code-examples/roundtrips.py | 3 +++ scripts/run_python_e2e_test.py | 15 ++++++----- tests/roundtrips.py | 3 +++ 5 files changed, 60 insertions(+), 49 deletions(-) diff --git a/crates/re_log/src/setup.rs b/crates/re_log/src/setup.rs index 310ce08d6d30..ca54803f029c 100644 --- a/crates/re_log/src/setup.rs +++ b/crates/re_log/src/setup.rs @@ -49,6 +49,26 @@ pub fn setup_native_logging() { let mut stderr_logger = env_logger::Builder::new(); stderr_logger.parse_filters(&log_filter); crate::add_boxed_logger(Box::new(stderr_logger.build())).expect("Failed to install logger"); + + if env_var_bool("RERUN_PANIC_ON_WARN") == Some(true) { + crate::add_boxed_logger(Box::new(PanicOnWarn {})) + .expect("Failed to enable RERUN_PANIC_ON_WARN"); + crate::info!("RERUN_PANIC_ON_WARN: any warning or error will cause Rerun to panic."); + } +} + +fn env_var_bool(name: &str) -> Option { + std::env::var(name).ok() + .and_then(|s| match s.to_lowercase().as_str() { + "0" | "false" | "off" | "no" => Some(false), + "1" | "true" | "on" | "yes" => Some(true), + _ => { + crate::warn!( + "Invalid value for environment variable {name}={s:?}. Expected 'on' or 'off'. It will be ignored" + ); + None + } + }) } #[cfg(target_arch = "wasm32")] @@ -60,3 +80,28 @@ pub fn setup_web_logging() { ))) .expect("Failed to install logger"); } + +// ---------------------------------------------------------------------------- + +struct PanicOnWarn {} + +impl log::Log for PanicOnWarn { + fn enabled(&self, metadata: &log::Metadata<'_>) -> bool { + match metadata.level() { + log::Level::Error | log::Level::Warn => true, + log::Level::Info | log::Level::Debug | log::Level::Trace => false, + } + } + + fn log(&self, record: &log::Record<'_>) { + let level = match record.level() { + log::Level::Error => "error", + log::Level::Warn => "warning", + log::Level::Info | log::Level::Debug | log::Level::Trace => return, + }; + + panic!("{level} logged with RERUN_PANIC_ON_WARN: {}", record.args()); + } + + fn flush(&self) {} +} diff --git a/crates/rerun/src/run.rs b/crates/rerun/src/run.rs index 65403abe4533..e906aed2f346 100644 --- a/crates/rerun/src/run.rs +++ b/crates/rerun/src/run.rs @@ -103,13 +103,9 @@ struct Args { #[clap(long)] skip_welcome_screen: bool, - /// Exit with a non-zero exit code if any warning or error is logged. Useful for tests. - #[clap(long)] - strict: bool, - /// Ingest data and then quit once the goodbye message has been received. /// - /// Used for testing together with the `--strict` argument. + /// Used for testing together with `RERUN_PANIC_ON_WARN=1`. /// /// Fails if no messages are received, or if no messages are received within a dozen or so seconds. #[clap(long)] @@ -277,11 +273,6 @@ where return Ok(0); } - if args.strict { - re_log::add_boxed_logger(Box::new(StrictLogger {})).expect("Failed to enter --strict mode"); - re_log::info!("--strict mode: any warning or error will cause Rerun to panic."); - } - let res = if let Some(command) = &args.command { match command { #[cfg(feature = "analytics")] @@ -723,35 +714,3 @@ fn reset_viewer() -> anyhow::Result<()> { Ok(()) } } - -// ---------------------------------------------------------------------------- - -use re_log::external::log; - -struct StrictLogger {} - -impl log::Log for StrictLogger { - fn enabled(&self, metadata: &log::Metadata<'_>) -> bool { - match metadata.level() { - log::Level::Error | log::Level::Warn => true, - log::Level::Info | log::Level::Debug | log::Level::Trace => false, - } - } - - fn log(&self, record: &log::Record<'_>) { - let level = match record.level() { - log::Level::Error => "error", - log::Level::Warn => "warning", - log::Level::Info | log::Level::Debug | log::Level::Trace => return, - }; - - eprintln!("{level} logged in --strict mode: {}", record.args()); - eprintln!( - "{}", - re_crash_handler::callstack_from(&["log::__private_api_log"]) - ); - std::process::exit(1); - } - - fn flush(&self) {} -} diff --git a/docs/code-examples/roundtrips.py b/docs/code-examples/roundtrips.py index f850daff880d..cca77317d9fe 100755 --- a/docs/code-examples/roundtrips.py +++ b/docs/code-examples/roundtrips.py @@ -219,6 +219,9 @@ def roundtrip_env(*, save_path: str | None = None) -> dict[str, str]: # Turn on strict mode to catch errors early env["RERUN_STRICT"] = "1" + # Treat any warning as panics + env["RERUN_PANIC_ON_WARN"] = "1" + if save_path: # NOTE: Force the recording stream to write to disk! env["_RERUN_TEST_FORCE_SAVE"] = save_path diff --git a/scripts/run_python_e2e_test.py b/scripts/run_python_e2e_test.py index 2417ab03e1f8..e0a3a3431cb0 100755 --- a/scripts/run_python_e2e_test.py +++ b/scripts/run_python_e2e_test.py @@ -87,15 +87,16 @@ def run_example(example: str, extra_args: list[str]) -> None: if python_executable is None: python_executable = "python3" - rerun_process = subprocess.Popen( - [python_executable, "-m", "rerun", "--port", str(PORT), "--strict", "--test-receive"] - ) - time.sleep(0.3) # Wait for rerun server to start to remove a logged warning + env = os.environ.copy() + env["RERUN_STRICT"] = "1" + env["RERUN_PANIC_ON_WARN"] = "1" + + cmd = [python_executable, "-m", "rerun", "--port", str(PORT), "--test-receive"] + rerun_process = subprocess.Popen(cmd, env=env) + time.sleep(0.5) # Wait for rerun server to start to remove a logged warning - run_env = os.environ.copy() - run_env["RERUN_STRICT"] = "1" cmd = [python_executable, example, "--connect", "--addr", f"127.0.0.1:{PORT}"] + extra_args - python_process = subprocess.Popen(cmd, env=run_env) + python_process = subprocess.Popen(cmd, env=env) print("Waiting for python process to finish…") returncode = python_process.wait(timeout=30) diff --git a/tests/roundtrips.py b/tests/roundtrips.py index 0512790fe846..817ae4bec17f 100755 --- a/tests/roundtrips.py +++ b/tests/roundtrips.py @@ -164,6 +164,9 @@ def roundtrip_env() -> dict[str, str]: # Turn on strict mode to catch errors early env["RERUN_STRICT"] = "1" + # Treat any warning as panics + env["RERUN_PANIC_ON_WARN"] = "1" + return env From 1693cf8fd0482fa823f1a5f0351d2dbb3c1dd9c2 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Mon, 16 Oct 2023 15:33:17 +0200 Subject: [PATCH 2/2] Fix compilation on web --- crates/re_log/src/setup.rs | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/crates/re_log/src/setup.rs b/crates/re_log/src/setup.rs index ca54803f029c..fdefbabb59e1 100644 --- a/crates/re_log/src/setup.rs +++ b/crates/re_log/src/setup.rs @@ -57,6 +57,19 @@ pub fn setup_native_logging() { } } +#[cfg(target_arch = "wasm32")] +pub fn setup_web_logging() { + crate::multi_logger::init().expect("Failed to set logger"); + log::set_max_level(log::LevelFilter::Debug); + crate::add_boxed_logger(Box::new(crate::web_logger::WebLogger::new( + log::LevelFilter::Debug, + ))) + .expect("Failed to install logger"); +} + +// ---------------------------------------------------------------------------- + +#[cfg(not(target_arch = "wasm32"))] fn env_var_bool(name: &str) -> Option { std::env::var(name).ok() .and_then(|s| match s.to_lowercase().as_str() { @@ -71,20 +84,10 @@ fn env_var_bool(name: &str) -> Option { }) } -#[cfg(target_arch = "wasm32")] -pub fn setup_web_logging() { - crate::multi_logger::init().expect("Failed to set logger"); - log::set_max_level(log::LevelFilter::Debug); - crate::add_boxed_logger(Box::new(crate::web_logger::WebLogger::new( - log::LevelFilter::Debug, - ))) - .expect("Failed to install logger"); -} - -// ---------------------------------------------------------------------------- - +#[cfg(not(target_arch = "wasm32"))] struct PanicOnWarn {} +#[cfg(not(target_arch = "wasm32"))] impl log::Log for PanicOnWarn { fn enabled(&self, metadata: &log::Metadata<'_>) -> bool { match metadata.level() {