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

provide feature to use eyre instead of anyhow #85

Closed
wants to merge 1 commit into from

Conversation

SuperFluffy
Copy link
Collaborator

This PR adds the anyhow and eyre features to all crates using anyhow for errors. anyhow is added to default so nothing changes for standard workflows. Caveat: anyhow does not allow to opt out of std.

What do you think? Does this fit into your design?

Background: I want to use eyre in cnidarium and other crates that astria's Sequencer depends on (hence this PR targetting ibc-types, which are a blocker for cnidarium to migrate). We are using an eyre error hook [1] in our services to customize our error Display formatting in order to get full source-chains emitted in tracing events. There are two reasons for this:

  1. Tracing implements Value only for dyn std::error::Error. So in order to get full error backtraces (while avoiding debug formatting), one needs to convert the anyhow error to a trait object using a clunky error = AsRef::<dyn std::error::Error>::as_ref(&the_error).
  2. Even if one correctly converted to a error trait-object, some tracing-subscriber formatters (namely the Json one) don't follow the source-chain, only printing the top-most context. There is a PR sitting idle for 6 months now and is not being merged [2].

Because of these 2 issues we customized the error hook lest we miss out on important information.

1: https://github.com/astriaorg/astria/blob/6f13e7f126c7df35c764cae309f79fa28090028c/crates/astria-eyre/src/lib.rs#L27
2: tokio-rs/tracing#2797

@SuperFluffy
Copy link
Collaborator Author

SuperFluffy commented Apr 22, 2024

The various jobs are unhappy because of this mutually exclusive feature. Not surprising as this is usually frowned upon unless there is no way around these (as is the case here).

As I see it there are 3 ways to go about this:

  1. reject this PR ( 😢 ).
  2. instead of running with --all-features, provide a feature flag --full on all sub-crates that explicitly spells out all features and use that instead.
  3. replace dynamic errors by typed errors.

Number 2. is likely the best way forward and has precedent in the ecosystem. I can go ahead and provide this if you are happy to move forward this with this approach.

EDIT: there is the additional option of doing the below, but I decided to forego this as being too surprising.

#[cfg(not(feature="eyre"))]
extern crate anyhow;
#[cfg(feature="eyre")]
extern crate eyre as anyhow;

@hdevalence hdevalence requested review from cratelyn and erwanor April 22, 2024 15:08
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

The various jobs are unhappy because of this mutually exclusive feature. Not surprising as this is usually frowned upon unless there is no way around these (as is the case here).

As I see it there are 3 ways to go about this:

1. reject this PR ( 😢 ).

2. instead of running with `--all-features`, provide a feature flag `--full` on all sub-crates that explicitly spells out all features and use that instead.

3. replace dynamic errors by typed errors.

Number 2. is likely the best way forward and has precedent in the ecosystem. I can go ahead and provide this if you are happy to move forward this with this approach.

this is great work, thanks! i'm also a big fan of eyre.

(2) sounds roughly like the right course of action to me.

my personal taste would be for using an explicit --features list in the affected CI jobs rather than introducing a full feature, but this position is lightly held, and i'd be interested in @erwanor's opinion regarding that.

i do care that we have a CI job(s) enforcing that the default feature set compiles properly, and that the library compiles when the std and eyre flags are set.

i would be happy with a simple CI job that invokes cargo check --features eyre, but the most ideal way of going about that would probably involve cargo hack. penumbra-zone/penumbra#4166 is a recent example of using cargo hack in CI if we want to try that, but i do not consider it a blocker.

@@ -18,7 +18,7 @@ publish = true
all-features = true

[features]
default = ["std"]
default = ["std", "anyhow"]
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 in favor of this as a default.

@erwanor
Copy link
Member

erwanor commented Jun 14, 2024

Discussed offline, we are going to circle back to this in a bit and use typed errors in the domain type subcrate.

@erwanor erwanor closed this Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants