Bug 1800: only open terminfo once

[rbtcollins]

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.

[rbtcollins]

I'm not sure why the tests are failing; they are passing for me :/.

[kinnison]

All the failures appear to be in cli-exact which is ultra-sensitive to changes in output format. it's possible there are newline or whitespace changes which are affected by your change, but I'll have to look harder to find out. I've kicked off a test run on my laptop to see if I can spot any difference.

[kinnison]

I can confirm i see the same errors on my laptop so I'll try to diagnose later today.

[kinnison]

Looks like there're formatting codes present in the output even under test...

I changed print_indented in clitools.rs to debug-print things and:

out.stdout (3 lines):
    "\n      nightly-x86_64-unknown-linux-gnu unchanged\u{1b}(B\u{1b}[m - 1.3.0 (hash-n-2)\n    \n    "
out.stderr (1 lines):
    "info: \u{1b}(B\u{1b}[msyncing channel updates for \'nightly-x86_64-unknown-linux-gnu\'\n    "
expected.stdout (3 lines):
    "\n      nightly-x86_64-unknown-linux-gnu unchanged - 1.3.0 (hash-n-2)\n    \n    "
expected.stderr (1 lines):
    "info: syncing channel updates for \'nightly-x86_64-unknown-linux-gnu\'\n    "

[rbtcollins]

Thanks; that will be a bug in the isatty detection glue then I guess - as the tests are passing for me under vscode on windows. Will look in ~48 hours.

[kinnison]

Mostly LGTM. A few comments / tweaks, and then I think we could merge. I like the caching, I like the split out of the markdown stuff (particularly since we've had some discussion about moving to a different markdown crate in the future too)

[rbtcollins]

What needs to happen to merge this?

[jens1o]

I'd be also interested in this, so I can really start working on #1818 without having merge conflicts in the end :/

[rbtcollins]

I have a followup that will likely conflict with 1818 as well; - the reason download_tracker didn't use term2 was that term2 was in the cli package, but the download_tracker was in the root, and I presume we don't want the root depending on the CLI.

[rbtcollins]

(that branch is https://github.com/rbtcollins/rustup.rs/pull/new/tracker-term-use ) - I'll set up the PR once this merges.

[kinnison]

Sorry for the delay :D