878861 - mach's need-to-clobber help output is too long by a few characters and wraps in 80-char terminals, since timestamps are prepended

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Daniel Holbert [:dholbert]]

Every time I need to clobber (say, ~daily), I get output that looks like this from mach:

> 0:00.25 /usr/bin/make -f client.mk -s
> 0:00.30 Adding client.mk options from /scratch/work/builds/mozilla-inbound/mozi
>lla/.mozconfig:
> 0:00.31     MOZ_OBJDIR=$(TOPSRCDIR)/../obj
> 0:00.31     MOZ_MAKE_FLAGS=-s -j8
> 0:00.47 ***
> 0:00.47 * The CLOBBER file has been updated, indicating that an incremental bui
>ld since
> 0:00.47 * your last build will probably not work. A full/clobber build is requi
>red.
> 0:00.47 *
> 0:00.47 * The reason for the clobber is:
> 0:00.47 *
> 0:00.47 *  Bug 778948 moved a file, which make is still looking for
> 0:00.47 *  Alternative to clobber is to run ./config.status from the objdir and
> to
> 0:00.47 *  touch the CLOBBER file in the objdir.
> 0:00.47 *
> 0:00.47 * Clobbering can be performed automatically. However, we didn't automat
>ically
> 0:00.47 * clobber this time because:
...newlines and all.

This is because my terminal is the (Ubuntu default) 80 characters wide, and mach's output is just barely over 80 characters.

It looks like we might be trying to stay under 80 characters in the source code, but we're not realizing that there's going to be an extra indent of 9  for the datestamp + space on either side, which ends up making a lot of lines to wrap by just a few characters (as shown above).

Could we rewrap these lines to keep them under ~70 characters, to allow for 9 characters of the space-padded datestamp indentation?

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot

Comment 2

[Arnaud Sourioux [:Six]]

Attachment: Preserve 80 cols output

Is this was you are expecting?
I prefer do it dynamically in code before printing rather than just modify the strings, in case we want to modify the printed messages.

Comment 3

[Daniel Holbert [:dholbert]]

Isn't this patch sort of equivalent to what my terminal is already doing for me? (inserting extra newlines so that the text remains onscreen)  I suppose this patch won't insert a newline  in the middle of a word, which is an improvement, but you'll still end up creating a long Line / short line / long line / short line pattern.

In any case -- given that this string is hardcoded (with newlines), it seems like it'd be better/simpler to just hardcode it "correctly", rather than to try to add another layer of additional intelligent-rewrapping on top it.

Comment 4

[Arnaud Sourioux [:Six]]

Attachment: Preseve 80 cols output with mach's timestamp

This patch is what was expected

Comment 5

[Arnaud Sourioux [:Six]]

Could you give it a try daniel? (if you got free time :) )

Comment 6

[Daniel Holbert [:dholbert]]

Yup -- it looks much better, with your patch applied. (I tested by editing CLOBBER and running ./mach build.)

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 758717 [details] [diff] [review]
Preseve 80 cols output with mach's timestamp

Review of attachment 758717 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

For extra credit, you could probably replace all of this with http://docs.python.org/2/library/textwrap.html. But don't let that hold up progress. File a new, follow-up bug only if you intend to hook up textwrap.

Comment 8

[Arnaud Sourioux [:Six]]

Thanks, i'm gonna fill a new bug using textwrap.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2ac3d6c628a

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d2ac3d6c628a