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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

report status to stderr instead of stdout #2693

Merged
merged 26 commits into from May 20, 2016

Conversation

Projects
None yet
6 participants
@matklad
Member

matklad commented May 14, 2016

All status messages are printed to stderr instead of stdout now.

This changes exactly three letters of code and breaks 211 tests 馃槰

I will hopefully fix all the tests here, but I decided to submit PR early in case somebody comes up with a less laborious way to do this.

Fixes #1473, #2661

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive May 14, 2016

r? @alexcrichton

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

rust-highfive commented May 14, 2016

r? @alexcrichton

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

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 14, 2016

Member

breaks 211 tests

... on the stable compiler. There should be even for failures for nightly

Member

matklad commented May 14, 2016

breaks 211 tests

... on the stable compiler. There should be even for failures for nightly

matklad added a commit to matklad/intellij-rust that referenced this pull request May 15, 2016

Add custom refactoring
Will help to fix [cargo-issue]

[cargo-issue]: rust-lang/cargo#2693

matklad added a commit to matklad/intellij-rust that referenced this pull request May 15, 2016

Add custom refactoring
Will help to fix [cargo-issue]

[cargo-issue]: rust-lang/cargo#2693
@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 15, 2016

Member

91 failures left :)

Member

matklad commented May 15, 2016

91 failures left :)

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 15, 2016

Member

Hm, I found an interesting phenomena: some tests before this changed asserted stdout and ignored stderr although some warnings were printed to stderr. When converting with_stdout to with_stderr these test fail because there is more output. One way to fix them is to add warnings to the expected output, like I did in this commit. But is this really the best way? Could changing with_stderr to with_stderr_contains be a better fix? I think it would, because it makes tests more robust.

Member

matklad commented May 15, 2016

Hm, I found an interesting phenomena: some tests before this changed asserted stdout and ignored stderr although some warnings were printed to stderr. When converting with_stdout to with_stderr these test fail because there is more output. One way to fix them is to add warnings to the expected output, like I did in this commit. But is this really the best way? Could changing with_stderr to with_stderr_contains be a better fix? I think it would, because it makes tests more robust.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 16, 2016

Member

@matklad yeah that's probably a split between "intentionally ignored", "too lazy to write out the output", or "oops forgot to assert that output". It should be fine to either add the assertions or switch to with_stdout_contains.

Also cc @rust-lang/tools, a somewhat large-ish semantic change to Cargo!

Member

alexcrichton commented May 16, 2016

@matklad yeah that's probably a split between "intentionally ignored", "too lazy to write out the output", or "oops forgot to assert that output". It should be fine to either add the assertions or switch to with_stdout_contains.

Also cc @rust-lang/tools, a somewhat large-ish semantic change to Cargo!

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 16, 2016

Member

Also cc @rust-lang/tools, a somewhat large-ish semantic change to Cargo!

Nice, I indeed feel a little uneasy because of the scope of the change (Although I personally think that stderr is the right stream for the status messages). I'll then wait for an explicit approval before I continue fixing the tests :)

Member

matklad commented May 16, 2016

Also cc @rust-lang/tools, a somewhat large-ish semantic change to Cargo!

Nice, I indeed feel a little uneasy because of the scope of the change (Although I personally think that stderr is the right stream for the status messages). I'll then wait for an explicit approval before I continue fixing the tests :)

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc May 16, 2016

Member

+1 for more disciplined use of stderr/out

Member

nrc commented May 16, 2016

+1 for more disciplined use of stderr/out

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson May 17, 2016

Contributor

I agree that stderr is the right place for status messages. How will this affect cargo run -q (which I use because cargo prints to stdout)? I guess it will continue to not print things to stderr? Is that still useful?

Contributor

brson commented May 17, 2016

I agree that stderr is the right place for status messages. How will this affect cargo run -q (which I use because cargo prints to stdout)? I guess it will continue to not print things to stderr? Is that still useful?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 17, 2016

Member

@brson in theory it won't affect cargo run -q as Cargo will continue to not print anything.

Member

alexcrichton commented May 17, 2016

@brson in theory it won't affect cargo run -q as Cargo will continue to not print anything.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 18, 2016

Member

Discussed during tools triage we were all on board, so let's do this! Wanna fix up the few remaining tests @matklad and I'll merge?

Member

alexcrichton commented May 18, 2016

Discussed during tools triage we were all on board, so let's do this! Wanna fix up the few remaining tests @matklad and I'll merge?

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 18, 2016

Member

Wanna fix up the few remaining tests @matklad and I'll merge?

Yeah, I'll do it, but I can't give a precise ETA (this weak perheps?). The remaining tests most likely all require some manual intervention.

Member

matklad commented May 18, 2016

Wanna fix up the few remaining tests @matklad and I'll merge?

Yeah, I'll do it, but I can't give a precise ETA (this weak perheps?). The remaining tests most likely all require some manual intervention.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 18, 2016

Member

Ah yeah no worries, and if you run out of time I can help out as well

Member

alexcrichton commented May 18, 2016

Ah yeah no worries, and if you run out of time I can help out as well

matklad added some commits May 20, 2016

matklad added some commits May 20, 2016

@@ -722,7 +722,7 @@ test!(dev_deps_no_rebuild_lib {
"#)
.file("src/lib.rs", r#"
#[cfg(test)] extern crate bar;
#[cfg(not(test))] fn foo() { env!("FOO"); }
#[cfg(not(test))] pub fn foo() { env!("FOO"); }

This comment has been minimized.

@matklad

matklad May 20, 2016

Member

@alexcrichton this tests looks a bit strange. Looks like it should set and read an environmental variable, but it actually is never asserted (and "unused function foo" is printed during the compilation).

Perhaps the test doesn't test what it is supposed to?

@matklad

matklad May 20, 2016

Member

@alexcrichton this tests looks a bit strange. Looks like it should set and read an environmental variable, but it actually is never asserted (and "unused function foo" is printed during the compilation).

Perhaps the test doesn't test what it is supposed to?

This comment has been minimized.

@alexcrichton

alexcrichton May 20, 2016

Member

I believe the env! macro fails compilation if the env var isn't defined

@alexcrichton

alexcrichton May 20, 2016

Member

I believe the env! macro fails compilation if the env var isn't defined

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 20, 2016

Member

@alexcrichton I think I have fixed all the tests except the cross compile ones (one does not simply link stuff on nixos :) ). Can you please finish the work here? I think the commits should be squashed before merge, but it may be simpler to review them one by one.

Member

matklad commented May 20, 2016

@alexcrichton I think I have fixed all the tests except the cross compile ones (one does not simply link stuff on nixos :) ). Can you please finish the work here? I think the commits should be squashed before merge, but it may be simpler to review them one by one.

bors added a commit that referenced this pull request May 20, 2016

Auto merge of #2722 - alexcrichton:stderr-not-stdout, r=alexcrichton
Report status to stderr -- last few tests

Fixup of #2693 for the last few tests.

bors added a commit that referenced this pull request May 20, 2016

Auto merge of #2722 - alexcrichton:stderr-not-stdout, r=alexcrichton
Report status to stderr -- last few tests

Fixup of #2693 for the last few tests.

bors added a commit that referenced this pull request May 20, 2016

Auto merge of #2722 - alexcrichton:stderr-not-stdout, r=alexcrichton
Report status to stderr -- last few tests

Fixup of #2693 for the last few tests.

@bors bors merged commit f2eb995 into rust-lang:master May 20, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@euclio euclio referenced this pull request Jun 8, 2016

Open

Fix coverage subcommand #55

@matklad matklad deleted the matklad:status-2-stderr branch Jun 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment