Android: allows using adb from a chromium checkout without envsetup.sh

Android: allows using adb from a chromium checkout without envsetup.sh

There are situations like running tests where it's not necessary to run envsetup.sh.
However, build/android/pylib requires adb to be in the path.
Rather than requiring envsetup, it can be smart enough to set the path itself.

BUG=242960
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202558

[bulach, 2013-05-23 16:52:49]

main motivation for this is to be able to run tests without envsetup.sh, see bug
for the cc_perftests.
ptal

[tonyg, 2013-05-23 17:10:05]

lgtm

Good catch. So this fixes the cc_perftests?

https://codereview.chromium.org/15891002/diff/1001/build/android/pylib/__init...
File build/android/pylib/__init__.py (right):

https://codereview.chromium.org/15891002/diff/1001/build/android/pylib/__init...
build/android/pylib/__init__.py:11: import subprocess
This should be at the top of the file

[bulach, 2013-05-23 17:29:03]

well.... :)

I've been thinking I'm done with this cc_perftests for a long, long time, every
patch I try highlights something else... :-/

but yeah, with this one it's possible to run cc_perftests for android without
running any envsetup.sh, which looks like it's the (current) failure.

https://codereview.chromium.org/15891002/diff/1001/build/android/pylib/__init...
File build/android/pylib/__init__.py (right):

https://codereview.chromium.org/15891002/diff/1001/build/android/pylib/__init...
build/android/pylib/__init__.py:11: import subprocess
On 2013/05/23 17:10:05, tonyg wrote:
> This should be at the top of the file

Done.

[tonyg, 2013-05-23 18:20:22]

Still lgtm

[frankf, 2013-05-23 19:51:38]

IIRC, for some reason odd reason, the upstream bots use adb from build_internal
(scripts/slave/android). Adding Armand, who should know why and whether there's
some version mismatching issues.

[navabi1, 2013-05-23 22:35:06]

+ilevy

The change was made by ilevy: https://codereview.chromium.org/11857014
I was not on the review so Isaac may have more answers, but I don't think
it is odd.  The infrastructure team would like to have control over the adb
version used on the bots rather than relying on the version in the SDK.

For this particular CL, I think the bots should use the adb in the SDK.
Developers running tests should be testing against the SDK version of adb.


On Thu, May 23, 2013 at 12:51 PM, <frankf@chromium.org> wrote:

> IIRC, for some reason odd reason, the upstream bots use adb from
> build_internal
> (scripts/slave/android). Adding Armand, who should know why and whether
> there's
> some version mismatching issues.
>
>
https://codereview.chromium.**org/15891002/<https://codereview.chromium.org/1...
>

[bulach, 2013-05-24 08:59:16]

sorry, I'm not sure what is the conclusion here? :)
should I go ahead with this?

please note the following scenarios:
- something else sets PATH to contain adb: regardless of running envsetup.sh,
before or after this patch, android_commands.py will use that.

- envsetup.sh is executed: again, it appends to PATH, so it'll use the SDK only
if nothing else is set.

- after this patch: nothing changes :) it appends to PATH, so if something else
is already set, use that... otherwise, use the same thing as envsetup.sh would
have done.

I guess my summary is that this patch doesn't change anything, except the case
where adb can't be found anyway, so it adds a fallback-safety-net... wdyt?

[frankf, 2013-05-24 17:35:51]

lgtm. But the scenario I was thinking: some poor infra folk logs to the upstream
bot and forgets to set the adb path. This patch will pick up the wrong adb
(still not sure what the difference is though) which may or may not lead to
subtle issues in repro'ing some test failures.

On 2013/05/24 08:59:16, bulach wrote:
> sorry, I'm not sure what is the conclusion here? :)
> should I go ahead with this?
> 
> please note the following scenarios:
> - something else sets PATH to contain adb: regardless of running envsetup.sh,
> before or after this patch, android_commands.py will use that.
> 
> - envsetup.sh is executed: again, it appends to PATH, so it'll use the SDK
only
> if nothing else is set.
> 
> - after this patch: nothing changes :) it appends to PATH, so if something
else
> is already set, use that... otherwise, use the same thing as envsetup.sh would
> have done.
> 
> I guess my summary is that this patch doesn't change anything, except the case
> where adb can't be found anyway, so it adds a fallback-safety-net... wdyt?

[navabi, 2013-05-24 18:16:51]

Marcus, this lgtm.

The infra folks should know which adb to use. Perhaps a warning print statement
in the except block would be useful. I'm fine with this change either way.

On 2013/05/24 17:35:51, frankf wrote:
> lgtm. But the scenario I was thinking: some poor infra folk logs to the
upstream
> bot and forgets to set the adb path. This patch will pick up the wrong adb
> (still not sure what the difference is though) which may or may not lead to
> subtle issues in repro'ing some test failures.
> 
> On 2013/05/24 08:59:16, bulach wrote:
> > sorry, I'm not sure what is the conclusion here? :)
> > should I go ahead with this?
> > 
> > please note the following scenarios:
> > - something else sets PATH to contain adb: regardless of running
envsetup.sh,
> > before or after this patch, android_commands.py will use that.
> > 
> > - envsetup.sh is executed: again, it appends to PATH, so it'll use the SDK
> only
> > if nothing else is set.
> > 
> > - after this patch: nothing changes :) it appends to PATH, so if something
> else
> > is already set, use that... otherwise, use the same thing as envsetup.sh
would
> > have done.
> > 
> > I guess my summary is that this patch doesn't change anything, except the
case
> > where adb can't be found anyway, so it adds a fallback-safety-net... wdyt?

[bulach, 2013-05-24 18:23:59]

thanks everyone! 
added a message, please let me know if it addresses all your concerns.

[navabi, 2013-05-24 18:34:17]

On 2013/05/24 18:23:59, bulach wrote:
> thanks everyone! 
> added a message, please let me know if it addresses all your concerns.

LGTM. Thanks Marcus.

[Isaac (away), 2013-05-24 19:17:01]

looks good :-).  two nits inline

https://chromiumcodereview.appspot.com/15891002/diff/14001/build/android/pyli...
File build/android/pylib/__init__.py (right):

https://chromiumcodereview.appspot.com/15891002/diff/14001/build/android/pyli...
build/android/pylib/__init__.py:13: subprocess.call(['adb', 'version'])
How about 'which adb' instead?  Also you need to pipe output /stderr to null

https://chromiumcodereview.appspot.com/15891002/diff/14001/build/android/pyli...
build/android/pylib/__init__.py:18: '..', '..', '..',
can we import constants.py and use the sdk_dir here?

[bulach, 2013-05-28 09:12:25]

thanks isaac! all addressed, CQing..

https://codereview.chromium.org/15891002/diff/14001/build/android/pylib/__ini...
File build/android/pylib/__init__.py (right):

https://codereview.chromium.org/15891002/diff/14001/build/android/pylib/__ini...
build/android/pylib/__init__.py:13: subprocess.call(['adb', 'version'])
On 2013/05/24 19:17:01, Isaac wrote:
> How about 'which adb' instead?  Also you need to pipe output /stderr to null

good points, done both.

https://codereview.chromium.org/15891002/diff/14001/build/android/pylib/__ini...
build/android/pylib/__init__.py:18: '..', '..', '..',
On 2013/05/24 19:17:01, Isaac wrote:
> can we import constants.py and use the sdk_dir here?

I don't think so, this is the module's __init__, I think its constants.py is not
yet available.

[commit-bot: I haz the power, 2013-05-28 10:47:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/15891002/20001

[commit-bot: I haz the power, 2013-05-28 14:27:33]

Change committed as 202558

[Isaac (away), 2013-05-31 19:34:06]

So this is messing with PATH in buildbot code where we import pylint for
buildbot_report.  Thinking about this more thoroughly, I think it wrong to
modify os.environ of the python process when a module is imported.  I think it'd
be cleaner to use this code in a GetAdb() function in constants.py and replace
calls to adb with that.  Other calls which depend on the android sdk binary
folder in the path can be replaced with absolute calls using constants.py to get
SDK_PATH.