374568 - Use MozBuild::Util::RunShellCommand instead of internal fork and wait methods

Status: RESOLVED WONTFIX

(Webtools Graveyard :: Tinderbox, enhancement)

Description

[Andrew Schultz]

The tinderbox build script uses its own wait_for_pid and fork_and_log methods.  wait_for_pid is particularly broken (it thinks timeouts are SIGHUP).  I have a patch that switches it to use MozBuild::Util's RunShellCommand instead (nye is running with the patch now and seems happy)

Would it be sufficient to add a note to INSTALL about the added dependency?  The tboxes would also need to all be configured.

Comment 1

[Andrew Schultz]

Attachment: patch

The resulting log file is a bit different.  You can see it in one of nye's logs:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1174846920.1174847807.17474.gz&fulltext=1

I didn't change run_shell_command, as the callers would need quite a bit of fixup to split up their arguments.

Comment 2

[Andrew Schultz]

Attachment: patch v2

This
1) renames print_test_results2 back to print_test_results
2) always passes a timeoutSecs value to print_test_results
3) passes sigNum to signal_name instead of sigName
4) omits a change to the DHTML test URL that nye has

Comment 3

[Robert Helmer [:rhelmer]]

This code was actually originally written for tinderbox, amusingly enough :) 
How does this actually improve tinderbox though? I guess it's possible now to more robustly check for errors, but we need to actually do that.

Also, MozBuild is in mozilla/tools/release/ which Tinderbox doesn't check out, how were you thinking of handling this?

I think this is a good idea, I'm just wary because I abandoned doing the same thing because fixing the error checking in tinderbox is kind of a big task, and I don't have time. It would be a very good thing to do though (along with making sure we capture stderr appropriately, etc.).

Comment 4

[Andrew Schultz]

> How does this actually improve tinderbox though?

See comment 0.  When I wrote this patch, wait_for_pid was totally broken, thinking that timeouts were crashes (since the browser died due to receiving a signal) and it consistently botched the signal that had been received.  It basically could tell when something didn't exit cleanly, but had no idea why.

At the time, I volunteered to fix wait_for_pid, but preed and you told me wait_for_pid was hope was hopeless and using RunShellCommand the way to go.

In the meantime, bug 412507 has fixed at least some of the problems with wait_for_pid.  I don't know what problems might remain that this would fix.

Beyond wait_for_pid, I think there were a few things that I changed as part of the patch to be more sane.  For instance, the regxpcom test (which doesn't actually run regxpcom anymore) currently runs through AliveTest.  AliveTest thinks the test fails because regxpcom exits on its own.  Then run_all_tests ignores the failure.  In the patch, run_all_tests invokes the command directly and can tell if the command failed or not.

> Also, MozBuild is in mozilla/tools/release/ which Tinderbox doesn't check out,
> how were you thinking of handling this?

Yes, well, that is a problem, although not a particularly big one.  Practically speaking, I'm not sure what I can do other than to add a comment somewhere that pulling mozilla/tools/release/ (and putting it somewhere appropriate) is needed.  There is really nothing more I do here.  Suggestions welcome.

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 259867 [details] [diff] [review]
patch v2

Clearing out my request queue.. I am assuming you don't want to do this still, let me know if this is not the case.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Tinderbox isn't maintained anymore. Closing.