Make blink_trybot recipe work on windows

Make blink_trybot recipe work on windows

R=agable@chromium.org
BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=208578

[agable, 2013-06-25 15:55:27]

looks mostly good to me

https://codereview.chromium.org/17635005/diff/1/scripts/common/annotator.py
File scripts/common/annotator.py (right):

https://codereview.chromium.org/17635005/diff/1/scripts/common/annotator.py#n...
scripts/common/annotator.py:444: line = '###'+line[3:]
This only replaces the leading @@@ with ###, whereas the old filter replaced all
@@@ with ###. The functionality is the same but the old one looked better.

https://codereview.chromium.org/17635005/diff/1/scripts/common/annotator.py#n...
scripts/common/annotator.py:525: v = str(v)
this isn't very unicode() friendly :(

https://codereview.chromium.org/17635005/diff/1/scripts/common/annotator.py#n...
scripts/common/annotator.py:538: ret[str(k)] = v
elif isinstance(v, dict):?

https://codereview.chromium.org/17635005/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://codereview.chromium.org/17635005/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/chromium/config.py:66: **_kwargs):
why?

https://codereview.chromium.org/17635005/diff/1/scripts/slave/recipe_modules/...
File scripts/slave/recipe_modules/python/api.py (right):

https://codereview.chromium.org/17635005/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/python/api.py:10: preamble = ['python']
call it 'cmd' or something -- it eventually holds ['python', '-u', 'foo.py']
which is pretty much the main body, not a preamble.

https://codereview.chromium.org/17635005/diff/1/scripts/slave/recipe_modules/...
scripts/slave/recipe_modules/python/api.py:16: def inline(self, name, program,
**kwargs):
docstring

https://codereview.chromium.org/17635005/diff/1/scripts/slave/recipes/blink_t...
File scripts/slave/recipes/blink_trybot.py (right):

https://codereview.chromium.org/17635005/diff/1/scripts/slave/recipes/blink_t...
scripts/slave/recipes/blink_trybot.py:5: DEPS = ['python', 'chromium',
'step_history', 'path', 'gclient', 'rietveld',
Can we please sort these DEPS everywhere?

[iannucci, 2013-06-25 21:22:09]

PTAL

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/common/annotat...
File scripts/common/annotator.py (right):

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/common/annotat...
scripts/common/annotator.py:444: line = '###'+line[3:]
On 2013/06/25 15:55:28, Aaron Gable wrote:
> This only replaces the leading @@@ with ###, whereas the old filter replaced
all
> @@@ with ###. The functionality is the same but the old one looked better.

I'll do you one better. I changed it to just print a '!' before the line. Now we
don't have to copy any strings at all. Yay.

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/common/annotat...
scripts/common/annotator.py:525: v = str(v)
On 2013/06/25 15:55:28, Aaron Gable wrote:
> this isn't very unicode() friendly :(

No, but we shouldn't have unicode in our annotator dictionaries. For some
reason, windows is hellbent on decoding ascii json as unicode(). 

In particular, subprocess hates unicode arguments for Popen (at least on 2.6),
and python hates unicode keys for dictionaries in kwargs (like when we **step).

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/common/annotat...
scripts/common/annotator.py:538: ret[str(k)] = v
On 2013/06/25 15:55:28, Aaron Gable wrote:
> elif isinstance(v, dict):?

Oops. Fixed.

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/config.py (right):

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/config.py:66: **_kwargs):
On 2013/06/25 15:55:28, Aaron Gable wrote:
> why?

Because when you set_config(..., SOME_OPTION=Data), SOME_OPTION is passed to all
the schema functions.

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/python/api.py (right):

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/python/api.py:10: preamble = ['python']
On 2013/06/25 15:55:28, Aaron Gable wrote:
> call it 'cmd' or something -- it eventually holds ['python', '-u', 'foo.py']
> which is pretty much the main body, not a preamble.

Done.

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/slave/recipe_m...
scripts/slave/recipe_modules/python/api.py:16: def inline(self, name, program,
**kwargs):
On 2013/06/25 15:55:28, Aaron Gable wrote:
> docstring

Good catch. Done.

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/slave/recipes/...
File scripts/slave/recipes/blink_trybot.py (right):

https://chromiumcodereview.appspot.com/17635005/diff/1/scripts/slave/recipes/...
scripts/slave/recipes/blink_trybot.py:5: DEPS = ['python', 'chromium',
'step_history', 'path', 'gclient', 'rietveld',
On 2013/06/25 15:55:28, Aaron Gable wrote:
> Can we please sort these DEPS everywhere?

Done.

[agable, 2013-06-25 21:26:35]

lgtm % mutually recursive function

https://chromiumcodereview.appspot.com/17635005/diff/7001/scripts/common/anno...
File scripts/common/annotator.py (right):

https://chromiumcodereview.appspot.com/17635005/diff/7001/scripts/common/anno...
scripts/common/annotator.py:526: v = force_list_str(v)
you want elif isinstance(v, dict) up here too, for these two to be truly
mutually recursive.

[iannucci, 2013-06-25 21:31:41]

On 2013/06/25 21:26:35, Aaron Gable wrote:
> lgtm % mutually recursive function
> 
>
https://chromiumcodereview.appspot.com/17635005/diff/7001/scripts/common/anno...
> File scripts/common/annotator.py (right):
> 
>
https://chromiumcodereview.appspot.com/17635005/diff/7001/scripts/common/anno...
> scripts/common/annotator.py:526: v = force_list_str(v)
> you want elif isinstance(v, dict) up here too, for these two to be truly
> mutually recursive.

blargh. I wish I didn't have to do this conversion at all :/

[commit-bot: I haz the power, 2013-06-25 22:11:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/17635005/24001

[commit-bot: I haz the power, 2013-06-25 22:12:56]

Presubmit check for 17635005-24001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Pylint (381 files) failed
************* Module common.annotator
W0611: 16,0: Unused import itertools
************* Module slave.annotated_run
W0611: 70,0: Unused import recipe_polyfill
************* Module slave.recipe_api
E0602: 36,34:InputDataPlaceholder.render: Undefined variable 'tempfile'
************* Module slave.recipe_polyfill
W0231: 39,4:OrderedDict.__init__: __init__ method from base class 'dict' is not
called
E0211:155,4:OrderedDict.update: Method has no argument
W0102:211,4:OrderedDict.__repr__: Dangerous default value {} as argument
W0221:282,4:SupportTestLoaderProtocol.loadTestsFromModule: Arguments number
differs from overridden method
************* Module recipes_test
W0611: 51,0: Unused import recipe_polyfill


scripts/common/unittests/annotator_test.py failed
.........FFF...FFFFFF.
======================================================================
FAIL: testAlwaysRun (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 361, in testAlwaysRun
    self.assertTrue('yo!' in self.capture.text)
AssertionError: False is not true

======================================================================
FAIL: testAlwaysRunNoDupes (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 372, in
testAlwaysRunNoDupes
    self.assertEquals(self.capture.text.count('yo!'), 1)
AssertionError: 0 != 1

======================================================================
FAIL: testCwd (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 399, in testCwd
    self.assertTrue(tmpdir in self.capture.text)
AssertionError: False is not true

======================================================================
FAIL: testSimpleExecution (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 326, in
testSimpleExecution
    self.assertEquals(result, self.capture.text)
AssertionError: Lists differ: ['', '@@@SEED_STEP one@@@', ''... != ['',
'@@@SEED_STEP one@@@', ''...

First differing element 20:
hello!
!hello!

Diff is 1040 characters long. Set self.maxDiff to None to see it.

======================================================================
FAIL: testSkip (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 384, in testSkip
    self.assertEquals(self.capture.text.count('yo!'), 1)
AssertionError: 0 != 1

======================================================================
FAIL: testStepEnvAdd (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 421, in testStepEnvAdd
    self.assertTrue('blah-blah' in self.capture.text)
AssertionError: False is not true

======================================================================
FAIL: testStepEnvKeep (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 410, in
testStepEnvKeep
    self.assertTrue('blah-blah' in self.capture.text)
AssertionError: False is not true

======================================================================
FAIL: testStepEnvRemove (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 444, in
testStepEnvRemove
    self.assertTrue('SOME_ENV is set: False' in self.capture.text)
AssertionError: False is not true

======================================================================
FAIL: testStepEnvReplace (__main__.TestExecution)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "scripts/common/unittests/annotator_test.py", line 432, in
testStepEnvReplace
    self.assertTrue('two' in self.capture.text)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 22 tests in 6.999s

FAILED (failures=9)


Presubmit checks took 43.9s to calculate.

[iannucci, 2013-06-25 22:43:09]

PTAL again

[commit-bot: I haz the power, 2013-06-25 23:38:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/17635005/24002

[commit-bot: I haz the power, 2013-06-25 23:40:03]

Change committed as 208578

[Isaac (away), 2013-06-29 05:34:01]

https://chromiumcodereview.appspot.com/17635005/diff/24002/scripts/slave/reci...
File scripts/slave/recipe_modules/gclient/api.py (right):

https://chromiumcodereview.appspot.com/17635005/diff/24002/scripts/slave/reci...
scripts/slave/recipe_modules/gclient/api.py:115: ['.',
self.m.path.depot_tools('gclient', wrapper=True)],
I think this should be gclient.py.  Recipes should probably enforce gclient is
not called directly because the auto-update mechanism makes builds non-hermetic.

[iannucci, 2013-06-29 06:05:41]

On 2013/06/29 05:34:01, Isaac wrote:
>
https://chromiumcodereview.appspot.com/17635005/diff/24002/scripts/slave/reci...
> File scripts/slave/recipe_modules/gclient/api.py (right):
> 
>
https://chromiumcodereview.appspot.com/17635005/diff/24002/scripts/slave/reci...
> scripts/slave/recipe_modules/gclient/api.py:115: ['.',
> self.m.path.depot_tools('gclient', wrapper=True)],
> I think this should be gclient.py.  Recipes should probably enforce gclient is
> not called directly because the auto-update mechanism makes builds
non-hermetic.

Good catch. I missed it because it's not being directly invoked.