rust-lang-deprecated / error-chain

Error boilerplate for Rust
Apache License 2.0
729 stars 111 forks source link

Cache whether RUST_BACKTRACE is enabled in a relaxed atomic static. #210

Closed eddyb closed 7 years ago

eddyb commented 7 years ago

Fixes #207. Inspired by libfringe (thanks, @edef1c!).

eddyb commented 7 years ago

Apparently there's a test that changes RUST_BACKTRACE at runtime. The intention seems to be to check a few potential RUST_BACKTRACE values, shouldn't it be done by running a different executable via std::process?

Yamakaky commented 7 years ago

It's an edge case, I think a subprocess would be fine.

Arnavion commented 7 years ago

Perhaps split it up into separate tests (one for each RUST_BACKTRACE value) rather than actual subprocesses? nvm, I see the flag is static, so it will apply to all tests in the binary.

faern commented 7 years ago

Would it not make sense to implement this with std::sync::Once instead of manually having to encode whether or not the environment variable has been loaded? But maybe that has noticeably worse performance?

faern commented 7 years ago

I have created a PR on @eddyb 's fork that fixes the failing test by running it as a subprocess. https://github.com/eddyb/error-chain/pull/1

I have verified it works on Linux, but I have not tested other platforms. I wrote the code with a special case for Windows, but I have not verified that it actually runs.

eddyb commented 7 years ago

Thanks, @faern! ping @Yamakaky

Yamakaky commented 7 years ago

Nice! Could you update the changelog?

faern commented 7 years ago

I added PR for updating change log: https://github.com/eddyb/error-chain/pull/2

eddyb commented 7 years ago

I still have to play with cargo run, unless @faern gets to it first 😆

faern commented 7 years ago

What is it that you are trying to solve? Moving has_backtrace.rs out of bin/ so it is not automatically picked up and built by cargo test? examples/ does not sound like an optimal place to put it IMO. examples/ is also automatically picked up by cargo test, plus users looking there expect to see examples of how to use the library.

faern commented 7 years ago

Manually compiling it has the downside of us not knowing how the user has configured their toolchain. Just running rustc ... might not be on the PATH or might not be what the user want to run. Maybe they have invoked the tests with cargo +beta test which is something a hardcoded cargo .../rustc ... would miss.

eddyb commented 7 years ago

Hmm in those cases I don't know how to actually have a separate process. Then again, this is a test so if cargo run doesn't work...

faern commented 7 years ago

I still didn't get what the problem we are trying to solve is? Is it too ugly to have it in bin/? Even if we rename it to test_has_backtrace.rs or internal_test_helper__do_not_use__has_backtrace.rs? :D

eddyb commented 7 years ago

It doesn't work. Look at the test failure. It worked for you only because you built it manually.

faern commented 7 years ago

Hmm. What? cargo clean && cargo test works for me. I didn't build anything manually. There is only one build failing, and it looks like that is because of some syntax error in the backtrace crate.

eddyb commented 7 years ago

@faern Huh, my bad, guess it... fixed itself? Really weird since the last failure I saw was not finding that executable. I'll try to rebase then.

faern commented 7 years ago

It looks like we have to bump the minimum required version from 1.13.0 to 1.14.0. Looks like backtrace 0.3.3 won't build on stable Rust 1.13.0. I verified locally that it builds on 1.14.0

faern commented 7 years ago

Ping @Yamakaky :)

Yamakaky commented 7 years ago

Yeah, it's good !

Good coop btw, keeping an eye on it ;)

Yamakaky commented 7 years ago

Are you good for merge?

eddyb commented 7 years ago

I think so.

Yamakaky commented 7 years ago

Manually merged. Thank you two!