Skip to content

custom compile progress via env var template #6070

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed

Conversation

matthiaskrgr
Copy link
Member

@matthiaskrgr matthiaskrgr commented Sep 21, 2018

Allows to customize the compile progress via env var.
This was inspired by NINJA_STATUS environmental variable:
https://ninja-build.org/manual.html#_environment_variables

I had to add some workarounds to make sure the CARGO_STATUS var does not mess with the git fetch/crate download progress indicators.
Right now this should only change the output of "cargo build/check"

Docs from the readme:

CARGO_STATUS — If this is set, cargo will read it as compile progress template.
The default format is [%b] %f/%t: %n which will look like this:
Building [=====> ] 115/145: libssh2-sys(build), thread_l...
The following parameters are supported:
* %b progress bar =======>
* %s number of started jobs
* %f number of finished jobs
* %t number of total jobs of the build
* %u number or remaining jobs to start
* %r number of active (currently running) jobs
* %p progress percentage with decimals
* %P progress percentage without decimals
* %% plain % character (can be used for %P%% => 14%)
* %n (truncated) list of names of running jobs
* %e elapsed time in seconds
* %E elapsed time in seconds (human readable)
* %o overall time per job
* %O overall time per job (human readable)
* %c jobs per second

@rust-highfive
Copy link

r? @alexcrichton

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

@@ -34,6 +34,20 @@ system:
will otherwise be used.
* `CARGO_CACHE_RUSTC_INFO` — If this is set to 0 then Cargo will not try to cache
compiler version information.
* `CARGO_STATUS` — If this is set, cargo will read it as compile progress template.
The default format is `[%b] %f/%t: %n` which will looke like this:
Copy link
Member Author

Choose a reason for hiding this comment

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

typo, will fix later

}
let length_of_formatters = template.len() - template_skelleton.len();

// Render the percentage at the far right and then figure how long the progress bar iscustom.compile_progress
Copy link
Member Author

Choose a reason for hiding this comment

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

will also fix later

@matthiaskrgr matthiaskrgr changed the title [WIP] custom compile progress via env var template custom compile progress via env var template Sep 21, 2018
@matthiaskrgr matthiaskrgr force-pushed the custom_compile_progress_pr branch from 54b953e to 938f92d Compare September 22, 2018 15:25
@alexcrichton
Copy link
Member

Thanks for the PR! In general though Cargo's output isn't customizable to this degree and we try to avoid mini-DSLs wherever possible in Cargo. I would personally expect a feature like this to likely require an RFC rather than being accepted through a PR. Others on @rust-lang/cargo may feel different though.

@joshtriplett
Copy link
Member

@alexcrichton I agree. If there's information not being provided via the build log, we should provide it. And if there's a different format we need when stdout isn't a TTY, we should provide that too. But let's not make it customizable.

@matthiaskrgr
Copy link
Member Author

matthiaskrgr commented Sep 26, 2018

mini-DSLs

what does DSL stand for?

When using ninja, I find the number of currently running jobs and the current build time very useful to know, but I didn't want to change the default progress of cargo which is why I made it customisable.

@dwijnand
Copy link
Member

what does DSL stand for?

Domain-specific language. (https://en.wikipedia.org/wiki/Domain-specific_language)

@alexcrichton
Copy link
Member

I think it's fine to tweak and update Cargo's default output, and it's likely beneficial to everyone to have a better default output rather than allowing one-off customization.

@joshtriplett
Copy link
Member

We discussed this in the @rust-lang/cargo meeting, and just as discussed here in this issue, this degree of output customization is not something we're looking to support. We'd prefer to make the output as usable as possible for various use cases, and if people have extremely specialized needs, they may need the full flexibility of build plans and custom build processes.

Given that:

@rfcbot fcp close

@joshtriplett
Copy link
Member

(oops, accidentally hit "close and comment")

@joshtriplett joshtriplett reopened this Sep 26, 2018
@joshtriplett joshtriplett added the T-cargo Team: Cargo label Sep 26, 2018
@joshtriplett
Copy link
Member

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Sep 26, 2018

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

@matthiaskrgr
Copy link
Member Author

matthiaskrgr commented Sep 26, 2018

Well ok.

In the latest commit I added a bunch of time-related formatters.

So far the NINJA_STATUS template that has served me well is CARGO_STATUS="[%P%%/%f/%r/%t/%e]".
This would look like this: Building [ 12%/50/4/402/22] ([percentage/done/active/total/seconds].
The current cargo default is [%b] %f/%t: %n ([progress bar] done/total: job_names)

If we are going to display additional information (without customizale formatting), I would vote for something like
"%P%% [%b] %f/%r/%t (%E): %n" as new default:
Building 29% [==========> ] 17/4/58 (14.19s): termcolor, ansi_term, natord, glob
(github is truncating away some whitespaces unfortunately..)
It shows the progress percentage, bar, and afterwards the number of finished jobs, number of currently running/active jobs (this is easier to see than counting the job names (which might get truncated anyway)) and how long the build has been running so far in human readable format, completed by the names of current jobs.

I would like to know what others would want to see or if they are happy with the current compile progress layout. :)

@alexcrichton
Copy link
Member

That looks reasonable to me, but the 17/4/58 I feel is a bit confusing in that it's sort of just three numbers where it's not clear what to do with it. The 29% I feel may also be redundant with the number of crates to build and total crates?

@alexcrichton
Copy link
Member

@matthiaskrgr have you had a chance to game this out and see what it would look like to change Cargo's default progress bar?

@matthiaskrgr
Copy link
Member Author

I'll hopefully get to this early in the upcomming week.

@matthiaskrgr
Copy link
Member Author

That looks reasonable to me, but the 17/4/58 I feel is a bit confusing in that it's sort of just three numbers where it's not clear what to do with it. The 29% I feel may also be redundant with the number of crates to build and total crates?

So you would opt for just the current progress bar plus the build time?
( CARGO_STATUS="[%b] %f/%t (%E): %n")

have you had a chance to game this out and see what it would look like to change Cargo's default progress bar?

Hm, I'm not sure if I understand "what it would look like"...
On the branch we can try everything out and once we decided for a new(?) format, I can remove all the unnecessary (:() code that made it customizable and hardcode/update to the new format.

@matthiaskrgr
Copy link
Member Author

bump?

@alexcrichton
Copy link
Member

Er sorry for the delay, but I can't really visualize what this would look like given the syntax above, can a gif be provided?

I don't think we're going to land this as-is. We can land, however, a change to the defaults

@matthiaskrgr
Copy link
Member Author

Ok:
default format [%b] %f/%t: %n
https://asciinema.org/a/eU1MJqRZTWVbPG3ZhAN4p7qFX

what I suggested earlier but is probably overkill %P%% [%b] %f/%r/%t (%E): %n
https://asciinema.org/a/SSbDV7HVfVDYk5GZ7S1lh7PPu

default format + build time "[%b] %f/%t (%E): %n"
https://asciinema.org/a/6HkyFGkJigy8ViAML3bIJVyAo

…ss formatting

You can pass CARGO_STATUS env var to cargo to change the formatting.

%b is the progress bar
%s is the number of done jobs
%t is the number of total jobs
%p is progress percentage
%n is a list of names of running jobs

The default formatting is "[%b] %s/%t%n"

You can pass alternatives like this:
	CARGO_STATUS="%s of %t, running: %n, [%b]" cargo build
…ng with download progress.

When downloading crates, use preset, non-costumizable compile-progress formats.
change %s (number of started jobs) to %f (number of finished jobs) since that's what it actually is
add new %s which represents the actual number of started jobs
add %u representing the number of remaining jobs to start
In a pattern like %%s, we would previously evaluate %% first and then have a bare s char
	%%s => %s
Evaluate %% last so that
	%%s => %2
	%e elapsed time in seconds
	%E elapsed time (human readable)
	%o overall time per job
	%O overall time per job (human readable)
	%c jobs done per second
@matthiaskrgr matthiaskrgr force-pushed the custom_compile_progress_pr branch from 2b7a7d3 to b973718 Compare November 1, 2018 14:23
@alexcrichton
Copy link
Member

Ok thanks for the gifs! I agree that the second one is probably too much information, but for the last one I'm not personally a huge fan of the timer only being updated when the output is otherwise updated, it looks sort of bad during long compilations where no activity is happening and the timer just sits there

@bors
Copy link
Contributor

bors commented Nov 6, 2018

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

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 9, 2018

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

psst @joshtriplett, I wasn't able to add the final-comment-period label, please do so.

@dwijnand dwijnand added the final-comment-period FCP — a period for last comments before action is taken label Nov 9, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 19, 2018

The final comment period, with a disposition to close, as per the review above, is now complete.

psst @joshtriplett, I wasn't able to add the finished-final-comment-period label, please do so.

@joshtriplett joshtriplett removed the final-comment-period FCP — a period for last comments before action is taken label Nov 19, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants