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 rustc-guide build failure ignoring no-fail-fast #63089

Merged
merged 1 commit into from Jul 29, 2019

Conversation

@kennytm
Copy link
Member

@kennytm kennytm commented Jul 28, 2019

No description provided.

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Jul 28, 2019

r? @nikomatsakis

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

Loading

@Centril
Copy link
Contributor

@Centril Centril commented Jul 28, 2019

Loading

@pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented Jul 28, 2019

Loading

builder.run(rustbook_cmd
.arg("linkcheck")
.arg(&src));
try_run(builder, rustbook_cmd.arg("linkcheck").arg(&src));
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Jul 28, 2019

Choose a reason for hiding this comment

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

We'll definitely want try_run_quiet here as try_run eats the command's output (a horrible and non-obvious default, I know -- we should fix that).

Loading

Copy link
Member

@RalfJung RalfJung Aug 1, 2019

Choose a reason for hiding this comment

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

Wait so quiet makes it not eat the output? That's, like, the opposite of what the name indicates.^^

Loading

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Aug 1, 2019

Choose a reason for hiding this comment

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

Yep, try_run internally calls run_silent which will eat all the output unconditionally, I believe.

Loading

Copy link
Member

@RalfJung RalfJung Aug 1, 2019

Choose a reason for hiding this comment

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

The one in lib.rs does, yeah. The one in test.rs calls builder.run.

So Builder::try_run should maybe just be renamed to try_run_silent, like the free-standing function it calls?

Loading

Copy link
Member

@RalfJung RalfJung Aug 1, 2019

Choose a reason for hiding this comment

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

Fun times, there's both free-standing run_silent (called by Builder::run) and run_suppressed (called by Builder::run_quiet).

Loading

Copy link
Member

@RalfJung RalfJung Aug 1, 2019

Choose a reason for hiding this comment

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

Actually, @Mark-Simulacrum are you sure about this? try_run_silent calls Command::status which says that

By default, stdin, stdout and stderr are inherited from the parent.

I have no idea why it is called "silent". There is no comment explaining that.

In contrast, run_suppressed (the helper behind Builder::run_quiet) calls Command::output, which does capture the output, and then throws it away (unless there was an error).

Loading

Copy link
Member

@RalfJung RalfJung Aug 1, 2019

Choose a reason for hiding this comment

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

See #63196: I think you are wrong and try_run does not eat the commends output. In contrast, try_run_quiet does eat (suppress) the output.

Loading

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum Aug 2, 2019

Choose a reason for hiding this comment

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

Hm, you could definitely be right! I don't have a good handle on these functions beyond "too many and bad naming" :)

Loading

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jul 28, 2019

r=me with that and the azure config change reverted

Loading

This caused clippy not being built on Linux previously.
@kennytm kennytm force-pushed the use-try-run-for-linkchecker branch from 20924d2 to ebd1bf7 Jul 29, 2019
@kennytm kennytm changed the title [WIP] Fix rustc-guide build failure ignoring no-fail-fast Fix rustc-guide build failure ignoring no-fail-fast Jul 29, 2019
@kennytm
Copy link
Member Author

@kennytm kennytm commented Jul 29, 2019

@bors r=Mark-Simulacrum

Loading

@bors
Copy link
Contributor

@bors bors commented Jul 29, 2019

📌 Commit ebd1bf7 has been approved by Mark-Simulacrum

Loading

@bors
Copy link
Contributor

@bors bors commented Jul 29, 2019

Testing commit ebd1bf7 with merge 04b88a9...

Loading

bors added a commit that referenced this issue Jul 29, 2019
…mulacrum

Fix rustc-guide build failure ignoring no-fail-fast
@bors
Copy link
Contributor

@bors bors commented Jul 29, 2019

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 04b88a9 to master...

Loading

@bors bors merged commit ebd1bf7 into rust-lang:master Jul 29, 2019
5 checks passed
Loading
@kennytm kennytm deleted the use-try-run-for-linkchecker branch Jul 29, 2019
Centril added a commit to Centril/rust that referenced this issue Aug 2, 2019
build_helper: try less confusing method names

build_helper's `*_silent` methods were likely called that way because they do not print the command being run to stdout. [In the original file this all makes sense](rust-lang@046e687#diff-5c3d6537a43ecae03014e118a7fe3321). But later it also gained `*_suppressed` methods and the difference between `silent` and `suppressed` is far from clear.

So rename `run` (which prints the command being run) to `run_verbose`. Then we can call the methods that just run a command and show its output but nothing extra `run` and `try_run`.

`run_verbose` (formerly `run`) is unused from what I can tell. Should I remove it?

r? @alexcrichton
Cc @Mark-Simulacrum
Also see rust-lang#63089 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants