Add pager support for `rustc --explain EXXXX` #42732

cengiz-io · 2017-06-18T14:27:53Z

Hello!

Fixes #32665.

Thanks!

EDIT: I've limited access to a Windows machine so this is taking longer than I've anticipated. 🐢

cc @alexcrichton @nikomatsakis @Mark-Simulacrum @retep998 @ollie27 @afiune

rust-highfive · 2017-06-18T14:27:58Z

r? @arielb1

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

cengiz-io · 2017-06-18T14:34:13Z

Oh and I had problems during switching stdout to another process other than rustc. That's why my implementation is using a temporary file. (Unwillingly). Maybe I'm missing something obvious.

Mark-Simulacrum · 2017-06-18T16:11:22Z

src/librustc_driver/lib.rs

+    use std::os::unix::io::FromRawFd;
+    let raw_file_descriptor_id = 2; // stdout
+
+    let pager = env::var("PAGER").unwrap_or(String::from("less"));


Since it's quite easy, could we use var_os here (and in the companion windows function)?

Mark-Simulacrum · 2017-06-18T16:12:44Z

src/librustc_driver/lib.rs

+        .expect("unable to write pager content to temporary file");
+
+    if spawn_pager_with_file(&file_path).is_err() {
+        println!("unable to spawn pager process");


hm, I think ideally we'd send this to stderr and (as in all other cases in this function) send the output to stdout. We shouldn't ever not show the output because a pager is missing.

Good catch!

carols10cents · 2017-06-19T13:04:43Z

Looks like this isn't compiling? https://travis-ci.org/rust-lang/rust/jobs/244227954#L1476

[00:14:23] error[E0433]: failed to resolve. Use of undeclared type or module `Stdio`
[00:14:23]    --> /checkout/src/librustc_driver/lib.rs:391:21
[00:14:23]     |
[00:14:23] 391 |             .stdout(Stdio::from_raw_fd(raw_file_descriptor_id))
[00:14:23]     |                     ^^^^^^^^^^^^^^^^^^ Use of undeclared type or module `Stdio`
[00:14:23] 
[00:14:23] error: aborting due to previous error

ollie27 · 2017-06-19T17:57:08Z

src/librustc_driver/lib.rs

+
+    Command::new(pager)
+        .arg(file_path)
+        .output()


I think the reason this is failing on Windows is the use of output() here. output() captures stdout which we definitely don't want, it should go to the console so using status() might be better. However as more.com and less accept input over stdin you should be able to do something like the following and avoid creating a file:

let mut child = Command::new(pager).stdin(Stdio::piped()).spawn()?; child.stdin.as_mut().unwrap().write_all(content.as_bytes())?; child.wait()

@ollie27 I have tried to use pipes with Command but the 'less' output was corrupted in Linux bash shell. Maybe it was something that I missed. Will try this first thing in the morning.

Thanks a lot

cengiz-io · 2017-06-22T22:02:51Z

@carols10cents thanks for the heads up

cengiz-io · 2017-06-22T22:07:20Z

It's been a very busy week. Sorry for the delay.

A revision will be ready next morning

bors · 2017-06-23T16:20:30Z

☔ The latest upstream changes (presumably #42856) made this pull request unmergeable. Please resolve the merge conflicts.

kennytm · 2017-06-28T22:47:45Z

LLVM and Cargo updated by mistake? Also, tidy error.

[00:03:00] tidy error: /checkout/src/librustc_driver/lib.rs:389: trailing whitespace
[00:03:00] tidy error: /checkout/src/librustc_driver/lib.rs:391: trailing whitespace
[00:03:00] tidy error: /checkout/src/librustc_driver/lib.rs:393: trailing whitespace

cengiz-io · 2017-06-28T23:16:55Z

@kennytm sorry. I was trying to update my fork only. thanks for notifying. fix is coming

kennytm · 2017-06-29T01:26:36Z

@cengizio Thanks. LLVM and Cargo submodules are still modified in 14b28d0af5c970414f8a8bb681da13b36d5fb658 though.

cengiz-io · 2017-06-29T01:29:21Z

@kennytm now it should be clear 😌

cengiz-io · 2017-06-29T01:32:31Z

src/librustc_driver/lib.rs

        }
        None => {
            early_error(output, &format!("no extended information for {}", code));
        }
    }
 }

+fn show_content_with_pager(content: &String) {
+    let pager_name = env::var_os("PAGER").unwrap_or(if cfg!(windows) {
+        OsString::from("more.com")


@Mark-Simulacrum I hope this is the right way of using OsStrings

Yes, I believe so, but unwrap_or_else please.

Why do we need a unwrap_or_else here?

OsString is heap allocated, and with unwrap_or the heap allocation always happens even if not necessary.

Makes sense now. Added!

cengiz-io · 2017-06-29T01:33:26Z

src/librustc_driver/lib.rs

+        }
+    }
+
+    // If pager fails for whatever reason, we should still print the content


@Mark-Simulacrum as you suggested, now we're falling back to plain printing whenever something goes wrong. And not printing anything to stdout if there's an error.

cengiz-io · 2017-06-29T01:34:10Z

src/librustc_driver/lib.rs

            // Slice off the leading newline and print.
            for line in description[1..].lines() {
                let indent_level = line.find(|c: char| !c.is_whitespace())
                    .unwrap_or_else(|| line.len());
                let dedented_line = &line[indent_level..];
                if dedented_line.starts_with("```") {
                    is_in_code_block = !is_in_code_block;
-                    println!("{}", &line[..(indent_level+3)]);
+                    text.push_str(&line[..(indent_level+3)]);
+                    text.push('\n');


@carols10cents maybe this can be prettier. Suggestions?

IMO It's ok to do that push(). Though you are doing it here and in like 375, so you are adding a carriage return regardless of the if statement, I would just put a single one between lines 376-377 instead, but that is totally a nitpick.

kennytm · 2017-06-29T02:58:17Z

src/librustc_driver/lib.rs

+    // If pager fails for whatever reason, we should still print the content
+    // to standard output
+    if fallback_to_println {
+        println!("{}", content);


UI test failure. Please either change this to print! or update src/test/ui/explain.stdout to include the new trailing \n.

@kennytm thanks again.

afiune · 2017-06-29T13:25:37Z

cengiz-io · 2017-06-29T15:04:56Z

src/librustc_driver/lib.rs

                }
+                text.push('\n');


@afiune 👍

cengiz-io · 2017-06-29T17:00:12Z

@carols10cents I think we can remove the waiting tag and proceed to reviewing

Mark-Simulacrum · 2017-06-29T17:13:06Z

@bors r+ rollup

bors · 2017-06-29T17:13:07Z

📌 Commit d2a0ead has been approved by Mark-Simulacrum

@alexcrichton

Add pager support for `rustc --explain EXXXX` Hello! This is my take on rust-lang#32665. Thanks! **EDIT:** _I've limited access to a Windows machine so this is taking longer than I've anticipated_. 🐢 cc @alexcrichton @nikomatsakis @Mark-Simulacrum @retep998 @ollie27 @afiune

kennytm · 2017-06-30T14:42:18Z

If this cannot be easily solved, I'd suggest // ignore-windows for this test, and later amend the UI test comparer to ignore difference of trailing whitespaces. This is not something that impacts UI too much.

@alexcrichton

Add pager support for `rustc --explain EXXXX` Hello! This is my take on rust-lang#32665. Thanks! **EDIT:** _I've limited access to a Windows machine so this is taking longer than I've anticipated_. 🐢 cc @alexcrichton @nikomatsakis @Mark-Simulacrum @retep998 @ollie27 @afiune

@alexcrichton

Add pager support for `rustc --explain EXXXX` Hello! This is my take on rust-lang#32665. Thanks! **EDIT:** _I've limited access to a Windows machine so this is taking longer than I've anticipated_. 🐢 cc @alexcrichton @nikomatsakis @Mark-Simulacrum @retep998 @ollie27 @afiune

Mark-Simulacrum · 2017-06-30T18:01:33Z

ahh @bors r-

ollie27 · 2017-06-30T18:01:56Z

Perhaps if rustc isn't outputting directly to terminal or console (if isatty returns false) then it shouldn't try to use a pager. I think that would fix the test.

cengiz-io · 2017-06-30T18:31:40Z

@ollie27 @Mark-Simulacrum @kennytm I'll modify the test as soon as possible. Thanks

cengiz-io · 2017-07-02T20:29:33Z

src/librustc_driver/Cargo.toml

@@ -35,3 +35,4 @@ serialize = { path = "../libserialize" }
 syntax = { path = "../libsyntax" }
 syntax_ext = { path = "../libsyntax_ext" }
 syntax_pos = { path = "../libsyntax_pos" }
+isatty = "0.1"


@Mark-Simulacrum I'm not 100% sure about the version here. Should I make it strict?

Cargo.lock will handle that for you. I don't know if you need this dependency though, I believe you can find equivalent code in rustbuild (search for cfg(Windows)) and possibly in librustc_errors... I'd prefer not to depend on isatty since that means winapi which currently has long build times.

cengiz-io · 2017-07-02T20:33:32Z

@kennytm @ollie27 @Mark-Simulacrum I've added istty crate and used its stdout check.

I've only tried it with Windows. *nix part needs to be tested.

Please feel free to comment on anything.

Thank you

Mark-Simulacrum · 2017-07-02T22:42:47Z

src/librustc_driver/lib.rs

+ * These are duplicated in
+ *   - bootstrap/compile.rs#L478
+ *   - librustc_errors/emitter.rs#L1253
+ */


Could you use a line comments here? Also, you'll need to remove isatty from Cargo.lock/Cargo.toml.

cengiz-io · 2017-07-03T13:54:33Z

Ok, tty check is done with an embedded logic.

cengiz-io · 2017-07-04T21:09:54Z

@Mark-Simulacrum I've tested with *nix and it behaves correctly. We can review and proceed accordingly.

Mark-Simulacrum · 2017-07-04T21:12:49Z

Looks good to me. Thanks!

@bors r+ rollup

bors · 2017-07-04T21:12:50Z

📌 Commit 06de114 has been approved by Mark-Simulacrum

bors · 2017-07-04T23:50:17Z

⌛ Testing commit 06de114 with merge 1421bed...

@alexcrichton

Add pager support for `rustc --explain EXXXX` Hello! Fixes #32665. Thanks! **EDIT:** _I've limited access to a Windows machine so this is taking longer than I've anticipated_. 🐢 cc @alexcrichton @nikomatsakis @Mark-Simulacrum @retep998 @ollie27 @afiune

bors · 2017-07-05T02:04:01Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 1421bed to master...

rust-highfive assigned arielb1 Jun 18, 2017

Mark-Simulacrum reviewed Jun 18, 2017

View reviewed changes

carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 19, 2017

ollie27 reviewed Jun 19, 2017

View reviewed changes

cengiz-io commented Jun 29, 2017

View reviewed changes

kennytm reviewed Jun 29, 2017

View reviewed changes

cengiz-io commented Jun 29, 2017

View reviewed changes

src/librustc_driver/lib.rs Outdated

}

text.push('\n');

Copy link

Contributor Author

cengiz-io Jun 29, 2017

Choose a reason for hiding this comment

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

@afiune 👍

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 29, 2017

Mark-Simulacrum added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 29, 2017

frewsxcv mentioned this pull request Jun 30, 2017

Rollup of 4 pull requests #42978

Closed

GuillaumeGomez mentioned this pull request Jun 30, 2017

Rollup of 4 pull requests #42989

Closed

GuillaumeGomez mentioned this pull request Jun 30, 2017

Rollup of 7 pull requests #42990

Closed

shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 30, 2017

cengiz-io commented Jul 2, 2017

View reviewed changes

Mark-Simulacrum reviewed Jul 2, 2017

View reviewed changes

cengiz-io and others added 7 commits July 3, 2017 14:13

use PAGER to view --explain output #32665

3146e55

do not append an extra newline char

1beeb5a

use unwrap_or_else to prevent unnecessary alloc

08b6beb

do not spawn pager if not tty

7b0a7fd

use embedded implementation instead of istty crate

0e18a9c

use single line comments

e1a9144

remove isatty dependency

06de114

bors merged commit 06de114 into rust-lang:master Jul 5, 2017

camelid mentioned this pull request Jan 11, 2021

driver: Use atty instead of rolling our own #80897

Merged

jyn514 mentioned this pull request Jan 12, 2021

Use atty crate in libtest instead of rolling our own #80937

Closed

Add pager support for rustc --explain EXXXX #42732

Add pager support for rustc --explain EXXXX #42732

Conversation

cengiz-io commented Jun 18, 2017 • edited by Mark-Simulacrum Loading

rust-highfive commented Jun 18, 2017

cengiz-io commented Jun 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cengiz-io commented Jun 22, 2017

cengiz-io commented Jun 22, 2017

bors commented Jun 23, 2017

kennytm commented Jun 28, 2017

cengiz-io commented Jun 28, 2017

kennytm commented Jun 29, 2017

cengiz-io commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afiune commented Jun 29, 2017

Choose a reason for hiding this comment

cengiz-io commented Jun 29, 2017

Mark-Simulacrum commented Jun 29, 2017

bors commented Jun 29, 2017

kennytm commented Jun 30, 2017

Mark-Simulacrum commented Jun 30, 2017

ollie27 commented Jun 30, 2017 • edited Loading

cengiz-io commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cengiz-io commented Jul 2, 2017

Choose a reason for hiding this comment

cengiz-io commented Jul 3, 2017

cengiz-io commented Jul 4, 2017

Mark-Simulacrum commented Jul 4, 2017

bors commented Jul 4, 2017

bors commented Jul 4, 2017

bors commented Jul 5, 2017

Add pager support for `rustc --explain EXXXX` #42732

Add pager support for `rustc --explain EXXXX` #42732

cengiz-io commented Jun 18, 2017 •

edited by Mark-Simulacrum

Loading

kennytm Jun 29, 2017 •

edited

Loading

ollie27 commented Jun 30, 2017 •

edited

Loading