test: skip terminfo parsing in Miri #100206
Conversation
rustbot
added
the
T-libs
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
r? @thomcc (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -119,13 +130,22 @@ pub(crate) struct TerminfoTerminal<T> { | |||
| impl<T: Write + Send> Terminal for TerminfoTerminal<T> { | |||
| fn fg(&mut self, color: color::Color) -> io::Result<bool> { | |||
| let color = self.dim_if_necessary(color); | |||
| if cfg!(miri) && color < 8 { | |||
There was a problem hiding this comment.
It's not obvious to me what happens if color = 9. The logic for num_colors isn't totally straightforward, but I think in Miri we end up with num_colors = 0, right? Could we explain this in a comment?
There was a problem hiding this comment.
Yeah num_colors is always 0 in Miri. I added a comment.
|
Wow this is a lot less code than I expected! |
| // The Miri logic for this only works for the most basic 8 colors, which we just assume | ||
| // the terminal will support. (`num_colors` is always 0 in Miri, so higher colors will | ||
| // just fail. But libtest doesn't use any higher colors anyway.) | ||
| let s = format!("\x1B[3{color}m"); |
There was a problem hiding this comment.
Huh, slightly surprised this is more efficient under miri than something like write!(self.out, "\x1B[3{color}m").
There was a problem hiding this comment.
I don't think it is more efficient. But Miri isn't that slow, a bit of formatting is fine.
The terminfo parser is doing a lot of work and constructing a huge data structure to represent the parsed result, that's why this is noticeable.
But your code looks nicer anyway. :D So, happy to change it accordingly.
|
@bors r+ (P.S. Let me know if you need review on any unix terminal or windows console stuff inside miri, terminals are a bit of an obsession of mine, so I'm pretty familiar with how this all works. I'm guessing this is probably enough for now though) |
|
📌 Commit 771df125f18c5c562744b21718dce10825530b58 has been approved by It is now in the queue for this repository. |
bors
added
S-waiting-on-bors
|
@bors r- I'll fix that nit you mentioned |
bors
added
S-waiting-on-author
Thanks! Miri does not really do anything with the terminal, it just forwards the bytes it gets from the interpreted program. Though #98070 will be nice for Miri since it means we can implement the However you might then be the right person for the other question I had in this PR: given that, from what I can see, these two command codes are literally everything libtest ever uses -- do you think it would make sense to remove all that terminfo parsing and just use these commands? (E.g. the |
|
@bors r=thomcc |
bors
added
S-waiting-on-bors
I'd have to look what all it uses. If it's just 8 colors and reset, this uses a very consistent set of escape sequences across a wide range of terminals -- you need to start worrying about terminfo mostly when you care about advanced features, especially events handling, since the sequences terminals send for events are often very different, even among modern terminals which in most other ways have converged on doing things like more or less xterm. That said, removing this would probably break compatibility with certain retro terminals though (which currently we probably support... But really, those terminals should just disabling color anyway, I suspect. If we're only interested in basic coloring, removing this is probably a good idea, as it will speed things up for everybody to not query the terminfo DB. It will also avoid doing weird things if users have That said, I'd like to look through terminfo.src to see if there's anything people are likely to still use that have other values for these (I know often reset (sgr0) will often reset the charset too, but we don't change this, so it's fine).
Windows doesn't have a terminfo db, and uses the console apis (e.g. winapi functions which probably turn into syscalls) to change the color. I believe we already do this in libtest, so windows isn't a concern. On modern windows (Win10 and later) the escape sequences are supported, although you may need to explicitly ask the console to That said, this is fiddley and for simple stuff like we want, so I wouldn't bother changing away from the console apis (besides, we still support win7 and win8 at the moment). |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#100019 (Revive suggestions for boxed trait objects instead of impl Trait) - rust-lang#100038 (Document the `no-std` target option in config.toml.example) - rust-lang#100194 (Remove even more box syntax uses from src/test) - rust-lang#100206 (test: skip terminfo parsing in Miri) - rust-lang#100230 (Use start_point instead of next_point to point to elided lifetime amp…) - rust-lang#100244 (Add armv4t-none-eabi take2) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
stop excluding TERM env var on Unix Effectively reverts #2018. Needs rust-lang/rust#100206 to not be terribly slow. Fixes #2292.
remove Windows TERM env var hack and -Zmiri-env-exclude The hack should not be needed any more since rust-lang/rust#100206. And that also mostly removes the need for `-Zmiri-env-exclude` -- if needed, users can still achieve the same by running `(unset VAR; cargo miri test)`. I am keeping `-Zmiri-env-forward` since it is still useful, e.g. to have RUST_BACKTRACE=full set in an otherwise deterministic execution. `@rust-lang/miri` any objections to removing `-Zmiri-env-exclude`?
Terminfo parsing takes a significant amount of time in Miri, making libtest startup very slow. To work around that Miri in fact unsets the
TERMvariable. However, this means we don't get colors incargo miri test.So I propose we add some logic in libtest that skips parsing terminfo files under Miri, and just uses the regular basic coloring commands (taken from the
coloredcrate).As far as I can see, these two commands are all that libtest ever needs from terminfo, so Miri doesn't even lose any functionality through this. If you want I can entirely remove the terminfo parsing code and just use these commands instead.
Cc rust-lang/miri#2292 @saethlin