Closed Bug 1315785 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(3 files)

Bug 1315785 - Set an environment variable when mach is attached to a TTY;
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1315785 - Invoke cargo with --color=always when original stdout is a TTY;
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1315785 - Restore environment variables after running command;
58 bytes, text/x-review-board-request
glandium
: review+
Details 
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.
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 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 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)
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 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 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 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+
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
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
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
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: