818545 - mach's mozconfig loader should display shell output when it fails

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Mike Hommey [:glandium]]

+++ This bug was initially created as a clone of Bug #818543 +++

When executing 'mach build' I get the following failure displayed:

Error running mach:

    ['build']

The error occured in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

CalledProcessError: Command '[u'/Volumes/data/code/firefox/nightly/python/mozbuild/mozbuild/mozconfig_loader', u'/Volumes/data/code/firefox/nightly/.mozconfig']' returned non-zero exit status 1
<snip>


It would be better if it would also include output from that command.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Display mozconfig errors gracefully, v1

I think this handles things gracefully. Although, I'd appreciate if you would test it and let me know if you think it is graceful enough.

There are essentially two parts to this:

1) Better handling of non-0 exit codes in the mozconfig loader
2) mach plumbing to print said exceptions

For #2, this could could be on the mozconfig @property itself. However, that seems to violate the library nature of that class. Instead, I added the exception handling code to the base class of mach commands that interact with the build system.

One part that worries me a little is that we now redirect stderr to stdout (needed to capture the shell error message). I don't think this will be an issue since all the important output is surrounded by our special tokens.

An undesirable consequence of the exception handling is that we load the mozconfig every time these mach commands are invoked, even if they may not need to access the mozconfig. I don't think this is too bad. That being said, there could be room for a new mach feature that allowed commands to share common exception handling. Perhaps classes providing mach commands could have the ability to define post-execution or exception handlers. Something to think about.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 688898 [details] [diff] [review]
Display mozconfig errors gracefully, v1

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

::: python/mozbuild/mozbuild/mozconfig.py
@@ +26,5 @@
>  via the $MOZCONFIG environment variable.
>  '''.strip()
>  
> +MOZCONFIG_BAD_EXIT_CODE = '''
> +Evaluation of your mozconfig exited with a non-0 status code. This could be

"exited with an error." ?

@@ +182,5 @@
>  
> +        try:
> +            # We need to capture stderr because that's where the shell sends
> +            # errors if execution fails.
> +            output = subprocess.check_output(args, stderr=subprocess.STDOUT,

shouldn't you capture stderr and merge it with the output you're gathering in the exception?

@@ +185,5 @@
> +            # errors if execution fails.
> +            output = subprocess.check_output(args, stderr=subprocess.STDOUT,
> +                cwd=self.topsrcdir, env=env)
> +        except subprocess.CalledProcessError as e:
> +            lines = e.output.split('\n')

splitlines()

Comment 4

[Gregory Szorc [:gps]]

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

With nits #1 and #3 addressed. #2 was a time zone issue :)