Have mach tell the build backend to colorize output, send color output from certain commands

RESOLVED FIXED in Firefox 52

Status

()

Core
Build Config
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

a year ago
A number of people have recently complained about build output not be colorized automatically. As explained by glandium in bug 1171610 comment 4, we need some plumbing between mach and the build system to make this work correctly.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
I held off adding color flags to $(CC) because that relies on detecting compiler support for color. And that will require code from bug 1171610. For the version of cargo we currently require in configure, --color should always be accepted.

Comment 4

a year ago
mozreview-review
Comment on attachment 8808369 [details]
Bug 1315785 - Set an environment variable when mach is attached to a TTY;

https://reviewboard.mozilla.org/r/91194/#review91180

::: python/mozbuild/mozbuild/mach_commands.py:324
(Diff revision 1)
>  
> +        # Tell the build backend that colorized output is acceptable.
> +        # Arguably this should be part of the backend invocation. But that
> +        # would require multiple exports of this variable.
> +        if os.isatty(sys.stdout.fileno()):
> +            os.environ[b'MACH_STDOUT_ISATTY'] = b'1'

Wouldn't this be better in MachCommandBase.__init__? (there's already a isatty test there btw).

For one, this wouldn't subtly fail to work for people doing "mach configure; mach build"
Attachment #8808369 - Flags: review?(mh+mozilla)

Comment 5

a year ago
mozreview-review
Comment on attachment 8808370 [details]
Bug 1315785 - Invoke cargo with --color=always when original stdout is a TTY;

https://reviewboard.mozilla.org/r/91196/#review91182

::: config/rules.mk:928
(Diff revision 1)
>  
> +# Enable color output if original stdout was a TTY and color settings
> +# aren't already present. This essentially restores the default behavior
> +# of cargo when running via `mach`.
> +ifdef MACH_STDOUT_ISATTY
> +ifeq (,$(filter --color,$(cargo_build_flags)))

This filter only works if cargo_build_flags contains "--color something", but doesn't if it contains "--color=something".
Attachment #8808370 - Flags: review?(mh+mozilla)
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8808369 [details]
Bug 1315785 - Set an environment variable when mach is attached to a TTY;

https://reviewboard.mozilla.org/r/91194/#review91180

> Wouldn't this be better in MachCommandBase.__init__? (there's already a isatty test there btw).
> 
> For one, this wouldn't subtly fail to work for people doing "mach configure; mach build"

Yeah, I thought about putting this in a lower-level class - possibly mach's dispatch code itself. Thinking on it some more, I think that's the right thing to do.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
Comment on attachment 8808369 [details]
Bug 1315785 - Set an environment variable when mach is attached to a TTY;

https://reviewboard.mozilla.org/r/91194/#review91440
Attachment #8808369 - Flags: review?(mh+mozilla) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8808782 [details]
Bug 1315785 - Restore environment variables after running command;

https://reviewboard.mozilla.org/r/91522/#review91446

::: python/mach/mach/main.py:368
(Diff revision 1)
>  
>              return 1
>  
>          finally:
> +            os.environ.clear()
> +            os.environ.update(orig_env)

The hidden side effect here is that this actually will call setenv for each key/value pair, even the unmodified ones. I guess we can live with that and the possibility that it subtly changes the environment.

Ideally, we shouldn't rely on os.environ for anything or touch it, but that's not going to happen easily. At least not instantly.
Attachment #8808782 - Flags: review?(mh+mozilla) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8808370 [details]
Bug 1315785 - Invoke cargo with --color=always when original stdout is a TTY;

https://reviewboard.mozilla.org/r/91196/#review91448
Attachment #8808370 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8808782 [details]
Bug 1315785 - Restore environment variables after running command;

https://reviewboard.mozilla.org/r/91522/#review91446

> The hidden side effect here is that this actually will call setenv for each key/value pair, even the unmodified ones. I guess we can live with that and the possibility that it subtly changes the environment.
> 
> Ideally, we shouldn't rely on os.environ for anything or touch it, but that's not going to happen easily. At least not instantly.

Correct: this will call setenv(). I've seen this pattern for resetting the environment in other Python programs and I've never heard of it causing any problems.

/me knocks on wood

Comment 14

a year ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce5669deefd2
Restore environment variables after running command; r=glandium
https://hg.mozilla.org/integration/autoland/rev/6e9a4c0b9cd8
Set an environment variable when mach is attached to a TTY; r=glandium
https://hg.mozilla.org/integration/autoland/rev/6cf52ce48a48
Invoke cargo with --color=always when original stdout is a TTY; r=glandium

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce5669deefd2
https://hg.mozilla.org/mozilla-central/rev/6e9a4c0b9cd8
https://hg.mozilla.org/mozilla-central/rev/6cf52ce48a48
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.