eyre-rs / eyre

A trait object based error handling type for easy idiomatic error handling and reporting in Rust applications
Apache License 2.0
1.35k stars 63 forks source link

Add color-eyre to workspace #110

Closed pksunkara closed 6 months ago

ten3roberts commented 10 months ago

@pksunkara how is it going with this? Do you want me to have a look at the failing tests?

ten3roberts commented 10 months ago

It could very well be the same issue as the one with color-spantrace as it fetches the relative filename in the wrong working directory the same way

pksunkara commented 10 months ago

I need to figure out how to test multiple feature combinations across different crates.

thenorili commented 10 months ago

I need to figure out how to test multiple feature combinations across different crates.

from the cargo book:

--features FEATURES: Enables the listed features. Multiple features may be separated with commas or spaces. If using spaces, be sure to use quotes around all the features if running Cargo from a shell (such as --features "foo bar"). If building multiple packages in a workspace, the package-name/feature-name syntax can be used to specify features for specific workspace members.

So package-name/feature-name syntax is how the output works, but you'll need to enumerate the feature list of all the crates so you can script all the combinations. For that, cargo_metadata provides the feature list of a given crate in structured data.

https://docs.rs/cargo_metadata/latest/cargo_metadata/

I can work on the script sometime in the coming week if you like =]

thenorili commented 10 months ago

I need to figure out how to test multiple feature combinations across different crates.

from the cargo book:

--features FEATURES: Enables the listed features. Multiple features may be separated with commas or spaces. If using spaces, be sure to use quotes around all the features if running Cargo from a shell (such as --features "foo bar"). If building multiple packages in a workspace, the package-name/feature-name syntax can be used to specify features for specific workspace members.

So package-name/feature-name syntax is how the output works, but you'll need to enumerate the feature list of all the crates so you can script all the combinations. For that, cargo_metadata provides the feature list of a given crate in structured data.

https://docs.rs/cargo_metadata/latest/cargo_metadata/

I can work on the script sometime in the coming week if you like =]

https://github.com/frewsxcv/cargo-all-features/

I think cargo-all-features is a good foundation for this. It already tests all combinations of features and has access to the list of dependencies, it just needs to recurse over those dependencies.

thenorili commented 10 months ago

I need to figure out how to test multiple feature combinations across different crates.

from the cargo book:

--features FEATURES: Enables the listed features. Multiple features may be separated with commas or spaces. If using spaces, be sure to use quotes around all the features if running Cargo from a shell (such as --features "foo bar"). If building multiple packages in a workspace, the package-name/feature-name syntax can be used to specify features for specific workspace members.

So package-name/feature-name syntax is how the output works, but you'll need to enumerate the feature list of all the crates so you can script all the combinations. For that, cargo_metadata provides the feature list of a given crate in structured data. https://docs.rs/cargo_metadata/latest/cargo_metadata/ I can work on the script sometime in the coming week if you like =]

https://github.com/frewsxcv/cargo-all-features/

I think cargo-all-features is a good foundation for this. It already tests all combinations of features and has access to the list of dependencies, it just needs to recurse over those dependencies.

@pksunkara I've tested it for you and it works!

~/dev/eyre : cargo test -p color-eyre -F color-eyre/track-caller -F eyre/pyo3
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   C:\Users\nori\dev\eyre\color-eyre\Cargo.toml
workspace: C:\Users\nori\dev\eyre\Cargo.toml

I believe this is well-formed, I see pyo3 and the track-caller feature didn't fail to parse. You can enumerate them manually for now if you want to get unblocked, I'll work on automating it this weekend.

ten3roberts commented 9 months ago

Hey @pksunkara, do you want me to take over this issue and fix the remaining things?

pksunkara commented 9 months ago

Yes, please since I won't be able to get to it until Jan.

Important thing though, we need to enable normal merge for this PR to preserve history

ten3roberts commented 9 months ago

The test_error_backwards_compatability is failing for cargo test --all --no-default-features.

The test is very complex and has failed in the past when rust std added another function in the backtrace due to some internal change.

I can not reproduce this locally, so it may be because of some rustc version or something else entirely

ten3roberts commented 7 months ago

Ok, I've now found the reason for failure.

It has do to with rustc versions. 1.65 causes it to fail, 1.71 passes

ten3roberts commented 7 months ago

As I suspected, "updating" the "minimal_features" backtrace to match caused other tests using 1.75.0 to fail, since somewhere between those versions, the functions and call stack of the panic machinery changed which causes them to match.

The solution I see here is to ignore the tests for the older version, or don't test backtraces on older compiler versions.

ten3roberts commented 7 months ago

Found a common denominator and a way to make the tests more lenient in the future.

thenorili commented 7 months ago

Remember not to squash this once it's done!

thenorili commented 7 months ago

Remember not to squash this once it's done!

I'm not sure this will be that simple.

How are we going to preserve history when merging color-eyre into eyre? How did y'all do it for spantraces? Does a rebase work? If so, do we need to ping jane for an exception to the policy?

pksunkara commented 7 months ago

Creating a merge commit is enough, isn't it?

thenorili commented 7 months ago

Creating a merge commit is enough, isn't it?

I tested it and yes, that is how that works! Sorry for the alarm : )

ten3roberts commented 6 months ago

This works now (barring the conflict), how do we feel about merging this?

ten3roberts commented 6 months ago

I'll make sure to merge it properly into main to preserve the history