Add a term option to configure the progress bar

[mchernyavsky]

Closes #7587.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[bors]

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

[ehuss]

Thanks for the PR!

Can you say a little more about your use case? It seems like it would be preferable, if you want to display a GUI progress bar, to use JSON instead of trying to parse the text. If Cargo provided whatever information is needed in JSON to compute a progress bar, would that work better for you?

A while back, I worked on a similar feature for controlling the progress bar, though I put it in config (here is the unfinished work). I ultimately decided it was a little too weird, but it would be possible to go that route. Generally it would be a config like:

[term]
progress = { when = 'always', width = 100 }

With it being a config value, it gets environment variable support for free. It also provides a way to pass in the width (hard-coding to 80 seems a little uneasy). We've also had a few requests for the ability to turn it off, but keep colored output, which this would also support.

[mchernyavsky]

@ehuss Hi!

Can you say a little more about your use case?

I use the progress bar not only to display a GUI progress bar, but also to understand when to finish build events in the event tree. For example:

Compiling libc v0.2.69                                         // send StartEvent(id="libc v0.2.69")
Compiling bytesize v1.0.0                                      // send StartEvent(id="bytesize v1.0.0")
 Building [...] 0/6: bytesize, libc(build.rs)                  // 
 Building [...] 1/6: libc(build.rs)                            // send FinishEvent(id="bytesize v1.0.0")
 Building [...] 2/6: libc(build)                               // 
 Building [...] 3/6: libc                                      // 
Compiling atty v0.2.14                                         // send StartEvent(id="atty v0.2.14")
 Building [...] 3/6: atty, libc                                // 
 Building [...] 4/6: atty                                      // send FinishEvent(id="libc v0.2.69")
Compiling example v0.1.0 (...)                                 // send StartEvent(id="example v0.1.0")
 Building [...] 5/6: example(bin)                              // send FinishEvent(id="atty v0.2.14")
 Finished dev [unoptimized + debuginfo] target(s) in 11.75s    // finish all events ("atty v0.2.14")

This is how it looks for projects with many dependencies:

NB: The progress bar is necessary to build the event tree, so I need it to always be shown. For now, I use a terminal emulator and remove the --quiet option from the build command line.

Also, I don't want to limit the maximum line length because I need parse all crate names.

If Cargo provided whatever information is needed in JSON to compute a progress bar, would that work better for you?

Yes, it would be perfect. The JSON message can include more information, for example, crate versions (atty, libc -> atty v0.2.14, libc v0.2.69).

[ehuss]

If JSON messages would work better, would you be willing to work through and suggest what needs to be added? The compiler-artifact message indicates when a particular unit is finished. Perhaps we could add a message when it starts? Maybe remove the Compiling status text? And a message at the beginning that indicates how many jobs there are? This wouldn't be exactly at the same granularity as "Package", but I would argue it is more useful to be more granular.

Are there any other messages that would be required?

[mchernyavsky]

@ehuss, sorry for a long response.

It looks like the compiler-artifact message is not shown if the build failed.
Example project:

• main.rs

fn main() { 0 }

Result:

$ cargo build --message-format=json
   Compiling sandbox v0.1.0 (/Users/mikhail.chernyavsky/Workspace/sandbox-rs)
{"reason":"compiler-message","package_id":"sandbox 0.1.0 (path+file:///Users/mikhail.chernyavsky/Workspace/sandbox-rs)","target":{"kind":["bin"],"crate_types":["bin"],"name":"sandbox","src_path":"/Users/mikhail.chernyavsky/Workspace/sandbox-rs/src/main.rs","edition":"2018","doctest":false},"message":{"rendered":"error[E0308]: mismatched types\n --> src/main.rs:1:13\n  |\n1 | fn main() { 0 }\n  |           - ^ expected `()`, found integer\n  |           |\n  |           expected `()` because of default return type\n\n","children":[],"code":{"code":"E0308","explanation":"Expected type did not match the received type.\n\nErroneous code example:\n\n```compile_fail,E0308\nlet x: i32 = \"I am not a number!\";\n//     ~~~   ~~~~~~~~~~~~~~~~~~~~\n//      |             |\n//      |    initializing expression;\n//      |    compiler infers type `&str`\n//      |\n//    type `i32` assigned to variable `x`\n```\n\nThis error occurs when the compiler is unable to infer the concrete type of a\nvariable. It can occur in several cases, the most common being a mismatch\nbetween two types: the type the author explicitly assigned, and the type the\ncompiler inferred.\n"},"level":"error","message":"mismatched types","spans":[{"byte_end":13,"byte_start":12,"column_end":14,"column_start":13,"expansion":null,"file_name":"src/main.rs","is_primary":true,"label":"expected `()`, found integer","line_end":1,"line_start":1,"suggested_replacement":null,"suggestion_applicability":null,"text":[{"highlight_end":14,"highlight_start":13,"text":"fn main() { 0 }"}]},{"byte_end":10,"byte_start":10,"column_end":11,"column_start":11,"expansion":null,"file_name":"src/main.rs","is_primary":false,"label":"expected `()` because of default return type","line_end":1,"line_start":1,"suggested_replacement":null,"suggestion_applicability":null,"text":[{"highlight_end":11,"highlight_start":11,"text":"fn main() { 0 }"}]}]}}
{"reason":"compiler-message","package_id":"sandbox 0.1.0 (path+file:///Users/mikhail.chernyavsky/Workspace/sandbox-rs)","target":{"kind":["bin"],"crate_types":["bin"],"name":"sandbox","src_path":"/Users/mikhail.chernyavsky/Workspace/sandbox-rs/src/main.rs","edition":"2018","doctest":false},"message":{"rendered":"error: aborting due to previous error\n\n","children":[],"code":null,"level":"error","message":"aborting due to previous error","spans":[]}}
{"reason":"compiler-message","package_id":"sandbox 0.1.0 (path+file:///Users/mikhail.chernyavsky/Workspace/sandbox-rs)","target":{"kind":["bin"],"crate_types":["bin"],"name":"sandbox","src_path":"/Users/mikhail.chernyavsky/Workspace/sandbox-rs/src/main.rs","edition":"2018","doctest":false},"message":{"rendered":"For more information about this error, try `rustc --explain E0308`.\n","children":[],"code":null,"level":"failure-note","message":"For more information about this error, try `rustc --explain E0308`.","spans":[]}}
{"reason":"build-finished","success":false}
error: could not compile `sandbox`.

I use the following cargo messages:

• Compiling ... and Fresh ... to create (and immediately complete in case of Fresh) a new node.
• Building ... to update the GUI progress bar and finish non-active nodes.
• Finished ... to complete all active nodes.
• Could not compile ... to unsuccessfully complete the current node and skip the other active nodes.

So at least I need start message and finish message with build status (fresh, success, failure, skipped) for every unit.

And a message at the beginning that indicates how many jobs there are?

Yes, this and start/finish messages should be enough to render the GUI progress bar.

This wouldn't be exactly at the same granularity as "Package", but I would argue it is more useful to be more granular.

It would be nice if start/finish messages contain information about which package this unit belongs to. Thus, we can create a separate subtree for units with the same package.

Also, cargo doesn't apply the --message-format=json option to its own errors. For example:

$ cargo build --message-format=json --unknown
error: Found argument '--unknown' which wasn't expected, or isn't valid in this context
...

So I have to parse lines with error: and warning: prefixes. It would be great if these messages are also in JSON format.

[ehuss]

Thanks for the details!

So it sounds like there are at least 2 new messages to add?

1. Message when starting a unit.
2. Message when a unit is finished (success or fail, fresh or dirty). The "compiler-artifact" message probably wouldn't work for the failure case.

I'm uncertain about the best way to express "progress". There are different stages (updating indexes, resolving, downloading, compiling, etc.). Using "unit count" may not be the best option due to that, and also that could make it difficult in the future if Cargo is more lazy or dynamic when deciding what to build (although maybe the unit count could be documented as an "estimate"?). An option is to have an initial "build-started" JSON message with the estimated unit count?

An alternative is to have Cargo compute some estimated progress amount as a number from 0 to 1. It could then include that in the JSON messages (or maybe as a separate message).

I'm also wondering about whether to show progress for the downloading phase separately, or should it just be part of a single "progress" progression? Downloading can be quite slow.

I wonder, do any other build tools have a similar concept?

So I have to parse lines with error: and warning: prefixes. It would be great if these messages are also in JSON format.

I filed #8283 for this. It is something I've wanted for a long time.

[bors]

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

[mchernyavsky]

@ehuss Should I close this PR and wait for #8283? Or we can merge this as a temporary solution?

[ehuss]

I think it would be useful to go ahead and add a config option. However, as mentioned above at #8165 (comment), I would kinda prefer the schema described there. I don't remember the exact status of the linked patch, but just a quick look it seems complete. If you want to try to rebase it and see if it works, that would be helpful. And if there are any questions, just let me know.

[mchernyavsky]

@ehuss This is not suitable for my case, because I need to run cargo with progress bar regardless of the settings specified by the user, i.e. I need a way to override user's settings from the IDE side. It can be a command-line option or an environment variable.

I have found recently added terminal-width option. I can use it in this PR. Would it be better if I just pass terminal-width value to the progress bar instead of introducing a new option?

[ehuss]

Maybe I'm a bit confused, but all config options can be set with environment variables. In this case, it would be CARGO_TERM_PROGRESS_WIDTH=100, which should override any user settings.

[mchernyavsky]

Maybe I'm a bit confused, but all config options can be set with environment variables. In this case, it would be CARGO_TERM_PROGRESS_WIDTH=100, which should override any user settings.

Oh, I didn't know it. Now I understand what you meant when you said, "it gets environment variable support for free" :)

I have rebased your patch, but it looks like this line from progress_or_string

https://github.com/rust-lang/cargo/blob/8f6d8d8e41d8ec16ba767f5a847f26340c6fc8a9/src/cargo/util/config/mod.rs#L1841

failed to parse term.progress.when field and always returns auto.

p.s. It correctly parses term.progress.width.

[bors]

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

[ehuss]

If you mean it doesn't work with environment variables, I think the following should fix it:

diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs
index c50ee8771..6cc34a3cc 100644
--- a/src/cargo/util/config/mod.rs
+++ b/src/cargo/util/config/mod.rs
@@ -1852,7 +1852,7 @@ where
         }
     }
 
-    deserializer.deserialize_any(ProgressVisitor)
+    deserializer.deserialize_option(ProgressVisitor)
 }
 
 /// A type to deserialize a list of strings from a toml file.

[mchernyavsky]

@ehuss I have added some tests, but, for some reason, always progress doesn't require width key. There are two checks for it in progress_or_string, but looks like this code isn't being used.

https://github.com/rust-lang/cargo/blob/44027cc31114c781d9d3b7dfc9d46a47fa27ad81/src/cargo/util/config/mod.rs#L1849

https://github.com/rust-lang/cargo/blob/44027cc31114c781d9d3b7dfc9d46a47fa27ad81/src/cargo/util/config/mod.rs#L1878-L1880

[ehuss]

Ah, I think the change to deserialize_option makes it go through a different code path. I believe the visit_map is no longer necessary, and a change like this might help:

diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs
index b75a86e31..eda4daade 100644
--- a/src/cargo/util/config/mod.rs
+++ b/src/cargo/util/config/mod.rs
@@ -1862,14 +1862,7 @@ where
         where
             D: serde::de::Deserializer<'de>,
         {
-            ProgressConfig::deserialize(deserializer).map(Some)
-        }
-
-        fn visit_map<M>(self, map: M) -> Result<Self::Value, M::Error>
-        where
-            M: serde::de::MapAccess<'de>,
-        {
-            let pc = Deserialize::deserialize(serde::de::value::MapAccessDeserializer::new(map))?;
+            let pc = ProgressConfig::deserialize(deserializer)?;
             if let ProgressConfig {
                 when: ProgressWhen::Always,
                 width: None,

[mchernyavsky]

@ehuss Thanks! Now the PR is ready.

[ehuss]

Thanks, can you also update the documentation? The places that need updating:

• Add to the TOML example at https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/config.md#configuration-format
• Add a section to https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/config.md#term
• Add CARGO_TERM_PROGRESS_WHEN and CARGO_TERM_PROGRESS_WIDTH to https://github.com/rust-lang/cargo/blob/master/src/doc/src/reference/environment-variables.md#configuration-environment-variables

[mchernyavsky]

@ehuss Done.

[ehuss]

@rfcbot fcp merge

@rust-lang/cargo This adds a new, stable configuration option. I feel like it is minor enough that it does not need to go through the unstable process, though feel free to disagree. The option is term.progress which allows controlling the progress bar. It takes the following values:

• "never": Complete disables the progress bar. This has been requested a few times for different contexts. Currently, there are relatively awkward ways to do this, such as setting the CI environment variables.
• "auto": This is the current behavior.
• {when: "always", width: 100}: Always enables the progress bar with the given screen width.

[rfcbot]

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @alexcrichton
• @ehuss
• @joshtriplett

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[ehuss]

Thanks!
@bors r+

[bors]

📌 Commit d649c66 has been approved by ehuss

[bors]

⌛ Testing commit d649c66 with merge c369b8c...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing c369b8c to master...