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

Fix compile probe and nightly backtraces #160

Merged
merged 6 commits into from
Jul 18, 2024
Merged

Conversation

ten3roberts
Copy link
Contributor

This addresses the invalid compile probe testing for a non-longer existing feature by updating it and eyre to use the error_generic_member_access features instead.

The report and errors have all been updated to accomodate this and the new backtrace provide API

Fixes: #84 and #97

@ten3roberts
Copy link
Contributor Author

I have to pin trybuild as it has added a dependency to toml which only supports >1.70

@ten3roberts ten3roberts force-pushed the fix-nightly-backtraces branch from 011f9a1 to be497c8 Compare March 26, 2024 12:34
@ten3roberts
Copy link
Contributor Author

Well... this is embarassing.

Once again the backtrace test is being finicky, this time due to (I believe) a warning injected by cargo, breaking it all.

@ten3roberts ten3roberts force-pushed the fix-nightly-backtraces branch from be497c8 to fdb7d84 Compare March 26, 2024 13:13
@ten3roberts
Copy link
Contributor Author

ten3roberts commented Mar 26, 2024

I fixed it in #161 and included the fix here.

Will rebase ontop of #161 when that is merged

eyre/build.rs Outdated
_ => {}
}
// Supports invoking rustc thrugh a wrapper
let mut cmd = if let Some(wrapper) = env::var_os("RUSTC_WRAPPER") {
Copy link

@RalfJung RalfJung Mar 26, 2024

Choose a reason for hiding this comment

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

It turns out there is also RUSTC_WORKSPACE_WRAPPER which should probably be honored as well. Furthermore, the wrapper can be empty and that means "no wrapping".

This is a reasonable template to start with.

@thenorili
Copy link
Contributor

thenorili commented Mar 28, 2024

It looks like this isn't quite finished. Have you considered working from this PR to fix nightly backtraces?

#124

@thenorili thenorili self-assigned this Apr 25, 2024
@thenorili thenorili self-requested a review April 25, 2024 21:12
@thenorili thenorili assigned ten3roberts and unassigned thenorili Apr 25, 2024
This addresses the invalid compile probe testing for a non-longer
existing feature by updating it and eyre to use the
`error_generic_member_access` features instead.

The report and errors have all been updated to accomodate this and the
new backtrace provide API

Fixes: #84 and #97
@ten3roberts ten3roberts force-pushed the fix-nightly-backtraces branch from e766676 to 92b6a57 Compare April 25, 2024 21:27
Copy link
Contributor

@thenorili thenorili left a comment

Choose a reason for hiding this comment

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

This looks okay to me to move forward with, sorry for the slowness!

Copy link
Contributor

Choose a reason for hiding this comment

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

Not your code / out of scope: We're getting a couple dead code errors in this file with DisplayError and NoneError. I'm surprised they're triggered even though we marked them public. Why don't we tag these structs as #[allow(dead_code)]?

I am similarly distressed by the other warnings we get a dozen copies of down below. I thought the missing_doc_code_examples thing was resolved ages ago.

Copy link

@RalfJung RalfJung May 6, 2024

Choose a reason for hiding this comment

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

I'm surprised they're triggered even though we marked them public.

pub in a private module can still be dead code.

pub(crate) is not even public, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understand that. I guess my surprise is that it's existing code that's had attention paid to its visibility and yet it's dead. I'll find the last time it wasn't dead code and figure its story out.

Copy link
Contributor

Choose a reason for hiding this comment

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

What was your motivation for rewriting this file?


Backtraces are captured when an error is converted to an `eyre::Report` (such as using `?` or `eyre!`).

If using the nightly toolchain, backtraces will also be captured and accessed from other errors using [error_generic_member_access](https://github.com/rust-lang/rfcs/pull/2895) if available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #121, some folks will continue to be unhappy about enforcing unstable features whenever we detect nightly. IIRC we discussed a while ago that we want to accomodate that use case. It doesn't make it any worse so I think it's fine to put that work off, but I thought it was worth mentioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, not wanting to break existing behavior in a "fix" PR, but rather make existing up to date nightly a tricky beast when it comes to libraries.

@ten3roberts ten3roberts merged commit df42dc4 into master Jul 18, 2024
29 checks passed
@ten3roberts ten3roberts deleted the fix-nightly-backtraces branch July 18, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-eyre Area: eyre subcrate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build.rs probe fails to propagate --target and does not use RUSTC_WRAPPER
4 participants