Use subprocess42 in recipe_engine to avoid threading madness.

Use subprocess42 in recipe_engine to avoid threading madness.

R=estaab@chromium.org, martiniss@chromium.org, maruel@chromium.org, tansell@chromium.org
BUG=522396
Committed: https://github.com/luci/recipes-py/commit/614e66740642550cba38bc78a2776866dae66ea6

[M-A Ruel, 2016-05-06 14:12:52]

lgtm

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:23: if sys.platform.startswith('win'):
if sys.platform == 'win32':

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:33: ctypes.windll.kernel32.SetErrorMode(0x0002)
s/0x0002/2/ ? :)

That's a good idea! I hadn't thought about that.

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:34: 
you don't use 2 lines between file level symbols?

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:229: cmd.extend(['%s=%s' % (k,repr(v)) for k, v in
properties.iteritems()])
[OT] don't you want to sort the resulting list just for determinism?

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:322: if len(linebufs) > 0:
if linebufs:

[iannucci, 2016-05-06 17:52:01]

OK, PTAL
  * made linebuffer more efficient for very long lines (no quadradic allocation:
some chromium tests emit megabytes of data on a single line, because they hate
everything)
  * made subprocess42 a property-controllable mode
  * extended tests to run both with and without subprocess42

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:23: if sys.platform.startswith('win'):
On 2016/05/06 14:12:51, M-A Ruel wrote:
> if sys.platform == 'win32':

Done.

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:33: ctypes.windll.kernel32.SetErrorMode(0x0002)
On 2016/05/06 14:12:51, M-A Ruel wrote:
> s/0x0002/2/ ? :)

I was matching the text in the msdn docs :)

Looking at the docs closer, I think we should be setting all of those error
flags.

> 
> That's a good idea! I hadn't thought about that.

Should I move this to subprocess42?

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:34: 
On 2016/05/06 14:12:51, M-A Ruel wrote:
> you don't use 2 lines between file level symbols?

lol, done

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:229: cmd.extend(['%s=%s' % (k,repr(v)) for k, v in
properties.iteritems()])
On 2016/05/06 14:12:51, M-A Ruel wrote:
> [OT] don't you want to sort the resulting list just for determinism?

I mean... probably, but this code is going to be reworked when I do DM
integration anyhow.

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:322: if len(linebufs) > 0:
On 2016/05/06 14:12:52, M-A Ruel wrote:
> if linebufs:

Done.

[M-A Ruel, 2016-05-06 17:57:51]

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:33: ctypes.windll.kernel32.SetErrorMode(0x0002)
On 2016/05/06 17:52:01, iannucci wrote:
> > That's a good idea! I hadn't thought about that.
> 
> Should I move this to subprocess42?

Good question. I'm not fond of changing the global state automatically. I guess
an helper function could be useful, but the real saving is just about
copy-pasting 3 lines which I don't see as being a big deal. As you want.

[iannucci, 2016-05-06 18:02:33]

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:313: PIPE = subprocess.PIPE
PIPE is actually always the same value (subprocess42 re-uses the subprocess
one), but for clarity's sake, I pulled them out.

https://codereview.chromium.org/1959563002/diff/60001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1959563002/diff/60001/recipe_engine/run.py#ne...
recipe_engine/run.py:210: engine = RecipeEngine(step_runner, properties,
universe)
because this __init__ can print, I wanted to make sure it shows up inside this
step.

[iannucci, 2016-05-06 18:03:17]

On 2016/05/06 17:57:51, M-A Ruel wrote:
>
https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
> File recipe_engine/step_runner.py (right):
> 
>
https://codereview.chromium.org/1959563002/diff/20001/recipe_engine/step_runn...
> recipe_engine/step_runner.py:33: ctypes.windll.kernel32.SetErrorMode(0x0002)
> On 2016/05/06 17:52:01, iannucci wrote:
> > > That's a good idea! I hadn't thought about that.
> > 
> > Should I move this to subprocess42?
> 
> Good question. I'm not fond of changing the global state automatically. I
guess
> an helper function could be useful, but the real saving is just about
> copy-pasting 3 lines which I don't see as being a big deal. As you want.

Ok, I'll leave it here. I'll add the additional flags in a followup CL.

[martiniss, 2016-05-06 18:09:14]

lgtm

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/run.py#ne...
recipe_engine/run.py:306: if '$recipe_engine' in properties:
I don't think the engine does any parsing of the properties into python stuff,
so I don't think these will be dictionaries; i think they'll still be strings?
But maybe I'm wrong?

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:254: retcode = subprocess42.call(cmd)
wait wat? is this right??

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/unittests...
File recipe_engine/unittests/run_test.py (right):

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/unittests...
recipe_engine/unittests/run_test.py:67: props.update(ENABLE_SUBPROCESS42)
Why are we using subprocess42 for all of these tests?

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/unittests...
recipe_engine/unittests/run_test.py:90: def test_nonexistent_command_s42(self):
Where did these new tests come from? They look like duplicated tests which use
subprocess42, correcT?

[iannucci, 2016-05-06 18:18:04]

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/run.py#ne...
recipe_engine/run.py:306: if '$recipe_engine' in properties:
On 2016/05/06 18:09:14, martiniss wrote:
> I don't think the engine does any parsing of the properties into python stuff,
> so I don't think these will be dictionaries; i think they'll still be strings?
> But maybe I'm wrong?

you are wrong :) All properties are json. I have tests if you don't believe me
;)

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:254: retcode = subprocess42.call(cmd)
On 2016/05/06 18:09:14, martiniss wrote:
> wait wat? is this right??

oops. (not that this one matters particularly much, but good catch)

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/unittests...
File recipe_engine/unittests/run_test.py (right):

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/unittests...
recipe_engine/unittests/run_test.py:67: props.update(ENABLE_SUBPROCESS42)
On 2016/05/06 18:09:14, martiniss wrote:
> Why are we using subprocess42 for all of these tests?

So... that we can test it? I'm running them all with and without subprocess42
(see first line of the loop)

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/unittests...
recipe_engine/unittests/run_test.py:90: def test_nonexistent_command_s42(self):
On 2016/05/06 18:09:14, martiniss wrote:
> Where did these new tests come from? They look like duplicated tests which use
> subprocess42, correcT?

yes

[martiniss, 2016-05-06 18:19:05]

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/unittests...
File recipe_engine/unittests/run_test.py (right):

https://codereview.chromium.org/1959563002/diff/40001/recipe_engine/unittests...
recipe_engine/unittests/run_test.py:67: props.update(ENABLE_SUBPROCESS42)
On 2016/05/06 at 18:18:04, iannucci wrote:
> On 2016/05/06 18:09:14, martiniss wrote:
> > Why are we using subprocess42 for all of these tests?
> 
> So... that we can test it? I'm running them all with and without subprocess42
(see first line of the loop)

ah missed that.

[estaab, 2016-05-06 19:05:43]

lgtm

[iannucci, 2016-05-06 19:19:45]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2016-05-06 19:19:45]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org,
martiniss@chromium.org
Link to the patchset: https://codereview.chromium.org/1959563002/#ps80001
(title: "oops")

[commit-bot: I haz the power, 2016-05-06 19:19:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1959563002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1959563002/80001

[iannucci, 2016-05-06 19:22:58]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2016-05-06 19:22:59]

The patchset sent to the CQ was uploaded after l-g-t-m from estaab@chromium.org,
maruel@chromium.org, martiniss@chromium.org
Link to the patchset: https://codereview.chromium.org/1959563002/#ps100001
(title: "*sigh* presubmit")

[commit-bot: I haz the power, 2016-05-06 19:23:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1959563002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1959563002/100001

[commit-bot: I haz the power, 2016-05-06 19:26:05]

Description was changed from

==========
Use subprocess42 in recipe_engine to avoid threading madness.

R=estaab@chromium.org, martiniss@chromium.org, maruel@chromium.org,
tansell@chromium.org
BUG=522396
==========

to

==========
Use subprocess42 in recipe_engine to avoid threading madness.

R=estaab@chromium.org, martiniss@chromium.org, maruel@chromium.org,
tansell@chromium.org
BUG=522396

Committed:
https://github.com/luci/recipes-py/commit/614e66740642550cba38bc78a2776866dae...
==========

[commit-bot: I haz the power, 2016-05-06 19:26:07]

Committed patchset #6 (id:100001) as
https://github.com/luci/recipes-py/commit/614e66740642550cba38bc78a2776866dae...