[Android] Fix environment variable handling in devil_env.

[Android] Fix environment variable handling in devil_env.

BUG=

Committed: https://crrev.com/771d799f0bc3e716974b218c0a133de976ccd4ad
Cr-Commit-Position: refs/heads/master@{#363768}

[jbudorick, 2015-12-08 02:40:13]

jbudorick@chromium.org changed reviewers:
+ mikecase@chromium.org

[mikecase (-- gone --), 2015-12-08 03:44:27]

Getting off bus, so sending out my comments now.

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
build/android/devil/devil_env.py:65: 
This change just adds the 'config_type': 'BaseConfig' line, right? Nothing else
has changed. I mean, I have no idea what this dict does, so I am assuming it is
not correct.

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
File build/android/devil/devil_env_test.py (right):

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
build/android/devil/devil_env_test.py:26: _sys_path_with_pymock)
I think its kinda weird that you are testing being able to import pymock when
pretty much nothing is going to work at all if the import fails.

Like, won't you just get an import errors in the "import mock" line before this
test ever runs.

I guess the assert euqals _sys_path_before, _sys_path_after part is a useful
test.

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
build/android/devil/devil_env_test.py:51: 'android_linux2': {
where is "android_linux2" coming from?

[jbudorick, 2015-12-08 03:48:17]

jbudorick@chromium.org changed reviewers:
+ rnephew@chromium.org

[jbudorick, 2015-12-08 03:48:17]

+rnephew

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
build/android/devil/devil_env.py:40: 'platform': 'android_linux2',
android_linux2 comes from here.

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
build/android/devil/devil_env.py:65: 
On 2015/12/08 03:44:27, mikecase wrote:
> This change just adds the 'config_type': 'BaseConfig' line, right? Nothing
else
> has changed. I mean, I have no idea what this dict does, so I am assuming it
is
> not correct.

this change both adds 'config_type' and moves the existing dict to
'dependencies'

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
File build/android/devil/devil_env_test.py (right):

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
build/android/devil/devil_env_test.py:26: _sys_path_with_pymock)
On 2015/12/08 03:44:27, mikecase wrote:
> I think its kinda weird that you are testing being able to import pymock when
> pretty much nothing is going to work at all if the import fails.

this isn't testing the import of pymock specifically; it's testing the path
manipulation that SysPath does.

> 
> Like, won't you just get an import errors in the "import mock" line before
this
> test ever runs.

not necessarily; this is also validating the order.

> 
> I guess the assert euqals _sys_path_before, _sys_path_after part is a useful
> test.

yep

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
build/android/devil/devil_env_test.py:51: 'android_linux2': {
On 2015/12/08 03:44:27, mikecase wrote:
> where is "android_linux2" coming from?

see other file

[mikecase (-- gone --), 2015-12-08 04:27:22]

lgtm

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1509493003/diff/40001/build/android/devil/dev...
build/android/devil/devil_env.py:65: 
On 2015/12/08 at 03:48:17, jbudorick wrote:
> On 2015/12/08 03:44:27, mikecase wrote:
> > This change just adds the 'config_type': 'BaseConfig' line, right? Nothing
else
> > has changed. I mean, I have no idea what this dict does, so I am assuming it
is
> > not correct.
> 
> this change both adds 'config_type' and moves the existing dict to
'dependencies'

oops, typo in my first comment, I meant to say I am assuming this new dict IS
correct. looks good.

[jbudorick, 2015-12-08 04:29:36]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-12-08 04:30:01]

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

[jbudorick, 2015-12-08 04:30:03]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2015-12-08 04:30:36]

On 2015/12/08 04:30:01, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1509493003/40001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1509493003/40001

pulled back out because the windows failures are legitimate.

[jbudorick, 2015-12-08 04:54:53]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-12-08 04:54:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
mikecase@chromium.org
Link to the patchset: https://codereview.chromium.org/1509493003/#ps60001
(title: "windows-friendly environment initialization")

[commit-bot: I haz the power, 2015-12-08 04:55:28]

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

[commit-bot: I haz the power, 2015-12-08 09:07:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-08 09:08:12]

Description was changed from

==========
[Android] Fix environment variable handling in devil_env.

BUG=
==========

to

==========
[Android] Fix environment variable handling in devil_env.

BUG=

Committed: https://crrev.com/771d799f0bc3e716974b218c0a133de976ccd4ad
Cr-Commit-Position: refs/heads/master@{#363768}
==========

[commit-bot: I haz the power, 2015-12-08 09:08:12]

Patchset 4 (id:??) landed as
https://crrev.com/771d799f0bc3e716974b218c0a133de976ccd4ad
Cr-Commit-Position: refs/heads/master@{#363768}