allow RUST_BACKTRACE=0 to act as if unset #32549

respeccing · 2016-03-28T13:00:29Z

UPDATE: RUST_BACKTRACE=0 to act as if the env. var is unset! (now 0 is what disabled was for, below)

When RUST_BACKTRACE is set to "disabled" then this acts as if the env. var is unset. So, either make sure RUST_BACKTRACE is not set OR set it to disabled to achieve the same effect.

Sample usage:

$ rustc -o /tmp/a.out -- <(echo 'fn main(){ panic!() }') && RUST_BACKTRACE=disabled /tmp/a.out
!! executing '/home/zazdxscf/build/1nonpkgs/rust/rust//x86_64-unknown-linux-gnu/stage2/bin//rustc' with args: '-o /tmp/a.out -- /dev/fd/63'
thread '<main>' panicked at 'explicit panic', /dev/fd/63:1
note: Run with `RUST_BACKTRACE=1` for a backtrace.

$ rustc -o /tmp/a.out -- <(echo 'fn main(){ panic!() }') && RUST_BACKTRACE=1 /tmp/a.out
!! executing '/home/zazdxscf/build/1nonpkgs/rust/rust//x86_64-unknown-linux-gnu/stage2/bin//rustc' with args: '-o /tmp/a.out -- /dev/fd/63'
thread '<main>' panicked at 'explicit panic', /dev/fd/63:1
stack backtrace:
   1:     0x55709e8148c0 - sys::backtrace::tracing::imp::write::h140f24a0cfc189b98Ru
   2:     0x55709e816a5b - panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::closure.45165
   3:     0x55709e8166e8 - panicking::default_hook::hed419823688cb82aXoA
   4:     0x55709e810fff - sys_common::unwind::begin_unwind_inner::hbb9642f6e212d56fmHt
   5:     0x55709e810513 - sys_common::unwind::begin_unwind::h16232867470678019594
   6:     0x55709e810489 - main::hb524f9576270962feaa
   7:     0x55709e816314 - sys_common::unwind::try::try_fn::h1274188004693518534
   8:     0x55709e813dfb - __rust_try
   9:     0x55709e815dab - rt::lang_start::h712b1cd650781872ahA
  10:     0x55709e810679 - main
  11:     0x7efd1026859f - __libc_start_main
  12:     0x55709e810348 - _start
  13:                0x0 - <unknown>

Some programs(eg. vim's syntactic used by rust.vim) cannot unset the env. var RUST_BACKTRACE if it's already set(eg. in .bashrc) but they can set it to some value, and I needed to ensure the env. var is unset in order to avoid this issue: #29293

EDIT: Sample usage 2:

$ export RUST_BACKTRACE=1

$ rustc -o /tmp/a.out -- <(echo 'fn main(){ panic!() }') && /tmp/a.out
!! executing '/home/zazdxscf/build/1nonpkgs/rust/rust//x86_64-unknown-linux-gnu/stage2/bin//rustc' with args: '-o /tmp/a.out -- /dev/fd/63'
thread '<main>' panicked at 'explicit panic', /dev/fd/63:1
stack backtrace:
   1:     0x55c2696738c0 - sys::backtrace::tracing::imp::write::h140f24a0cfc189b98Ru
   2:     0x55c269675a5b - panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::closure.45165
   3:     0x55c2696756e8 - panicking::default_hook::hed419823688cb82aXoA
   4:     0x55c26966ffff - sys_common::unwind::begin_unwind_inner::hbb9642f6e212d56fmHt
   5:     0x55c26966f513 - sys_common::unwind::begin_unwind::h16023941661074805588
   6:     0x55c26966f489 - main::hb524f9576270962feaa
   7:     0x55c269675314 - sys_common::unwind::try::try_fn::h1274188004693518534
   8:     0x55c269672dfb - __rust_try
   9:     0x55c269674dab - rt::lang_start::h712b1cd650781872ahA
  10:     0x55c26966f679 - main
  11:     0x7f593d58459f - __libc_start_main
  12:     0x55c26966f348 - _start
  13:                0x0 - <unknown>

$ rustc -o /tmp/a.out -- <(echo 'fn main(){ panic!() }') && RUST_BACKTRACE=disabled /tmp/a.out
!! executing '/home/zazdxscf/build/1nonpkgs/rust/rust//x86_64-unknown-linux-gnu/stage2/bin//rustc' with args: '-o /tmp/a.out -- /dev/fd/63'
thread '<main>' panicked at 'explicit panic', /dev/fd/63:1
note: Run with `RUST_BACKTRACE=1` for a backtrace.

rust-highfive · 2016-03-28T13:00:36Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

respeccing · 2016-03-28T14:54:34Z

src/libstd/sys/common/backtrace.rs

@@ -36,7 +36,7 @@ pub fn log_enabled() -> bool {
    }

    let val = match env::var_os("RUST_BACKTRACE") {
-        Some(..) => 2,
+        Some(x) => if x.eq("disabled") { 1 } else { 2 },


Should I transform this x.eq("disabled")
into this? x.to_lowercase().eq("disabled")
So that cases like DisAbled or DISABLED can also mean the same thing?!

EDIT: Nevermind, that's what I was afraid of:
sys/common/backtrace.rs:39:25: 39:37 error: no method named to_lowercase found for type ffi::os_str::OsString in the current scope

Why do you use .eq instead of ==?

I have no idea, should I change it to == ? I currently don't know the difference...

I remembered...

sys/common/backtrace.rs:39:23: 39:38 error: the trait `core::cmp::PartialEq<&str>` is not implemented for the type `ffi::os_str::OsString` [E0277] sys/common/backtrace.rs:39 Some(x) => if x == "disabled" { 1 } else { 2 }, ^~~~~~~~~~~~~~~ sys/common/backtrace.rs:39:23: 39:38 help: run `rustc --explain E0277` to see a detailed explanation sys/common/backtrace.rs:39:23: 39:38 help: the following implementations were found: sys/common/backtrace.rs:39:23: 39:38 help: <ffi::os_str::OsString as core::cmp::PartialEq> sys/common/backtrace.rs:39:23: 39:38 help: <ffi::os_str::OsString as core::cmp::PartialEq<str>> sys/common/backtrace.rs:39:23: 39:38 help: <ffi::os_str::OsString as core::cmp::PartialEq<ffi::os_str::OsStr>> sys/common/backtrace.rs:39:23: 39:38 help: <ffi::os_str::OsString as core::cmp::PartialEq<&'a ffi::os_str::OsStr>> sys/common/backtrace.rs:39:23: 39:38 help: and 5 others error: aborting due to previous error

Maybe there's another way to use '==', but I haven't yet reached those tutorials... still in the process of (re)reading rust-book.

and this works Some(x) => if &x == "disabled" { 1 } else { 2 }, apparently...

On another note, if I wanted to use to_lowercase() looks like this would work:

// Some(x) => if x.eq("disabled") { 1 } else { 2 }, Some(x) => { if match x.to_str() { None => "", Some(y) => y, }.to_lowercase() == "disabled" { 1 } else { 2 } },

Here's a test program that shows the above works.

Should I make a new commit with this? Is this desirable? Maybe there's a better way(EDIT: if at least formatting-wise)? Or is not desirable to have this logic(of allowing case insensitive disabled to work) ?

Please advise? Thank ye:)
EDIT2:: Oops, I pushed the commit :-" I figure, you'll let me know if to revert it or change it anyway, and it's better if it's here.

aturon · 2016-03-28T16:44:57Z

cc @brson @alexcrichton

alexcrichton · 2016-03-28T17:16:03Z

Thanks for the PR @respeccing! Seems like a legit use case in that removing env vars is typically more painful than setting them, and there's definitely use cases where you want to unset the env var.

I wonder if we may want to explore some other conventions for the naming here, though? The compiler, for example, has "disabling codgen options" of the form -C foo=no where the negative forms accepted are "n", "no", and "off". (no case folding either). I'd also be fine adding "0" in this case to be the negative of "RUST_BACKTRACE=1"

To me that seems somewhat more conventional than "disabled", although maybe I'm forgetting some tools? Do you know if other tools use "disabled" as a value for disabling something?

respeccing · 2016-03-28T17:33:01Z

Do you know if other tools use "disabled" as a value for disabling something?

I know of no such tools. The "disabled" was kind of random. I will change it to 0 in advance (even if this PR gets dropped eventually in favor of -C for which I wouldn't be able to generate code for, myself, but I'm ok with this PR being dropped in favor of that, no problem. Perhaps both =0 and the -C both make sense to have as features).

The =0 does make a lot more sense! than disabled. Not to mention it simplifies the code a ton! I like it! Thank you!

alexcrichton · 2016-03-28T18:29:12Z

Oh sorry I just used -C as an example, I don't think this will ever actually be an option to the compiler. Just in terms of conventions we may want to accept a few negatives here, but "0" seems like a good start at least.

respeccing · 2016-03-28T18:55:07Z

Oh I see what you meant now. I misread that before. Apologies :)

I could add those "n", "no", and "off". I'll take a look at other compiler variants. (and I guess "disable" and "disabled" are out of the question and I agree as to why)
But should they be case-insensitive?

error: incorrect value ON for codegen option debug-assertions - one of: y, yes, on, n, no, or off was expected

But it should probably be case-insensitive for our env. var... since otherwise it would be taken as enabled + there's no way to warn/err in this case! (imagine: NO or No or N, while expecting only no or n, would be bad to be taken as yes)

respeccing · 2016-03-28T20:09:51Z

Let me know(whenever possible, no rush) if I need to make any more changes, even if it means deleting features(case-insensitive) or code, or formatting. It's really no problem for me and I don't mind the time. I'd rather have it "done right" than be "good enough because we try to be kind".

Perhaps this needs to be documented somewhere... I'll try to figure out where... Some such places where I see RUST_BACKTRACE mentioned are RELEASES.md (which seems like I shouldn't touch with a PR myself) and functions.md (which I will touch with a commit immediately EDIT: rendered)

I appreciate everybody's time.
Cheers,

PS: sorry for the excessive rigmarole for what seems like such a trivial issue ;-) (I can't help it)

alexcrichton · 2016-03-28T21:07:35Z

Thanks @respeccing! I personally feel that detecting case folding isn't quite worth it. We most likely definitely don't want to allocate as part of this as it's called during unwinding (and we want to curb allocations there as much as possible), but others on @rust-lang/libs may feel differently.

respeccing · 2016-03-29T02:36:13Z

We most likely definitely don't want to allocate as part of this as it's called during unwinding

I gotta admit this was on my mind. I will undo the case-insensitive detection to avoid allocations.

(and we want to curb allocations there as much as possible)

I don't know about others but, I agree with this.

alexcrichton · 2016-03-31T00:59:22Z

The libs team discussed this during triage and the decision was we'd like to merge! We decided that only "0" should be used to disable backtraces, and all other values would implicitly mean "enabled".

Can you also update the handling of RUST_TEST_NOCAPTURE in libtest? That's another case which should likely mirror this as well.

respeccing · 2016-03-31T03:33:41Z

There are some other places that need to be checked... I'm on it... but, meanwhile I wanted to ask:
Is this acceptable:

nocapture = env::var("RUST_TEST_NOCAPTURE").unwrap_or("0".to_string()) != "0";

Notice how I had to use to_string() there... which is probably not a good way, in which case, I guess I'm stuck with this one:

nocapture = match env::var("RUST_TEST_NOCAPTURE") {
            Ok(val) => &val != "0",                                             
            Err(_) => false
        };

EDIT: Hmm, seems like these 2 settings should each have a function to query for their value... otherwise that check for whether the value is 0 must be done ... I see a few places for RUST_BACKTRACE...
EDIT2: not ready for merge yet... i must fix some grep finds :) EDIT3: I have to take a nap, else I can't think :) btw is &val above better than val in our context of less allocations/stuff ? (I just left it &val from an earlier code revision)

alexcrichton · 2016-03-31T04:49:47Z

Thanks @respeccing! Could you add a test for RUST_BACKTRACE=0 as well? Other than that this looks good to me.

respeccing · 2016-03-31T12:16:58Z

All done :)
(assuming tests pass - I couldn't check locally; well, they're in progress anyway, supposedly xD)

alexcrichton · 2016-03-31T16:51:04Z

Can you also squash all the commits down into one? r=me other than that

/# This is a combination of 16 commits. /# The first commit's message is: allow RUST_BACKTRACE=disabled to act as if unset When RUST_BACKTRACE is set to "disabled" then this acts as if the env. var is unset. /# This is the 2nd commit message: case insensitive "DiSaBLeD" RUST_BACKTRACE value previously it expected a lowercase "disabled" to treat the env. var as unset /# This is the 3rd commit message: RUST_BACKTRACE=0 acts as if unset previously RUST_BACKTRACE=disabled was doing the same thing /# This is the 4th commit message: RUST_BACKTRACE=0|n|no|off acts as if unset previously only RUST_BACKTRACE=0 acted as if RUST_BACKTRACE was unset Now added more options (case-insensitive): 'n','no' and 'off' eg. RUST_BACKTRACE=oFF /# This is the 5th commit message: DRY on the value of 2 DRY=don't repeat yourself Because having to remember to keep the two places of '2' in sync is not ideal, even though this is a simple enough case. /# This is the 6th commit message: Revert "DRY on the value of 2" This reverts commit 95a0479d5cf72a2b2d9d21ec0bed2823ed213fef. Nevermind this DRY on 2, because we already have a RY on 1, besides the code is less readable this way... /# This is the 7th commit message: attempt to document unsetting RUST_BACKTRACE /# This is the 8th commit message: curb allocations when checking for RUST_BACKTRACE this means we don't check for case-insensitivity anymore /# This is the 9th commit message: as decided, RUST_BACKTRACE=0 turns off backtrace /# This is the 10th commit message: RUST_TEST_NOCAPTURE=0 acts as if unset (that is, capture is on) Any other value acts as if nocapture is enabled (that is, capture is off) /# This is the 11th commit message: update other RUST_TEST_NOCAPTURE occurrences apparently only one place needs updating /# This is the 12th commit message: update RUST_BACKTRACE in man page /# This is the 13th commit message: handle an occurrence of RUST_BACKTRACE /# This is the 14th commit message: ensure consistency with new rules for backtrace /# This is the 15th commit message: a more concise comment for RUST_TEST_NOCAPTURE /# This is the 16th commit message: update RUST_TEST_NOCAPTURE in man page

respeccing · 2016-04-01T00:14:07Z

r? @alexcrichton

(I waited for travis-ci to finish up:) )

alexcrichton · 2016-04-01T17:48:15Z

@bors: r+ e1d2eda

bors · 2016-04-02T08:48:00Z

⌛ Testing commit e1d2eda with merge f2285bd...

…hton allow RUST_BACKTRACE=0 to act as if unset **UPDATE:** `RUST_BACKTRACE=0` to act as if the env. var is unset! (now `0` is what `disabled` was for, below) When RUST_BACKTRACE is set to "disabled" then this acts as if the env. var is unset. So, either make sure `RUST_BACKTRACE` is not set OR set it to `disabled` to achieve the same effect. Sample usage: ```bash $ rustc -o /tmp/a.out -- <(echo 'fn main(){ panic!() }') && RUST_BACKTRACE=disabled /tmp/a.out !! executing '/home/zazdxscf/build/1nonpkgs/rust/rust//x86_64-unknown-linux-gnu/stage2/bin//rustc' with args: '-o /tmp/a.out -- /dev/fd/63' thread '<main>' panicked at 'explicit panic', /dev/fd/63:1 note: Run with `RUST_BACKTRACE=1` for a backtrace. $ rustc -o /tmp/a.out -- <(echo 'fn main(){ panic!() }') && RUST_BACKTRACE=1 /tmp/a.out !! executing '/home/zazdxscf/build/1nonpkgs/rust/rust//x86_64-unknown-linux-gnu/stage2/bin//rustc' with args: '-o /tmp/a.out -- /dev/fd/63' thread '<main>' panicked at 'explicit panic', /dev/fd/63:1 stack backtrace: 1: 0x55709e8148c0 - sys::backtrace::tracing::imp::write::h140f24a0cfc189b98Ru 2: 0x55709e816a5b - panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::closure.45165 3: 0x55709e8166e8 - panicking::default_hook::hed419823688cb82aXoA 4: 0x55709e810fff - sys_common::unwind::begin_unwind_inner::hbb9642f6e212d56fmHt 5: 0x55709e810513 - sys_common::unwind::begin_unwind::h16232867470678019594 6: 0x55709e810489 - main::hb524f9576270962feaa 7: 0x55709e816314 - sys_common::unwind::try::try_fn::h1274188004693518534 8: 0x55709e813dfb - __rust_try 9: 0x55709e815dab - rt::lang_start::h712b1cd650781872ahA 10: 0x55709e810679 - main 11: 0x7efd1026859f - __libc_start_main 12: 0x55709e810348 - _start 13: 0x0 - <unknown> ``` Some programs(eg. [vim's syntactic](https://github.com/scrooloose/syntastic) used by [rust.vim](https://github.com/rust-lang/rust.vim)) cannot unset the env. var RUST_BACKTRACE if it's already set(eg. in .bashrc) but [they can set it to some value](https://github.com/respeccing/gentooskyline/blob/cb5533e1598f871d3fdf7c3d8248ce767b5b9360/system/Z575/OSes/gentoo/on_baremetal/filesystem_now/gentoo/home/zazdxscf/build/1nonpkgs/rust.vim/upd#L17), and I needed to ensure the env. var is unset in order to avoid this issue: #29293 **EDIT:** Sample usage 2: ```bash $ export RUST_BACKTRACE=1 $ rustc -o /tmp/a.out -- <(echo 'fn main(){ panic!() }') && /tmp/a.out !! executing '/home/zazdxscf/build/1nonpkgs/rust/rust//x86_64-unknown-linux-gnu/stage2/bin//rustc' with args: '-o /tmp/a.out -- /dev/fd/63' thread '<main>' panicked at 'explicit panic', /dev/fd/63:1 stack backtrace: 1: 0x55c2696738c0 - sys::backtrace::tracing::imp::write::h140f24a0cfc189b98Ru 2: 0x55c269675a5b - panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::closure.45165 3: 0x55c2696756e8 - panicking::default_hook::hed419823688cb82aXoA 4: 0x55c26966ffff - sys_common::unwind::begin_unwind_inner::hbb9642f6e212d56fmHt 5: 0x55c26966f513 - sys_common::unwind::begin_unwind::h16023941661074805588 6: 0x55c26966f489 - main::hb524f9576270962feaa 7: 0x55c269675314 - sys_common::unwind::try::try_fn::h1274188004693518534 8: 0x55c269672dfb - __rust_try 9: 0x55c269674dab - rt::lang_start::h712b1cd650781872ahA 10: 0x55c26966f679 - main 11: 0x7f593d58459f - __libc_start_main 12: 0x55c26966f348 - _start 13: 0x0 - <unknown> $ rustc -o /tmp/a.out -- <(echo 'fn main(){ panic!() }') && RUST_BACKTRACE=disabled /tmp/a.out !! executing '/home/zazdxscf/build/1nonpkgs/rust/rust//x86_64-unknown-linux-gnu/stage2/bin//rustc' with args: '-o /tmp/a.out -- /dev/fd/63' thread '<main>' panicked at 'explicit panic', /dev/fd/63:1 note: Run with `RUST_BACKTRACE=1` for a backtrace. ```

bors · 2016-04-02T11:35:21Z

rust-highfive assigned aturon Mar 28, 2016

respeccing reviewed Mar 28, 2016
View reviewed changes

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 28, 2016

respeccing changed the title ~~allow RUST_BACKTRACE=disabled to act as if unset~~ allow RUST_BACKTRACE=0 to act as if unset Mar 28, 2016

respeccing force-pushed the rust_backtrace_disabled branch from e65c5cd to e1d2eda Compare March 31, 2016 21:04

rust-highfive assigned alexcrichton and unassigned aturon Apr 1, 2016

bors merged commit e1d2eda into rust-lang:master Apr 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow RUST_BACKTRACE=0 to act as if unset #32549

allow RUST_BACKTRACE=0 to act as if unset #32549

respeccing commented Mar 28, 2016

rust-highfive commented Mar 28, 2016

respeccing Mar 28, 2016

jonas-schievink Mar 28, 2016

respeccing Mar 28, 2016

respeccing Mar 28, 2016

respeccing Mar 28, 2016

respeccing Mar 28, 2016

aturon commented Mar 28, 2016

alexcrichton commented Mar 28, 2016

respeccing commented Mar 28, 2016

alexcrichton commented Mar 28, 2016

respeccing commented Mar 28, 2016

respeccing commented Mar 28, 2016

alexcrichton commented Mar 28, 2016

respeccing commented Mar 29, 2016

alexcrichton commented Mar 31, 2016

respeccing commented Mar 31, 2016

alexcrichton commented Mar 31, 2016

respeccing commented Mar 31, 2016

alexcrichton commented Mar 31, 2016

respeccing commented Apr 1, 2016

alexcrichton commented Apr 1, 2016

bors commented Apr 2, 2016

bors commented Apr 2, 2016

allow RUST_BACKTRACE=0 to act as if unset #32549

allow RUST_BACKTRACE=0 to act as if unset #32549

Conversation

respeccing commented Mar 28, 2016

rust-highfive commented Mar 28, 2016

respeccing Mar 28, 2016

Choose a reason for hiding this comment

jonas-schievink Mar 28, 2016

Choose a reason for hiding this comment

respeccing Mar 28, 2016

Choose a reason for hiding this comment

respeccing Mar 28, 2016

Choose a reason for hiding this comment

respeccing Mar 28, 2016

Choose a reason for hiding this comment

respeccing Mar 28, 2016

Choose a reason for hiding this comment

aturon commented Mar 28, 2016

alexcrichton commented Mar 28, 2016

respeccing commented Mar 28, 2016

alexcrichton commented Mar 28, 2016

respeccing commented Mar 28, 2016

respeccing commented Mar 28, 2016

alexcrichton commented Mar 28, 2016

respeccing commented Mar 29, 2016

alexcrichton commented Mar 31, 2016

respeccing commented Mar 31, 2016

alexcrichton commented Mar 31, 2016

respeccing commented Mar 31, 2016

alexcrichton commented Mar 31, 2016

respeccing commented Apr 1, 2016

alexcrichton commented Apr 1, 2016

bors commented Apr 2, 2016

bors commented Apr 2, 2016