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.

Sign up for GitHub

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

Output errors on a single line if backtraces are disabled #1514

Merged
merged 8 commits into from
Nov 16, 2021
Merged

Output errors on a single line if backtraces are disabled #1514

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fa84de4
Output errors on a single line if ANSI output is disabled
romac Oct 27, 2021
59ed7f4
Add changelog entry
romac Nov 9, 2021
af05fca
Use published version of `oneline-eyre`
romac Nov 10, 2021
ca0cdba
Use oneline logs if RUST_BACKTRACE is unset, set to 0 or empty
romac Nov 10, 2021
a35c775
Use colored logs only if ANSI output is enabled
romac Nov 10, 2021
fc4eec8
Use value of RUST_BACKTRACE at runtime instead of compile time
romac Nov 10, 2021
8588226
Simplify code
romac Nov 10, 2021
b5969be
Use single-line errors if backtraces are disabled even with a tty
romac Nov 11, 2021
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
2 changes: 2 additions & 0 deletions .changelog/unreleased/improvements/ibc-relayer-cli/1515-single-line-errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Output errors on a single line if ANSI output is disabled
([#1515](https://github.com/informalsystems/ibc-rs/issues/1515))
10 changes: 10 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions relayer-cli/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ tracing = "0.1.29"
tracing-subscriber = { version = "0.3.1", features = ["fmt", "env-filter", "json"]}
eyre = "0.6.5"
color-eyre = "0.5"
oneline-eyre = "0.1"
futures = "0.3.17"
toml = "0.5.8"
serde_derive = "1.0.116"
Expand Down
8 changes: 7 additions & 1 deletion relayer-cli/src/bin/hermes/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,15 @@ use ibc_relayer_cli::components::enable_ansi;

/// Boot Cli
fn main() -> eyre::Result<()> {
if enable_ansi() {
if compact_logs() {
oneline_eyre::install()?;
} else if enable_ansi() {
color_eyre::install()?;
}

abscissa_core::boot(&APPLICATION);
}

fn compact_logs() -> bool {
matches!(std::env::var("RUST_BACKTRACE").as_deref(), Ok("" | "0"))
Copy link
Contributor

@soareschen soareschen Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think in this case if RUST_BACKTRACE is not set, the error case Err(_) will return false when it should be true.

This should be clearer with comment explaining what should happen in different branches:

fn main() -> eyre::Result<()> {
    install_error_reporter()?;

    abscissa_core::boot(&APPLICATION);
}

fn install_error_reporter() -> eyre::Result<()> {
    if enable_ansi() {
        // If we are in a terminal supporting color, display
        // full error logs in color
        color_eyre::install()?;
    } else if !backtrace_enabled() {
        // Else if we are piping logs to file and backtrace is *not* enabled,
        // display errors in single line.
        oneline_eyre::install()?;
    } else {
        // Else backtrace is enabled and we are piping to logs, so use the
        // default error report handler, which displays multiline errors
        // without color.
    }

    Ok(())
}

fn backtrace_enabled() -> bool {
    match std::env::var("RUST_BACKTRACE").as_deref() {
        Ok("" | "0") | Err(_) => false,
        Ok(_) => true,
    }
}

Copy link
Member Author

@romac romac Nov 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch, thanks! Shouldn't we invert the first two branches? ie. if backtraces are not enabled, we use one-line logs, otherwise if there is a tty we use colors, else we use the default reporter.

fn install_error_reporter() -> eyre::Result<()> {
    if !backtrace_enabled() {
        // If backtraces are disabled, display errors in single line.
        oneline_eyre::install()
    } else if enable_ansi() {
        // Else, if backtraces are enabled and we are in a terminal
        // supporting color, display full error logs in color.
        color_eyre::install()
    } else {
        // Otherwise, backtraces are enabled and we are piping to logs, so use the
        // default error report handler, which displays multiline errors
        // without color.
        Ok(())
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it depends on what we want the default behavior to be. Most of the time the relayer will run without RUST_BACKTRACE set, even for terminal cases. Do we want those to display one-line errors as well?

Copy link

Choose a reason for hiding this comment

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

Most of the time the relayer will be piping to logs. So one line logs should be the default case.

}