libterm: synchronize with public "term" crate v0.4.0 #31205

erickt · 2016-01-26T05:05:23Z

The largest impact is that the public "term" crate now has it's own term::Error type. Most of the changes are propagating this type through the other crates.

I also had a minor extraction of a subset of the "winapi" and "kernel32" crates.

cc @cmr, who made the original change to term.

rust-highfive · 2016-01-26T05:05:28Z

r? @aturon

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

erickt · 2016-01-26T05:06:27Z

I mainly want this so I can update syntex to use the latest term crate :) Keep a close eye on the Windows code. I wasn't able to test it out on my Mac.

Stebalien · 2016-01-26T16:54:59Z

FYI, in terminfo/searcher.rs:

get_dbpath_for_term probably doesn't need #[allow(deprecated)].
The test case (test_get_dbpath_for_term) is outdated, broken, disabled, and should probably be removed.

Stebalien · 2016-01-26T16:55:08Z

Otherwise, LGTM.

erickt · 2016-01-26T17:02:22Z

@Stebalien: Good point. I made both of your suggested changes in the latest patch.

bors · 2016-01-27T00:32:01Z

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

aturon · 2016-01-27T16:49:40Z

Hm, I'm marked as reviewer but know nothing about this crate. @alexcrichton, do you know of a good person to r? here?

alexcrichton · 2016-01-27T17:42:22Z

Yes I can take a look

alexcrichton · 2016-01-27T17:43:05Z

src/libsyntax/errors/emitter.rs

+    fn from(err: term::Error) -> Self {
+        Error::Term(err)
+    }
+}


This seems pretty unfortunate, shouldn't a term::Error be convertible into a io::Error?

This same duplication seems to happen in libtest which is what makes me wary

Yeah, I thought about that. I initially started by just adding:

impl From<term::Error> for io::Error { fn from(err: term::Error) -> io::Error { match err { term::Io(err) => err, err => io::Error::new(io::ErrorKind::Other, err), } } }

But I decided against that because it seems a shame that we're giving up the flexibility to do sensible things with the term errors, and making it slightly harder to add in custom emitter and libtest errors in the future. There's a side of me that just wants us to not get into the habit of stuffing things into error types like io::Error. It probably doesn't really matter much in this case since these uses are internal to the compiler, but I'm trying to be a good role model :)

In terms of integration with libsyntax/libtest, however, it seems pretty onerous to always require a shim like this (or to merge any two libraries together)

Yep, it does get a bit obnoxious to setup the first time, and if we never take advantage of it, it does get a bit silly. I'm fine either way, assuming if we add this impl to term it's fine with @Stebalien.

Personally, I'd prefer the following so we don't loose any information:

impl From<term::Error> for io::Error { fn from(err: term::Error) -> io::Error { let kind = match &err { &term::Error::Io(ref e) => e.kind(), _ => ErrorKind::Other, }; io::Error::new(kind, err) } }

This way, we keep the fact that this IO error was caused by a term error.

Thoughts?

Sounds good to me!

bors · 2016-01-29T03:47:57Z

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

alexcrichton · 2016-01-29T22:38:01Z

If we get a conversion of term::Error -> io::Error then this all looks good to me, thanks @erickt!

Stebalien · 2016-01-29T22:53:25Z

@alexcrichton done in term 0.4.1.

alexcrichton · 2016-01-30T21:21:07Z

Note that @pnkfelix found a bug in what appears to be term when cargo itself upgraded, prompting a downgrade back to 0.2.

Along those lines we may want to be hesitant about aggressively updating to new code? @Stebalien could you speak to some of the bigger changes made between term 0.2 and 0.4?

Stebalien · 2016-01-30T21:34:20Z

@alexcrichton term used to return Ok(true) if something succeeded, Ok(false) if the operation was unsupported, and Err(io::Error) if there was an IO error. Now, it returns Ok(()) if the operation succeeds and Err(term::Error) if the operation fails. If the operation is unsupported, it returns Err(term::Error::NotSupported). I'm adding a supports_color() function that will allow crates to determine if the terminal supports color/reset up front. See Stebalien/term#56.

Stebalien · 2016-01-30T21:54:17Z

After Stebalien/term#56 is merged, the fix here would be to check if color is supported in syntax::errors::emitter::Destination::from_stderr.

Stebalien · 2016-01-30T21:55:26Z

I apologize for not catching this when initially reviewing this patch, I completely forgot about this change. Thanks @pnkfelix for noticing this.

alexcrichton · 2016-01-31T07:54:56Z

Ok, thanks for the summary @Stebalien! @erickt it looks like there's going to need to be some changes in that case? e.g. if we call methods like reset() we'll need a guard to see whether the terminal supports it at all

pnkfelix · 2016-02-01T10:58:23Z

I'm sort of surprised that I was seeing the behavior that I was observing given that the API change described was something where unsupported operations now lead to Err -- in particular, why was I still seeing a zero exit code in that case? (Update: I accidentally wrote "non-zero" in the first version of this comment.)

This is what I mean (from rust-lang/cargo#2338 (comment) ):

% /Users/fklock/.multirust/toolchains/nightly-2016-01-27/bin/cargo --version
% echo $?
0

this is the scenario where cargo --version was running under a terminal environment that is not as full-featured (presumably emacs M-x shell said it did not support color?). But why was cargo exiting with exit code zero in this scenario?

Stebalien · 2016-02-01T15:12:00Z

@pnkfelix it's probably just ignoring the errors as unimportant (printing to the screen is not critical) For example, the following would say nothing:

fn say_something(&self)  -> Result {
    try!(self.term.reset());
    write!(term, "something");
}

fn do_something_important() {
    let _ = sayer.say_something();
}

erickt · 2016-02-05T03:08:12Z

@Stebalien / @alexcrichton: I've updated my patch to sync with term v0.4.4, and got rid of the custom Error implementations. Rust built for me, but this patch is still untested on windows.

Stebalien · 2016-02-05T13:39:03Z

See #31205 (comment).

You need to check if the terminal supports color.

Steven Allen

alexcrichton · 2016-02-05T19:21:26Z

Looks good to me modulo what @Stebalien mentioned

The only point of divergence from the public crate is that this patch uses `std::sys::windows` instead of the `winapi` and `kernel32` crate to access the Windows Console APIs.

erickt · 2016-02-17T18:12:06Z

@Stebalien: This PR should now be synced with term, modulo the clippy work you did in the repo. Does that address your comment about checking the terminal if it supports color?

Stebalien · 2016-02-17T19:55:39Z

Not quite. You need to actually check for color support in syntax::errors::emitter::Destination::from_stderr. See Stebalien/cargo@cfcfa08 for how I did this in cargo.

erickt · 2016-02-18T04:24:42Z

@Stebalien: I added a patch that I think implements the check you were looking for. How does it look?

Stebalien · 2016-02-18T04:49:26Z

src/libsyntax/errors/emitter.rs

+                    Raw(Box::new(io::stderr()))
+                }
+            }
+            None => Raw(Box::new(io::stderr())),


alexcrichton · 2016-02-18T18:00:28Z

@bors: r+ 388cf4e

bors · 2016-02-18T19:26:50Z

⌛ Testing commit 388cf4e with merge c8c296a...

bors · 2016-02-18T19:42:00Z

💔 Test failed - auto-win-gnu-32-nopt-t

steveklabnik · 2016-02-22T22:19:14Z

src/libterm/win.rs

 use std::io::prelude::*;
+use std::io;
+use std::ptr;
+use std::sys::windows::c as kernel32;


looks like this stuff casued the build failure

../src/libterm\win.rs:18:5: 18:37 error: unresolved import `std::sys::windows::c`. Could not find `windows` in `std::sys` [E0432]

bors · 2016-03-23T19:39:57Z

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

alexcrichton · 2016-05-01T18:00:13Z

Closing due to inactivity, but seems good to merge with a rebase and test fixes!

rust-highfive assigned aturon Jan 26, 2016

erickt force-pushed the term branch from 8281abc to b9ed129 Compare January 27, 2016 00:33

alexcrichton assigned alexcrichton and unassigned aturon Jan 27, 2016

alexcrichton reviewed Jan 27, 2016
View reviewed changes

erickt force-pushed the term branch from b9ed129 to abca2b3 Compare February 5, 2016 01:11

erickt added 3 commits February 17, 2016 10:01

term: Remove unnecessary #[allow(deprecated)] and broken test

7f1b282

term: synchronize with public "term" crate v0.4.4

cc238a3

The only point of divergence from the public crate is that this patch uses `std::sys::windows` instead of the `winapi` and `kernel32` crate to access the Windows Console APIs.

test: Remove superflouous return

1602ca5

syntax: Replace glob import to reduce risk of name collisions

869a895

erickt force-pushed the term branch from abca2b3 to 869a895 Compare February 17, 2016 18:07

syntax: Only use color output if the terminal supports it

388cf4e

Stebalien reviewed Feb 18, 2016
View reviewed changes

src/libsyntax/errors/emitter.rs

Raw(Box::new(io::stderr()))

}

}

None => Raw(Box::new(io::stderr())),

Copy link

Contributor

Stebalien Feb 18, 2016

Choose a reason for hiding this comment

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

LGTM.

steveklabnik reviewed Feb 22, 2016
View reviewed changes

alexcrichton closed this May 1, 2016

libterm: synchronize with public "term" crate v0.4.0 #31205

libterm: synchronize with public "term" crate v0.4.0 #31205

Conversation

erickt commented Jan 26, 2016

rust-highfive commented Jan 26, 2016

erickt commented Jan 26, 2016

Stebalien commented Jan 26, 2016

Stebalien commented Jan 26, 2016

erickt commented Jan 26, 2016

bors commented Jan 27, 2016

aturon commented Jan 27, 2016

alexcrichton commented Jan 27, 2016

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

bors commented Jan 29, 2016

alexcrichton commented Jan 29, 2016

Stebalien commented Jan 29, 2016

alexcrichton commented Jan 30, 2016

Stebalien commented Jan 30, 2016

Stebalien commented Jan 30, 2016

Stebalien commented Jan 30, 2016

alexcrichton commented Jan 31, 2016

pnkfelix commented Feb 1, 2016

Stebalien commented Feb 1, 2016

erickt commented Feb 5, 2016

Stebalien commented Feb 5, 2016

You need to check if the terminal supports color.

alexcrichton commented Feb 5, 2016

erickt commented Feb 17, 2016

Stebalien commented Feb 17, 2016

erickt commented Feb 18, 2016

Choose a reason for hiding this comment

alexcrichton commented Feb 18, 2016

bors commented Feb 18, 2016

bors commented Feb 18, 2016

Choose a reason for hiding this comment

bors commented Mar 23, 2016

alexcrichton commented May 1, 2016