742607 - mozharness.mozilla.talos breaks unit.sh

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Aki Sasaki (not active)]

### Running *.py [--list-actions]
18:50:29     INFO - MultiFileLogger online at 20120404 18:50:29 in /src/firefox-fetch/mozharness
18:50:29     INFO - Run as mozharness/mozilla/talos.py
18:50:29    FATAL - No path to binary specified; please specify --binary
18:50:29    FATAL - Exiting -1

This not only breaks unit.sh, but breaks the --list-actions option for any script inheriting Talos.

We can solve this by moving the logic out of __init__() into helper methods.

Assigning this to myself but not particularly attached to being the one who fixes this.

Comment 1

[Jeff Hammel]

So I didn't realize this would break tests but I was thinking about this today.  If, on one hand, you only check for (e.g.) the path to the binary (though --tests is currently in the same dilemma) in a method that requires it, then you will create a virtualenv and install all required software (and for jetperf do several other time consuming things) before the script will fail out.  On the other hand, having it this way not only breaks tests and --list-actions, but if you only want to, say, create the virtualenv then there is no reason to have the path to the binary.

There are two methodologies I can think of:
1. We do this last-minute checking which has the disadvantage I describe above of not failing immediately
2. We check for e.g. the binary and tests iff the actions specified to the script overlap sets of actions that require these parameters.  This would leave the check in __init__ but only selectively require binary, tests, etc.

I favor 2.  Aki?

Comment 2

[Aki Sasaki (not active)]

Attachment: this actually fixes it

unit.sh calls scripts/*.py with --list-actions, and mozharness/**/*.py with no arguments.  Since this had a __main__ section that instantiates and runs Talos.run() with no commandline arguments or config file, that failed.

This patch fixes this bug.

As for your question, I think both work, and the latter is preferable.  We may end up wanting to go that route.  We may end up not fatal()ing if c['binary'] isn't set at first, if we're set up to install firefox.  But we aren't forced to make that decision right now for this specific bug :)

Comment 3

[Jeff Hammel]

Comment on attachment 612446 [details] [diff] [review]
this actually fixes it

wfm for now.  we'll want to put this back in when the root issue is fixed as otherwise there is no way to run the script directly

Comment 4

[Aki Sasaki (not active)]

We don't have to put it back if we create a stub scripts/ script like http://hg.mozilla.org/build/mozharness/file/default/scripts/multil10n.py ...

Comment 5

[Aki Sasaki (not active)]

Comment on attachment 612446 [details] [diff] [review]
this actually fixes it

http://hg.mozilla.org/build/mozharness/rev/776008125d68

Comment 6

[Jeff Hammel]

Follow-ups filed: bug 742749, bug 742750