From 9d2164a84262698fb787bd2731519097c322a530 Mon Sep 17 00:00:00 2001 From: Robert Collins Date: Tue, 30 Apr 2019 08:52:48 +1200 Subject: [PATCH] Bug 1800: only open terminfo once I did a slightly more than minimal fix here, promoting the term::Terminal trait to be the contract that our markdown formatting is written to, making the auto-disabling of formatting on non-tty's and handling of non-colour terminals be clearly separated out. The handling of corrupt or missing terminfo configuration is also pushed down to the underlying terminal trait object rather than having a bypass in our higher level code; this makes the upper layers easier to reason about. There is a modest increase in size, as the full term::Terminal trait is now implemented for the translation layer; a decorator macro could probably be writtent to shrink that if needed. There is a remaining direct use of term in download_tracker that causes a second open of the terminfo database, which should also be fixed but it is constant in nature so a later pass can do it. I don't have any tests for this; doing a unit test would be uninteresting in this case - rust's types give use what we need at that level, and for the conceptual level 'nothing else in the system calls any term calls', unit tests are inappropriate. A good way to test this would be to have the functional tests instrumented and make assertions about the behaviour of those executions; then any future regressions (e.g. if switching to a different term library) would be detected. --- src/cli/common.rs | 1 + src/cli/log.rs | 1 + src/cli/main.rs | 1 + src/cli/markdown.rs | 184 ++++++++++++++++++ src/cli/rustup_mode.rs | 3 +- src/cli/self_update.rs | 12 +- src/cli/term2.rs | 427 ++++++++++++++++++----------------------- src/utils/tty.rs | 2 +- 8 files changed, 384 insertions(+), 247 deletions(-) create mode 100644 src/cli/markdown.rs diff --git a/src/cli/common.rs b/src/cli/common.rs index 40de32802e..52620fd6d4 100644 --- a/src/cli/common.rs +++ b/src/cli/common.rs @@ -14,6 +14,7 @@ use std::process::{Command, Stdio}; use std::sync::Arc; use std::time::Duration; use std::{cmp, iter}; +use term2::Terminal; use wait_timeout::ChildExt; pub fn confirm(question: &str, default: bool) -> Result { diff --git a/src/cli/log.rs b/src/cli/log.rs index 4e7a07462d..aa45124396 100644 --- a/src/cli/log.rs +++ b/src/cli/log.rs @@ -1,6 +1,7 @@ use crate::term2; use std::fmt; use std::io::Write; +use term2::Terminal; macro_rules! warn { ( $ ( $ arg : tt ) * ) => ( $crate::log::warn_fmt ( format_args ! ( $ ( $ arg ) * ) ) ) diff --git a/src/cli/main.rs b/src/cli/main.rs index 1315a12ac5..910a44dd51 100644 --- a/src/cli/main.rs +++ b/src/cli/main.rs @@ -22,6 +22,7 @@ mod download_tracker; mod errors; mod help; mod job; +mod markdown; mod proxy_mode; mod rustup_mode; mod self_update; diff --git a/src/cli/markdown.rs b/src/cli/markdown.rs new file mode 100644 index 0000000000..a5f496c906 --- /dev/null +++ b/src/cli/markdown.rs @@ -0,0 +1,184 @@ +// Write Markdown to the terminal + +use crate::term2::{color, Attr, Terminal}; +use markdown::tokenize; +use markdown::{Block, ListItem, Span}; +use std::io; + +// Handles the wrapping of text written to the console +struct LineWrapper<'a, T: Terminal> { + indent: u32, + margin: u32, + pos: u32, + pub w: &'a mut T, +} + +impl<'a, T: Terminal + 'a> LineWrapper<'a, T> { + // Just write a newline + fn write_line(&mut self) { + let _ = writeln!(self.w); + // Reset column position to start of line + self.pos = 0; + } + // Called before writing text to ensure indent is applied + fn write_indent(&mut self) { + if self.pos == 0 { + // Write a space for each level of indent + for _ in 0..self.indent { + let _ = write!(self.w, " "); + } + self.pos = self.indent; + } + } + // Write a non-breaking word + fn write_word(&mut self, word: &str) { + // Ensure correct indentation + self.write_indent(); + let word_len = word.len() as u32; + + // If this word goes past the margin + if self.pos + word_len > self.margin { + // And adding a newline would give us more space + if self.pos > self.indent { + // Then add a newline! + self.write_line(); + self.write_indent(); + } + } + + // Write the word + let _ = write!(self.w, "{}", word); + self.pos += word_len; + } + fn write_space(&mut self) { + if self.pos > self.indent { + if self.pos < self.margin { + self.write_word(" "); + } else { + self.write_line(); + } + } + } + // Writes a span of text which wraps at the margin + fn write_span(&mut self, text: &str) { + // Allow words to wrap on whitespace + let mut is_first = true; + for word in text.split(char::is_whitespace) { + if is_first { + is_first = false; + } else { + self.write_space(); + } + self.write_word(word); + } + } + // Constructor + fn new(w: &'a mut T, indent: u32, margin: u32) -> Self { + LineWrapper { + indent, + margin, + pos: indent, + w, + } + } +} + +// Handles the formatting of text +struct LineFormatter<'a, T: Terminal + io::Write> { + wrapper: LineWrapper<'a, T>, + attrs: Vec, +} + +impl<'a, T: Terminal + io::Write + 'a> LineFormatter<'a, T> { + fn new(w: &'a mut T, indent: u32, margin: u32) -> Self { + LineFormatter { + wrapper: LineWrapper::new(w, indent, margin), + attrs: Vec::new(), + } + } + fn push_attr(&mut self, attr: Attr) { + self.attrs.push(attr); + let _ = self.wrapper.w.attr(attr); + } + fn pop_attr(&mut self) { + self.attrs.pop(); + let _ = self.wrapper.w.reset(); + for attr in &self.attrs { + let _ = self.wrapper.w.attr(*attr); + } + } + fn do_spans(&mut self, spans: Vec) { + for span in spans { + match span { + Span::Break => {} + Span::Text(text) => { + self.wrapper.write_span(&text); + } + Span::Code(code) => { + self.push_attr(Attr::Bold); + self.wrapper.write_word(&code); + self.pop_attr(); + } + Span::Emphasis(spans) => { + self.push_attr(Attr::ForegroundColor(color::BRIGHT_RED)); + self.do_spans(spans); + self.pop_attr(); + } + _ => {} + } + } + } + fn do_block(&mut self, b: Block) { + match b { + Block::Header(spans, _) => { + self.push_attr(Attr::Bold); + self.wrapper.write_line(); + self.do_spans(spans); + self.wrapper.write_line(); + self.pop_attr(); + } + Block::CodeBlock(code) => { + self.wrapper.write_line(); + self.wrapper.indent += 2; + for line in code.lines() { + // Don't word-wrap code lines + self.wrapper.write_word(line); + self.wrapper.write_line(); + } + self.wrapper.indent -= 2; + } + Block::Paragraph(spans) => { + self.wrapper.write_line(); + self.do_spans(spans); + self.wrapper.write_line(); + } + Block::UnorderedList(items) => { + self.wrapper.write_line(); + for item in items { + self.wrapper.indent += 2; + match item { + ListItem::Simple(spans) => { + self.do_spans(spans); + } + ListItem::Paragraph(blocks) => { + for block in blocks { + self.do_block(block); + } + } + } + self.wrapper.write_line(); + self.wrapper.indent -= 2; + } + } + _ => {} + } + } +} + +pub fn md<'a, S: AsRef, T: Terminal + io::Write + 'a>(t: &'a mut T, content: S) { + let mut f = LineFormatter::new(t, 0, 79); + let blocks = tokenize(content.as_ref()); + for b in blocks { + f.do_block(b); + } +} diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index 2f5243a5e6..a3dee0760c 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -3,6 +3,7 @@ use crate::errors::*; use crate::help::*; use crate::self_update; use crate::term2; +use crate::term2::Terminal; use clap::{App, AppSettings, Arg, ArgGroup, ArgMatches, Shell, SubCommand}; use rustup::dist::dist::{PartialTargetTriple, PartialToolchainDesc, TargetTriple}; use rustup::dist::manifest::Component; @@ -807,7 +808,7 @@ fn show(cfg: &Cfg) -> Result<()> { } } - fn print_header(t: &mut term2::Terminal, s: &str) -> Result<()> { + fn print_header(t: &mut term::StdoutTerminal, s: &str) -> Result<()> { t.attr(term2::Attr::Bold)?; writeln!(t, "{}", s)?; writeln!(t, "{}", iter::repeat("-").take(s.len()).collect::())?; diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 555821d188..f089abc8b2 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -32,6 +32,7 @@ use crate::common::{self, Confirm}; use crate::errors::*; +use crate::markdown::md; use crate::term2; use rustup::dist::dist; use rustup::utils::utils; @@ -227,11 +228,12 @@ pub fn install(no_prompt: bool, verbose: bool, mut opts: InstallOpts) -> Result< check_existence_of_rustc_or_cargo_in_path(no_prompt)?; do_anti_sudo_check(no_prompt)?; + let mut term = term2::stdout(); if !do_msvc_check(&opts)? { if no_prompt { warn!("installing msvc toolchain without its prerequisites"); } else { - term2::stdout().md(MSVC_MESSAGE); + md(&mut term, MSVC_MESSAGE); if !common::confirm("\nContinue? (Y/n)", true)? { info!("aborting installation"); return Ok(()); @@ -242,10 +244,10 @@ pub fn install(no_prompt: bool, verbose: bool, mut opts: InstallOpts) -> Result< if !no_prompt { let msg = pre_install_msg(opts.no_modify_path)?; - term2::stdout().md(msg); + md(&mut term, msg); loop { - term2::stdout().md(current_install_opts(&opts)); + md(&mut term, current_install_opts(&opts)); match common::confirm_advanced()? { Confirm::No => { info!("aborting installation"); @@ -312,7 +314,7 @@ pub fn install(no_prompt: bool, verbose: bool, mut opts: InstallOpts) -> Result< cargo_home = cargo_home ) }; - term2::stdout().md(msg); + md(&mut term, msg); if !no_prompt { // On windows, where installation happens in a console @@ -745,7 +747,7 @@ pub fn uninstall(no_prompt: bool) -> Result<()> { if !no_prompt { println!(); let msg = format!(pre_uninstall_msg!(), cargo_home = canonical_cargo_home()?); - term2::stdout().md(msg); + md(&mut term2::stdout(), msg); if !common::confirm("\nContinue? (y/N)", false)? { info!("aborting uninstallation"); return Ok(()); diff --git a/src/cli/term2.rs b/src/cli/term2.rs index f3f6ed8bd9..b295164f27 100644 --- a/src/cli/term2.rs +++ b/src/cli/term2.rs @@ -2,250 +2,131 @@ //! that does not fail if `StdoutTerminal` etc can't be constructed, which happens //! if TERM isn't defined. -use markdown::tokenize; -use markdown::{Block, ListItem, Span}; +use lazy_static::lazy_static; use rustup::utils::tty; use std::io; +use std::sync::Mutex; pub use term::color; pub use term::Attr; +pub use term::Terminal; -pub trait Instantiable { - fn instance() -> Self; -} +mod termhack { + // Things we should submit to term as improvements: here temporarily. + use std::collections::HashMap; + use std::io; + use term::terminfo::TermInfo; + #[cfg(windows)] + use term::WinConsole; + use term::{StderrTerminal, StdoutTerminal, Terminal, TerminfoTerminal}; -impl Instantiable for io::Stdout { - fn instance() -> Self { - io::stdout() + // Works around stdio instances being unclonable. + pub trait Instantiable { + fn instance() -> Self; } -} -impl Instantiable for io::Stderr { - fn instance() -> Self { - io::stderr() + impl Instantiable for io::Stdout { + fn instance() -> Self { + io::stdout() + } } -} - -pub trait Isatty { - fn isatty() -> bool; -} -impl Isatty for io::Stdout { - fn isatty() -> bool { - tty::stdout_isatty() + impl Instantiable for io::Stderr { + fn instance() -> Self { + io::stderr() + } } -} -impl Isatty for io::Stderr { - fn isatty() -> bool { - tty::stderr_isatty() + /// Return a Terminal object for T on this platform. + /// If there is no terminfo and the platform requires terminfo, then None is returned. + fn make_terminal(terminfo: Option) -> Option + Send>> + where + T: 'static + io::Write + Send + Instantiable, + { + let mut result = terminfo + .map(move |ti| TerminfoTerminal::new_with_terminfo(T::instance(), ti.clone())) + .map(|t| Box::new(t) as Box + Send>); + #[cfg(windows)] + { + result = result.or_else(|| { + WinConsole::new(T::instance()) + .ok() + .map(|t| Box::new(t) as Box + Send>) + }) + } + result } -} - -pub struct Terminal(Option + Send>>) -where - T: Instantiable + Isatty + io::Write; -pub type StdoutTerminal = Terminal; -pub type StderrTerminal = Terminal; - -pub fn stdout() -> StdoutTerminal { - Terminal(term::stdout()) -} - -pub fn stderr() -> StderrTerminal { - Terminal(term::stderr()) -} -// Handles the wrapping of text written to the console -struct LineWrapper<'a, T: io::Write> { - indent: u32, - margin: u32, - pos: u32, - pub w: &'a mut T, -} - -impl<'a, T: io::Write + 'a> LineWrapper<'a, T> { - // Just write a newline - fn write_line(&mut self) { - let _ = writeln!(self.w); - // Reset column position to start of line - self.pos = 0; + fn make_terminal_with_fallback( + terminfo: Option, + ) -> Box + Send> + where + T: 'static + io::Write + Send + Instantiable, + { + make_terminal(terminfo) + .or_else(|| { + let ti = TermInfo { + names: vec![], + bools: HashMap::new(), + numbers: HashMap::new(), + strings: HashMap::new(), + }; + let t = TerminfoTerminal::new_with_terminfo(T::instance(), ti); + Some(Box::new(t) as Box + Send>) + }) + .unwrap() } - // Called before writing text to ensure indent is applied - fn write_indent(&mut self) { - if self.pos == 0 { - // Write a space for each level of indent - for _ in 0..self.indent { - let _ = write!(self.w, " "); - } - self.pos = self.indent; - } + /// Return a Terminal wrapping stdout, or None if a terminal couldn't be + /// opened. + #[allow(unused)] + pub fn stdout(terminfo: Option) -> Option> { + make_terminal(terminfo) } - // Write a non-breaking word - fn write_word(&mut self, word: &str) { - // Ensure correct indentation - self.write_indent(); - let word_len = word.len() as u32; - - // If this word goes past the margin - if self.pos + word_len > self.margin { - // And adding a newline would give us more space - if self.pos > self.indent { - // Then add a newline! - self.write_line(); - self.write_indent(); - } - } - // Write the word - let _ = write!(self.w, "{}", word); - self.pos += word_len; + /// Return a Terminal wrapping stderr, or None if a terminal couldn't be + /// opened. + #[allow(unused)] + pub fn stderr(terminfo: Option) -> Option> { + make_terminal(terminfo) } - fn write_space(&mut self) { - if self.pos > self.indent { - if self.pos < self.margin { - self.write_word(" "); - } else { - self.write_line(); - } - } - } - // Writes a span of text which wraps at the margin - fn write_span(&mut self, text: &str) { - // Allow words to wrap on whitespace - let mut is_first = true; - for word in text.split(char::is_whitespace) { - if is_first { - is_first = false; - } else { - self.write_space(); - } - self.write_word(word); - } + + /// Return a Terminal wrapping stdout. + pub fn stdout_with_fallback(terminfo: Option) -> Box { + make_terminal_with_fallback(terminfo) } - // Constructor - fn new(w: &'a mut T, indent: u32, margin: u32) -> Self { - LineWrapper { - indent, - margin, - pos: indent, - w, - } + + /// Return a Terminal wrapping stderr. + pub fn stderr_with_fallback(terminfo: Option) -> Box { + make_terminal_with_fallback(terminfo) } } -// Handles the formatting of text -struct LineFormatter<'a, T: Instantiable + Isatty + io::Write> { - wrapper: LineWrapper<'a, Terminal>, - attrs: Vec, +pub trait Isatty { + fn isatty() -> bool; } -impl<'a, T: Instantiable + Isatty + io::Write + 'a> LineFormatter<'a, T> { - fn new(w: &'a mut Terminal, indent: u32, margin: u32) -> Self { - LineFormatter { - wrapper: LineWrapper::new(w, indent, margin), - attrs: Vec::new(), - } - } - fn push_attr(&mut self, attr: Attr) { - self.attrs.push(attr); - let _ = self.wrapper.w.attr(attr); - } - fn pop_attr(&mut self) { - self.attrs.pop(); - let _ = self.wrapper.w.reset(); - for attr in &self.attrs { - let _ = self.wrapper.w.attr(*attr); - } - } - fn do_spans(&mut self, spans: Vec) { - for span in spans { - match span { - Span::Break => {} - Span::Text(text) => { - self.wrapper.write_span(&text); - } - Span::Code(code) => { - self.push_attr(Attr::Bold); - self.wrapper.write_word(&code); - self.pop_attr(); - } - Span::Emphasis(spans) => { - self.push_attr(Attr::ForegroundColor(color::BRIGHT_RED)); - self.do_spans(spans); - self.pop_attr(); - } - _ => {} - } - } - } - fn do_block(&mut self, b: Block) { - match b { - Block::Header(spans, _) => { - self.push_attr(Attr::Bold); - self.wrapper.write_line(); - self.do_spans(spans); - self.wrapper.write_line(); - self.pop_attr(); - } - Block::CodeBlock(code) => { - self.wrapper.write_line(); - self.wrapper.indent += 2; - for line in code.lines() { - // Don't word-wrap code lines - self.wrapper.write_word(line); - self.wrapper.write_line(); - } - self.wrapper.indent -= 2; - } - Block::Paragraph(spans) => { - self.wrapper.write_line(); - self.do_spans(spans); - self.wrapper.write_line(); - } - Block::UnorderedList(items) => { - self.wrapper.write_line(); - for item in items { - self.wrapper.indent += 2; - match item { - ListItem::Simple(spans) => { - self.do_spans(spans); - } - ListItem::Paragraph(blocks) => { - for block in blocks { - self.do_block(block); - } - } - } - self.wrapper.write_line(); - self.wrapper.indent -= 2; - } - } - _ => {} - } +impl Isatty for io::Stdout { + fn isatty() -> bool { + tty::stdout_isatty() } } -impl io::Write for Terminal { - fn write(&mut self, buf: &[u8]) -> Result { - if let Some(t) = self.0.as_mut() { - t.write(buf) - } else { - let mut t = T::instance(); - t.write(buf) - } - } - - fn flush(&mut self) -> Result<(), io::Error> { - if let Some(t) = self.0.as_mut() { - t.flush() - } else { - let mut t = T::instance(); - t.flush() - } +impl Isatty for io::Stderr { + fn isatty() -> bool { + tty::stderr_isatty() } } +// Decorator to: +// - Disable all terminal controls on non-tty's +// - Swallow errors when we try to use features a terminal doesn't have +// such as setting colours when no TermInfo DB is present +pub struct AutomationFriendlyTerminal(Box + Send>) +where + T: Isatty + io::Write; +pub type StdoutTerminal = AutomationFriendlyTerminal; +pub type StderrTerminal = AutomationFriendlyTerminal; + macro_rules! swallow_unsupported { ( $call:expr ) => {{ use term::Error::*; @@ -256,56 +137,122 @@ macro_rules! swallow_unsupported { }}; } -impl Terminal { - pub fn fg(&mut self, color: color::Color) -> Result<(), term::Error> { +impl term::Terminal for AutomationFriendlyTerminal +where + T: io::Write + Isatty, +{ + type Output = T; + + fn fg(&mut self, color: color::Color) -> term::Result<()> { if !T::isatty() { return Ok(()); } + swallow_unsupported!(self.0.fg(color)) + } - if let Some(t) = self.0.as_mut() { - swallow_unsupported!(t.fg(color)) - } else { - Ok(()) + fn bg(&mut self, color: color::Color) -> term::Result<()> { + if !T::isatty() { + return Ok(()); } + swallow_unsupported!(self.0.bg(color)) } - pub fn attr(&mut self, attr: Attr) -> Result<(), term::Error> { + fn attr(&mut self, attr: Attr) -> term::Result<()> { if !T::isatty() { return Ok(()); } - if let Some(t) = self.0.as_mut() { - if let Err(e) = t.attr(attr) { - // If `attr` is not supported, try to emulate it - match attr { - Attr::Bold => swallow_unsupported!(t.fg(color::BRIGHT_WHITE)), - _ => swallow_unsupported!(Err(e)), - } - } else { - Ok(()) + if let Err(e) = self.0.attr(attr) { + // If `attr` is not supported, try to emulate it + match attr { + Attr::Bold => swallow_unsupported!(self.0.fg(color::BRIGHT_WHITE)), + _ => swallow_unsupported!(Err(e)), } } else { Ok(()) } } - pub fn reset(&mut self) -> Result<(), term::Error> { + fn supports_attr(&self, attr: Attr) -> bool { + self.0.supports_attr(attr) + } + + fn reset(&mut self) -> term::Result<()> { if !T::isatty() { return Ok(()); } + swallow_unsupported!(self.0.reset()) + } - if let Some(t) = self.0.as_mut() { - swallow_unsupported!(t.reset()) - } else { - Ok(()) - } + /// Returns true if reset is supported. + fn supports_reset(&self) -> bool { + self.0.supports_reset() + } + + fn supports_color(&self) -> bool { + self.0.supports_color() } - pub fn md>(&mut self, content: S) { - let mut f = LineFormatter::new(self, 0, 79); - let blocks = tokenize(content.as_ref()); - for b in blocks { - f.do_block(b); + fn cursor_up(&mut self) -> term::Result<()> { + if !T::isatty() { + return Ok(()); } + + swallow_unsupported!(self.0.cursor_up()) + } + + fn delete_line(&mut self) -> term::Result<()> { + swallow_unsupported!(self.0.delete_line()) + } + + fn carriage_return(&mut self) -> term::Result<()> { + // This might leak control chars in !isatty? needs checking. + swallow_unsupported!(self.0.carriage_return()) + } + + fn get_ref(&self) -> &Self::Output { + self.0.get_ref() + } + + fn get_mut(&mut self) -> &mut Self::Output { + self.0.get_mut() + } + + /// Returns the contained stream, destroying the `Terminal` + fn into_inner(self) -> Self::Output + where + Self: Sized, + { + unimplemented!() + // self.0.into_inner().into_inner() + } +} + +impl io::Write for AutomationFriendlyTerminal { + fn write(&mut self, buf: &[u8]) -> Result { + self.0.write(buf) } + + fn flush(&mut self) -> Result<(), io::Error> { + self.0.flush() + } +} + +lazy_static! { + // Cache the terminfo database for performance. + // Caching the actual terminals may be better, as on Windows terminal + // detection is per-fd, but this at least avoids the IO subsystem and + // caching the stdout instances is more complex + static ref TERMINFO: Mutex> = + Mutex::new(term::terminfo::TermInfo::from_env().ok()); +} + +pub fn stdout() -> StdoutTerminal { + let info_result = TERMINFO.lock().unwrap().clone(); + AutomationFriendlyTerminal(termhack::stdout_with_fallback(info_result)) +} + +pub fn stderr() -> StderrTerminal { + let info_result = TERMINFO.lock().unwrap().clone(); + AutomationFriendlyTerminal(termhack::stderr_with_fallback(info_result)) } diff --git a/src/utils/tty.rs b/src/utils/tty.rs index 349e5d4526..241b1cce48 100644 --- a/src/utils/tty.rs +++ b/src/utils/tty.rs @@ -1,4 +1,4 @@ -// Copied from rustc. atty crate did not work as expected +// Copied from rustc. isatty crate did not work as expected #[cfg(unix)] pub fn stderr_isatty() -> bool { isatty(libc::STDERR_FILENO)