847175 - make time prefixing optional

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Zack Weinberg (:zwol)]

"./mach build" puts a timestamp at the beginning of every line, which means that emacs' compilation-mode fails to detect error messages.  Please put timestamps only on messages emitted by mach itself; pass through all output from the compiler _without modification_.  (You can continue to reduce the verbosity of the messages emitted by "make" if you insist.)

Comment 1

[Gregory Szorc [:gps]]

Use vim ;)

Seriously, we should have a flag and/or environment variable for this.

Comment 2

[Zack Weinberg (:zwol)]

(In reply to Gregory Szorc [:gps] from comment #1)
> Use vim ;)

You're going to laugh, but I can't use vim because I have SunOS 4 vi wired into my fingers.

> Seriously, we should have a flag and/or environment variable for this.

Why?  "Un-break me" options are bad.  Not mangling compiler output is the Right Thing and therefore you should just do it always.  (This would, for instance, also make the "have to do something special to get colorized clang diagnostics" problem just go away.)

Comment 3

[Gregory Szorc [:gps]]

(In reply to Zack Weinberg (:zwol) from comment #2)
> Why?  "Un-break me" options are bad.  Not mangling compiler output is the
> Right Thing and therefore you should just do it always.  (This would, for
> instance, also make the "have to do something special to get colorized clang
> diagnostics" problem just go away.)

I think people like the timings by default. They give people a sense of how long builds take and have taken. When builds start becoming slow or times are off, people ask questions. This is a good thing, IMO.

In case of compiler warnings, did you know that mach records them itself? See |mach warnings-list|. It's not perfect, but no system is when you are dealing with make and its tendency to interleave output from multiple processes.

The Clang coloring problem is caused by having any program sit between Clang and the terminal that doesn't "proxy" stdout and stderr all the way to the terminal. Clang is using isatty() to determine whether to add coloring. For mach to intercept the output so it can parse warnings requires isatty() to be false. I suppose we may use some low-level file descriptor magic on some machines to trick Clang. However, this would require patches to mozprocess. Patches welcome!

We could consider hacking up the build system to force Clang to output colored warnings when mach is used. That bug hasn't yet been filed IIRC.

Comment 4

[Zack Weinberg (:zwol)]

Yeah, the timestamps are nice, but they don't need to be on every line; it would be more than adequate if they just appeared on the lines that name the files being compiled.  In other words, you should be able to accomplish all your goals by modifying only the *make* output and leaving the *compiler* output strictly alone.

Comment 5

[Zack Weinberg (:zwol)]

Also, for clarity's sake, it's the errors I care about, not the warnings.  As long as we can't convince everyone that false-positive may-be-used-uninitialized warnings need to be corrected by adding initializations to the code, digging through the warnings list is a waste of time.  (I do ensure that my own changes do not add warnings.)

Comment 6

[Gregory Szorc [:gps]]

(In reply to Zack Weinberg (:zwol) from comment #4)
> Yeah, the timestamps are nice, but they don't need to be on every line; it
> would be more than adequate if they just appeared on the lines that name the
> files being compiled.  In other words, you should be able to accomplish all
> your goals by modifying only the *make* output and leaving the *compiler*
> output strictly alone.

From the perspective of mach, the make invocation to build the tree is largely a black box. So, it's difficult to isolate *make* output from *compiler* output because from mach's perspective they are both the same. Now, we do kinda/sorta parse compiler warnings, so maybe we could not print warnings on those lines. Maybe.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: patch

(In reply to Gregory Szorc [:gps] from comment #6)
> 
> From the perspective of mach, the make invocation to build the tree is
> largely a black box. So, it's difficult to isolate *make* output from
> *compiler* output because from mach's perspective they are both the same.

I was hoping you would be able to do something more principled than this, but here, this patch adequately distinguishes the two for my purposes.  It really isn't all that complicated.

> Now, we do kinda/sorta parse compiler warnings, so maybe we could not print
> warnings on those lines. Maybe.

That would not be adequate, unless by "warnings" you also mean "errors".

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 720469 [details] [diff] [review]
patch

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

This won't fly. /python/mach is generic code not tied to Mozilla or the build system. Or, at least it's supposed to be that way.

Fixing this bug will require some somewhat major changes to mach's logging infrastructure. I have an unfinished patch in my patch queue that starts this work:

https://hg.mozilla.org/users/gszorc_mozilla.com/mq-sc/file/d5db939c798d/mach-fix-logging

Until then, if you want to hack up a patch to add a command line argument or environment variable to not print times, I'd probably r+ that.

Comment 9

[Zack Weinberg (:zwol)]

(In reply to Gregory Szorc [:gps] from comment #8)
> 
> This won't fly. /python/mach is generic code not tied to Mozilla or the
> build system. Or, at least it's supposed to be that way.

This is the point where I confess to not understanding your goals with mach at all.  At the moment mach is a workflow _impediment_ for me.  I keep trying to use it and I keep filing bugs because it seems like it could _become_ a workflow improvement, but what you care about and what I care about seem to be very different.  Also, fundamentally it seems to be barking up the wrong tree -- the build system is already too complicated and you want to add another layer to it?!  I can't conceive of anyone outside Mozilla wanting to use it, or anyone inside Mozilla wanting to use it for anything but a layer of command-line sugar over the build process, so I don't understand why you would reject an expedient change that makes it a *better* layer of command-line sugar.

(If you imagine it eventually being usable as a generic replacement for both configure and make, then I do approve of that goal, but a very high priority should be making it _precisely_ command-line and output-compatible with modern autoconf/automake build systems, so that it can be a drop-in replacement; and again, I don't see why you would reject an expedient change in that direction.  You can always pull it back out when you get your logging rewrite done.)

> Until then, if you want to hack up a patch to add a command line argument or
> environment variable to not print times, I'd probably r+ that.

If I have to remember to type "./mach --unbreak-me" to get a compilation-mode-compatible build, it's not better than "make -f client.mk" and why should I bother?

Comment 10

[Gregory Szorc [:gps]]

I sympathize with your problems.

Currently, mach is mostly syntactic sugar around existing commands. However, it does have some value adds, including warning capture during builds and native integration with the xpcshell test harness (so you can more easily see what options are available).

If mach isn't working for you today, you can just invoke commands the other/existing way. This is especially true for building, where |mach build| is invoking |make -f client.mk|. The real value adds of mach today are for things like test running (no environment variable BS). Since your problems seem to mostly be focused around building, I suggest you just type |make -f client.mk| instead of |mach build| until mach's building suits your needs. FWIW, mach works for dozens of people today as-is. While I wish we could support your use case, I think you are in the minority and - it pains me to say this - optimizing your workflow is realistically a low priority in the grand scheme of things. That being said, I want to fix mach's logging so cases like yours "just work." It will come in time.

Comment 11

[Gregory Szorc [:gps]]

Attachment: mach mode to not prefix lines with times

This patch is long overdue.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 807549 [details] [diff] [review]
mach mode to not prefix lines with times

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

::: python/mach/mach/main.py
@@ +514,5 @@
>              action='store_true', default=False,
>              help='Prefix log line with interval from last message rather '
>                  'than relative time. Note that this is NOT execution time '
>                  'if there are parallel operations.')
> +        global_group.add_argument('--log-no-times', dest='log_no_times',

This argument reads oddly. Putting the "no" after "log" makes it awkward, but I understand the desire to have --log at the beginning.

Comment 13

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5bc7c86110

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a5bc7c86110