Dull the errors

[sbeckeriv]

This resolves #426

Dearest Reviewer,

I have updated the error messages to use say_status at the shell level. I have also changed say_status to print the message in bold. I do think it looks nice but it does have the side effect of making some seemingly unrelated text bold. I do think it looks better bold but it is also very easy to revert. I have included examples of both.

Thank you,
Becker

Bold: Note the usage is bold.

No bold:

Both rendered on a mac with iterm 2.9.0

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

Thanks for the PR @sbeckeriv! I'm quite happy to see the UI of Cargo get some attention I feel like it's... been lacking for awhile :)

On one hand I think we may want to try to mirror the compiler's output as it's tried and true by this point as well. That would mean starting the error message with "error" instead of "Error" and using bold for the message. That runs the risk, however, of having Cargo's errors be indistinguishable from the compiler's errors, which could be unfortunate.

On the other hand I feel like it's definitely pretty bad today that we just print the entire message in red which makes it easy to overlook various parts. UI's not always been my strong suit, but I'm sure others may have opinions!

cc @rust-lang/tools, @brson, @wycats

[sbeckeriv]

The lower case error is an easy change. Might I suggest "error[cargo]:" or "cargo error:". I often use something like the first format when sending email errors to tell me which system is having the error.

Thanks
Becker

[alexcrichton]

Hm "cargo error" may not be bad, yeah, we probably want to avoid "error[cargo]" unless the compiler also starts doing that as well.

I'm also a little curious why no tests needed to change? I would expect some tests need to be updated to match this output...

[sbeckeriv]

Playing with manual error cases I have found a few more for example using the bad-cksum package. I will also search for anywhere currently using RED.

My thought around "error[subsystem]" is that only the compiler would print "error" every other tool would identify itself in the message. I like "cargo error" just as well. I

[sbeckeriv]

[This is incorrect please see my next comment]
So funny story. The reason the test are green on github and they appear to pass locally is because I am running my test with the installed local cargo not the one I just built with the new error message. Also locally I get no output after the "test test_shell::no_term".

So before I go changing all the text in most of the test what are we thinking for text? Also should the test in the CI run on the newly compiled version of cargo?

I searched issues but found nothing about improving cargo's tests. I do not have a suggestion currently but the exact text matching is a bummer.

[sbeckeriv]

TLDR: mulitrust was causing my cargo tests to fail. I do have an errant error with no message after the test complete running running that I do not have with my installed version of cargo. What the errant error looks like https://gist.github.com/sbeckeriv/16bd5cabbcd32f9ae613

Long did read:
Ok. Now I am just embarrassing myself. I am running ./target/debug/cargo test or ./target/release/cargo test on master with a lot of failures. But my 0.7.0 nightly cargo passes tests. After running a standalone test I found the message:

"multirust: no default toolchain configured"

So I uninstalled mulitrust <sorry @brson > and I am running on beta 1.7 and master is fine. Also I get my standard 14 errors on this branch.

[sbeckeriv]

To follow up on the errant error message it comes from https://github.com/rust-lang/cargo/blob/master/src/bin/test.rs#L110 which also appears in bench as well. I am guessing it is so an error status is returned? Maybe we could just add a message "test have failed"?

[alexcrichton]

Ah yeah I've had problems with multirust in the past because the test suite changes HOME which can confuse multirust. I forget the incantation to fix this though because I use multirust and cargo test "just works" for me locally.

Maybe there's something wrong with the output matching in the test suite? We surely print an error somewhere and match on it, so we should expect the output to be somewhere...

[sbeckeriv]

I updated a test to check for the text error in a message. It appears that with_stderr acts more like partial match and since I am prepending the message the test do not fail.

It looks like the error in test and bench at https://github.com/rust-lang/cargo/blob/master/src/bin/test.rs#L110 print to stdout not stderr. I have yet to locate this output location. I am adding text to the empty error "test failed" and "bench failed" just so its not a random looking error message.

Thanks!
Becker

[alexcrichton]

Hm that's definitely a bug if we're just doing suffix matching and not matching the entire line.

[alexcrichton]

Ok, found the bug and fixed in #2433. Once that lands this will likely need updates to quite a few tests.

[sbeckeriv]

186 failed! I am guessing you do not want me just place [..] in front of every message? Is there a final word on the text before I dive in? I should have some free time this weekend.

Thanks
Becker

[alexcrichton]

I'd personally prefer to update the text that's matched, meaning we'd add error: in front. It may be worth getting some more consensus on this first though before changing all of them.

[sbeckeriv]

I updated all the errors to use format! and I included a new support helper ERROR. updating the error text should be a 3 line change once consensus is reached.

[alexcrichton]

Thanks @sbeckeriv! We've got a tools triage meeting this week so I'll be sure to bring this up as part of that.

[alexcrichton]

Ok, thanks for the patience @sbeckeriv! Could you remove the colon from error: (as it's already justified like the other status messages), and can you also remove the bold from the line? Other than that this looks good to go!

[sbeckeriv]

@alexcrichton Updated.

Thanks for the help!
Becker

[alexcrichton]

Thanks! Could you also squash the commits down?

[sbeckeriv]

Squashed!

Thanks!
Becker

[alexcrichton]

@bors: r+ 97a63ca99093cc13c357e4e86f974ef8a6bcad23

Can't wait to give this a spin!

[sbeckeriv]

Rebased off of master. Updated a new test that was added. For some reason nightly is failing but I dont think its related to my change.

[alexcrichton]

@bors: r+ f04e1b225ff64459fb2095e2940a1faa13c98226

[bors]

⌛ Testing commit f04e1b2 with merge 0050a68...

[bors]

💔 Test failed - cargo-linux-64

[sbeckeriv]

Updated test_cargo_cross_compile::platform_specific_dependencies_do_not_leak for the linux-64 build

[sbeckeriv]

Rebased master. Nightly should pass again.

[alexcrichton]

@bors: r+ 1c43987

[bors]

⌛ Testing commit 1c43987 with merge 626eeb7...

[bors]

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64