1361003 - cargo errors during rust vendoring should be printed

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:glob ✱]

the error message produced should `mach vendor rust` fail is not useful - it contains just that executing cargo failed and a stacktrace to the run_process fn.

$ ./mach vendor rust
 0:02.88 rm -rf /home/servo-sync/firefox-overlay/third_party/rust
 0:02.88 /home/servo-sync/.cargo/bin/cargo update --manifest-path /home/servo-sync/firefox-overlay/toolkit/library/rust/Cargo.toml -p gkrust
Error running mach:

    ['vendor', 'rust']

The error occurred 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:

Exception: Process executed with non-0 exit code 101: [u'/home/servo-sync/.cargo/bin/cargo', u'update', u'--manifest-path', u'/home/servo-sync/firefox-overlay/toolkit/library/rust/Cargo.toml', u'-p', u'gkrust']

  File "/home/servo-sync/firefox-overlay/python/mozbuild/mozbuild/mach_commands.py", line 1819, in vendor_rust
    vendor_command.vendor(**kwargs)
  File "/home/servo-sync/firefox-overlay/python/mozbuild/mozbuild/vendor_rust.py", line 275, in vendor
    self._run_command_in_srcdir(args=[cargo, 'update', '--manifest-path', mozpath.join(path, 'Cargo.toml'), '-p', lib])
  File "/home/servo-sync/firefox-overlay/python/mozbuild/mozbuild/base.py", line 600, in _run_command_in_srcdir
    return self.run_process(cwd=self.topsrcdir, **args)
  File "/home/servo-sync/firefox-overlay/python/mach/mach/mixin/process.py", line 147, in run_process
    raise Exception('Process executed with non-0 exit code %d: %s' % (status, args))


running cargo directly shows the actual error message:

$ cargo update --manifest-path /home/servo-sync/firefox-overlay/toolkit/library/rust/Cargo.toml -p gkrust
    Updating registry `https://github.com/rust-lang/crates.io-index`
error: failed to select a version for `smallvec` (required by `style`):
all possible versions conflict with previously selected versions of `smallvec`
  version 0.3.2 in use by smallvec v0.3.2
  possible versions to select: 0.3.3

this error should have been included in the output of `mach vendor rust`.

Comment 1

[(not currently active) Ted Mielczarek]

We're currently calling `run_process` to invoke cargo (by way of `_run_command_in_srcdir`), which uses mozprocess under the hood:
https://dxr.mozilla.org/mozilla-central/rev/5278e2a35fc8f2be390243db1e62858bf0982055/python/mach/mach/mixin/process.py#133

I'd have to look at the mozprocess docs/code to see if there's anything nice it gives us to handle this situation. If not, we could just use `subprocess.check_call` directly, which will raise a `CalledProcessError` if the command fails, which will give us stderr in the exception object:
https://docs.python.org/2.7/library/subprocess.html#subprocess.check_call

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> We're currently calling `run_process` to invoke cargo (by way of
> `_run_command_in_srcdir`), which uses mozprocess under the hood:
> https://dxr.mozilla.org/mozilla-central/rev/
> 5278e2a35fc8f2be390243db1e62858bf0982055/python/mach/mach/mixin/process.
> py#133
> 
> I'd have to look at the mozprocess docs/code to see if there's anything nice
> it gives us to handle this situation. If not, we could just use
> `subprocess.check_call` directly, which will raise a `CalledProcessError` if
> the command fails, which will give us stderr in the exception object:
> https://docs.python.org/2.7/library/subprocess.html#subprocess.check_call

It doesn't look like the mozprocess code provides anything nice for dealing with stderr, just code to process stdout as it comes in.  subprocess.check_call it is!

Comment 4

[:glob ✱]

Attachment: 1361003_1.patch

Comment 5

[:glob ✱]

Attachment: 1361003_2.patch

minor update to silence .cargo/config instructions.

Comment 6

[:glob ✱]

Attachment: 1361003_3.patch

*sigh* updated so it applies to tip, instead of a revision from march.
sorry about the bugspam.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8879449 [details] [diff] [review]
1361003_3.patch

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

I know we have some niceties in mozprocess that we'll probably lose out on here, like timeout handling, but in automation that generally gets handled in other layers anyway.

::: python/mozbuild/mozbuild/vendor_rust.py
@@ +271,5 @@
>              ('geckodriver', 'testing/geckodriver'),
>          )
>          for (lib, crate_root) in crates_and_roots:
>              path = mozpath.join(self.topsrcdir, crate_root)
> +            # We use check_call instead of mozprocess to ensure errors are displayed. See bug 1361003

I don't think the "see bug X" comment is really necessary, that's what blame is for.

Comment 8

[(not currently active) Ted Mielczarek]

I think I originally meant `check_output`, but I guess it's not really important to hide the stdout of these commands, and in fact it might be better to have it all printed.

Comment 9

[:glob ✱]

Attachment: 1361003_4.patch

previous patch conflicts with bug 1369156.
carrying forward r+

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6bfc0bcf3c8
Use check_call when calling "mach vendor rust". r=ted

Comment 11

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

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