Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace --strict flag with RERUN_PANIC_ON_WARN env-var #3872

Merged
merged 2 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions crates/re_log/src/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ 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.");
}
}

#[cfg(target_arch = "wasm32")]
Expand All @@ -60,3 +66,45 @@ pub fn setup_web_logging() {
)))
.expect("Failed to install logger");
}

// ----------------------------------------------------------------------------

#[cfg(not(target_arch = "wasm32"))]
fn env_var_bool(name: &str) -> Option<bool> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could swear I've seen this snippet several times over in the codebase, but can't find other instances right now

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(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() {
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) {}
}
43 changes: 1 addition & 42 deletions crates/rerun/src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -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) {}
}
3 changes: 3 additions & 0 deletions docs/code-examples/roundtrips.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 8 additions & 7 deletions scripts/run_python_e2e_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions tests/roundtrips.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down
Loading