Extend format status output - Allow to change also order of description in NINJA_STATUS #1045

pali · 2015-10-28T22:23:55Z

Align all edge numbers to same length when formatting output

So when updating status output on terminal, make sure that all numbers will
be always aligned at same length. Even if progress variables change number
of characters.

Allow to change also order of description in NINJA_STATUS

This commit adds new format string %d for printing description. When %d is
not specified in NINJA_STATUS, then description is automatically printed at
the end of line.

This allows to add additional strings after description, for example
changing colors on ANSI terminals via NINJA_STATUS variable.

pali · 2016-02-12T17:43:36Z

Hi! Can you review my patches?

tfarina · 2016-02-12T18:04:21Z

@pali can you collapse all these patches into one? I believe that will make it easier to be reviewed.

pali · 2016-02-12T18:13:06Z

Hm.. there are more patches as before... and some not mine. Have you did some git push --force? Looks like there are more changes since I created this pull request in Oct 2015.

pali · 2016-02-12T18:26:30Z

I cherry-picked those two my commits on top of current master.

pali · 2016-03-04T15:37:25Z

Is there anything wrong with my patch?

pali · 2016-07-15T14:11:44Z

@tfarina so it is OK now? Ready for merge?

tfarina · 2016-07-16T12:37:34Z

@pali, sorry, it is not my call. The decision makers are @nico and/or @evmar.

pali · 2016-08-07T19:57:44Z

@nico, @evmar. any objects?

evmar · 2016-08-08T17:45:06Z

src/build.cc

@@ -168,6 +168,9 @@ string BuildStatus::FormatProgressStatus(
  string out;
  char buf[32];
  int percent;
+  char total_edges_str[32];
+  snprintf(total_edges_str, sizeof(total_edges_str), "%d", total_edges_);


Note: total_edges_ can change over the course of a build, so this isn't guaranteed. The most common thing in fact is for the total edge count to be overestimated and then reduced in the case where a command doesn't modify its outputs.

I think that this is not a problem. It is just used as an estimate for length alignment. It just provide better output for next print...

evmar · 2016-08-08T17:48:28Z

Sorry for not looking at this earlier. The single pull request with a vague description makes it easy to overlook! I think the %d change is pretty reasonable but I think the alignment one may not work as you expect.

pali · 2016-08-08T18:23:51Z

Alignment in all snprintf calls make sure that all "%d" formats will be aligned to same length. As during one FormatProgressStatus call is total_edges the highest number which can "%_d" format.

Also that number total_edges_ is not directly used. But its logarithm, so if number of edges do not start exponentially increasing or decreasing then output is pretty constant and good aligned also when doing progress.

pali · 2016-08-08T21:42:44Z

@evmar Let me know if you are going accept this patch in some way and if something is needed to rework...

pali · 2016-11-20T13:36:30Z

@evmar I updated github pull request description from commit messages (sorry for vague description on github, moreover it is github fault, that it hides git commits itself!)

@evmar can you look at above comments ↑↑↑?

pali · 2016-12-26T17:34:55Z

@evmar: PING

pali · 2017-04-13T13:16:48Z

@evmar gentle PING

jhasse · 2018-11-14T17:15:12Z

This looks quite useful, especially if we add support for colors in NINJA_STATUS. Would you be willing to rebase this on current master?

pali · 2018-11-14T17:23:06Z

I already did it more times. But nobody replied.

pali · 2018-11-15T08:29:16Z

So is still somebody interesting in this patch after rebasing? And can review it?

jhasse · 2018-11-16T08:19:38Z

Yes, I would do it.

So when updating status output on terminal, make sure that all numbers will be always aligned at same length. Even if progress variables change number of characters.

pali · 2018-11-16T09:37:02Z

@jhasse Done!

jhasse · 2018-11-16T09:39:39Z

Thanks :) Please have a look at the CI failures.

This commit adds new format string %d for printing description. When %d is not specified in NINJA_STATUS, then description is automatically printed at the end of line. This allows to add additional strings after description, for example changing colors on ANSI terminals via NINJA_STATUS variable.

pali · 2018-11-16T09:47:23Z

Done!

jhasse · 2018-11-16T09:55:25Z

Thanks. This will come in handy when #1494 is merged.

Code looks good, though I don't like the need for %*d. Maybe use %D for the description instead?

pali · 2018-11-16T11:36:24Z

I don't like the need for %*d

printf's %*d is one of the main points of this pull request. It aligns all numbers to the same length when formatting.

Maybe use %D for the description instead?

I used lowercase d for NINJA_STATUS format as all other NINJA_STATUS format characters are lowercase.

jhasse · 2018-11-16T11:41:59Z

Ah, I thought that was because it would clash with %d from the description otherwise. Sorry, I didn't read it more clearly.

Can you split the PR in "Align all edge numbers to same length when formatting output" (not sure if we want this by default, needs discussion) and "Allow to change also order of description in NINJA_STATUS"?

pali · 2018-11-16T11:49:28Z

Reason for aligning is that output format does not change so drastically when printing it in-place on terminal. Before this change, when updating 1/100 to 10/100 and then to 100/100 it changes 3 times width of text. So if you have custom NINJA_STATUS, text in that status move on that line... After this you would see 1/100, 10/100 and 100/100, always this text consist of 7 characters and with custom NINJA_STATUS text, it would not move and stay on same position.

Second commit allows to put description at any position and to have it on stable position, that alignment from first commit is really needed.

jhasse · 2018-11-16T11:55:04Z

I understand the reasoning. Users may rely on the old behavior though and it changes Ninja's default output. A way to change the behavior, maybe %S instead of %s, might still be nice to have.

I used lowercase d for NINJA_STATUS format as all other NINJA_STATUS format characters are lowercase.

Yeah, keep that. I was completely wrong there, nvm ;)

pali · 2018-12-17T11:29:24Z

@jhasse anything else is needed to process this pull request? or it is just waiting for #1494?

jhasse · 2018-12-17T12:23:08Z

No, this is independent of #1494.

I'm still not sure about the "Align all edge numbers to same length when formatting output" part. What do you think about

A way to change the behavior, maybe %S instead of %s, might still be nice to have.

?

(btw: As for all frontend stuff, I'm a fan of #1210 instead of extending Ninja itself)

pali · 2018-12-17T13:40:27Z

And what with %u, %r and %f then?

evmar · 2018-12-18T03:51:08Z

BTW, one of my design regrets in Ninja is that we have this mechanism for generating command line strings internally (via "$foo ${other}") and then we didn't reuse it for status. Not sure if it's worth thinking about here though.

jhasse · 2019-08-02T12:23:43Z

And what with %u, %r and %f then?

Good point. Hm ... maybe it would indeed be better to go with #1210 / #1600 for such changes.

pali force-pushed the master branch from 2a36067 to f0fa276 Compare October 28, 2015 22:30

evmar force-pushed the master branch from 9a4eb58 to a484344 Compare December 16, 2015 16:51

pali force-pushed the master branch from f0fa276 to 4d68ec2 Compare February 12, 2016 18:19

evmar reviewed Aug 8, 2016
View reviewed changes

pali changed the title ~~Extend format status output~~ Extend format status output - Allow to change also order of description in NINJA_STATUS Nov 20, 2016

jhasse added the frontend label Nov 14, 2018

Align all edge numbers to same length when formatting output

e622266

So when updating status output on terminal, make sure that all numbers will be always aligned at same length. Even if progress variables change number of characters.

pali force-pushed the master branch from 4d68ec2 to 8e109df Compare November 16, 2018 09:36

pali force-pushed the master branch from 8e109df to 56dd115 Compare November 16, 2018 09:40

jhasse added this to the 1.10.0 milestone Nov 16, 2018

jhasse removed this from the 1.10.0 milestone Aug 2, 2019

jhasse mentioned this pull request Jul 20, 2020

Provide a way to silence status update with empty string in NINJA_STA… #1816

Closed

jhasse mentioned this pull request May 2, 2021

Fully suppress ninja status when NINJA_STATUS is the empty string #1954

Closed

jhasse added this to the 2.0.0 milestone Apr 14, 2024

jhasse mentioned this pull request Oct 31, 2024

Update status periodically in smart terminals #2516

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend format status output - Allow to change also order of description in NINJA_STATUS #1045

Extend format status output - Allow to change also order of description in NINJA_STATUS #1045

pali commented Oct 28, 2015 •

edited

Loading

pali commented Feb 12, 2016

tfarina commented Feb 12, 2016

pali commented Feb 12, 2016

pali commented Feb 12, 2016

pali commented Mar 4, 2016

pali commented Jul 15, 2016

tfarina commented Jul 16, 2016

pali commented Aug 7, 2016

evmar Aug 8, 2016

pali Aug 8, 2016

evmar commented Aug 8, 2016

pali commented Aug 8, 2016

pali commented Aug 8, 2016

pali commented Nov 20, 2016

pali commented Dec 26, 2016

pali commented Apr 13, 2017

jhasse commented Nov 14, 2018

pali commented Nov 14, 2018

pali commented Nov 15, 2018

jhasse commented Nov 16, 2018

pali commented Nov 16, 2018

jhasse commented Nov 16, 2018

pali commented Nov 16, 2018

jhasse commented Nov 16, 2018

pali commented Nov 16, 2018

jhasse commented Nov 16, 2018

pali commented Nov 16, 2018

jhasse commented Nov 16, 2018

pali commented Dec 17, 2018

jhasse commented Dec 17, 2018

pali commented Dec 17, 2018

evmar commented Dec 18, 2018

jhasse commented Aug 2, 2019

Extend format status output - Allow to change also order of description in NINJA_STATUS #1045

Are you sure you want to change the base?

Extend format status output - Allow to change also order of description in NINJA_STATUS #1045

Conversation

pali commented Oct 28, 2015 • edited Loading

pali commented Feb 12, 2016

tfarina commented Feb 12, 2016

pali commented Feb 12, 2016

pali commented Feb 12, 2016

pali commented Mar 4, 2016

pali commented Jul 15, 2016

tfarina commented Jul 16, 2016

pali commented Aug 7, 2016

evmar Aug 8, 2016

Choose a reason for hiding this comment

pali Aug 8, 2016

Choose a reason for hiding this comment

evmar commented Aug 8, 2016

pali commented Aug 8, 2016

pali commented Aug 8, 2016

pali commented Nov 20, 2016

pali commented Dec 26, 2016

pali commented Apr 13, 2017

jhasse commented Nov 14, 2018

pali commented Nov 14, 2018

pali commented Nov 15, 2018

jhasse commented Nov 16, 2018

pali commented Nov 16, 2018

jhasse commented Nov 16, 2018

pali commented Nov 16, 2018

jhasse commented Nov 16, 2018

pali commented Nov 16, 2018

jhasse commented Nov 16, 2018

pali commented Nov 16, 2018

jhasse commented Nov 16, 2018

pali commented Dec 17, 2018

jhasse commented Dec 17, 2018

pali commented Dec 17, 2018

evmar commented Dec 18, 2018

jhasse commented Aug 2, 2019

pali commented Oct 28, 2015 •

edited

Loading