Rebuild after ./mach test-unit

[mttr]

This is a quick and dirty workaround for issue #3928. Basically, cargo test is deleting ./target/servo, which is clearly not ideal if we want to do anything with servo after running the unit tests. This PR makes sure to rebuild after running ./mach test-unit.

I'm not familiar enough with cargo yet to know why it's doing this or what better alternatives there are to fixing this. Having to rebuild afterwards feels pretty ugly to me, but my rationalization right now is that the time it takes to build is negligible in comparison to the time it takes to run the tests. Ideally, this should be something we could take care of in Cargo.toml, but again, I'm new to this (and the documentation seems less than helpful from what I can tell so far).

I won't be available for the rest of the day, so if anyone has suggestions, or wants to wait for a better solution, I'll get back to it tomorrow probably. Otherwise, this PR at least makes ./mach test work properly, so there's that.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/3229

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[jdm]

r? @metajack

[larsbergstrom]

Could we just reorder the steps things are run in for the test rule? i.e., put unit at the end of: https://github.com/mttr/servo/blob/465546ee089db3eba44f68390984de9c9f60ccb7/python/servo/testing_commands.py#L50

I'm a little wary of forcing a rebuild of the project after running unit tests, as it will add time to our builders, which I'm pushing pretty hard to keep as low as possible. I also suspect this problem should be fixed once cargo figures out what's supposed to be going on with target sharing:
rust-lang/cargo#482

[mttr]

I realized I never updated this.

Here's the conversation I had with @larsbergstrom on IRC on the 21st (link to full log):

19:22 matt-r larsberg: Quick question on your comment on pr #4048.. Running the unit tests last would be easy, but it also leaves people without their servo binary unexpectedly. Should I just toss in a warning at the end of test-unit that says "You might want to rebuild now?"
19:22 crowbot Issue #4048: Rebuild after ./mach test-unit - #4048
19:23 * Ms2ger just wishes cargo wasn't so braindead
19:23 SimonSapin matt-r: is the cargo bug filed?
19:24 larsberg matt-r: I'm also a bit confused about why this seems to happen during 'mach test' but not if you individually run the test-* rules. e.g., if you look at http://build.servo.org/builders/linux1/builds/27, we run test-unit and then test-wpt, and the servo binary is still around after unit tests.
19:24 matt-r SimonSapin: From larsberg's comment, it might be related to rust-lang/cargo#482
19:24 crowbot Issue #482: Sharing target directories - rust-lang/cargo#482
19:27 matt-r larsberg: If you run each test individually it rebuilds on its own if the binary is missing
19:28 larsberg matt-r: but there's no rebuild of servo in the subsequent test-wpt rules, etc., at least that I see in the logs...
19:28 larsberg matt-r: wait, nevermind! you're totally right
19:28 larsberg " Compiling servo v0.0.1 (file:///home/servo/buildbot/slave/linux1/build)"
19:28 larsberg my bad: good catch
19:30 larsberg matt-r: Now I'm concerned, since this bug is clearly adding seconds to Servo's build time :-) Do we know what's actually deleting the file from disk? I can also try and summon acrichto to take a look. hopefully the crates.io release has quieted down a bit for him...
19:31 matt-r larsberg: All I know is that I threw in a pdb breakpoint and saw my binary vanish after cargo gets called by subprocess
19:32 larsberg matt-r: So we suspect cargo, not mach, as the thing deleting the binary in #3928 ? After my servo enlistments recover from testing these CEF builds, I'll try and see if I can figure out what's deleting it. Sorry for sitting on your review for a little bit and thanks for investigating it!

I'm not sure where that leaves this PR, but I knew from the beginning that this was an ugly fix anyway.

[metajack]

This is caused by a Cargo bug: rust-lang/cargo#961

[metajack]

I think reordering the targets to put test-unit last is probably the best workaround until we get a Cargo fix. We don't want to increase the build time on the builders. I'm also fine with putting a warning in when Cargo test runs.

[larsbergstrom]

r+

[bors-servo]

saw approval from larsbergstrom
at mttr@f69dfc5

[bors-servo]

merging mttr/servo/mach_unit_test_fix = f69dfc5 into auto

[bors-servo]

mttr/servo/mach_unit_test_fix = f69dfc5 merged ok, testing candidate = f06e0a8

[bors-servo]

all tests pass:
success: http://build.servo.org/builders/linux1/builds/387
success: http://build.servo.org/builders/linux2/builds/386
success: http://build.servo.org/builders/android/builds/422
success: http://build.servo.org/builders/mac1/builds/603
success: http://build.servo.org/builders/mac2/builds/604

[bors-servo]

fast-forwarding master to auto = f06e0a8