Use CHROME_HEADLESS to check if download_sdk_extras.py is running on a bot.

Use CHROME_HEADLESS to check if download_sdk_extras.py is running on a bot.

This will also change where depot_tools is found. Instead of assuming it is part of the build_bot checkout it will use find_depot_tools.

BUG=454220
Committed: https://crrev.com/b0c3ed39916e25bed2900b653974672a39fcb254
Cr-Commit-Position: refs/heads/master@{#316737}

[navabi, 2015-02-17 20:23:06]

navabi@google.com changed reviewers:
+ cjhopman@chromium.org, hinoka@chromium.org, iannucci@chromium.org,
kjellander@chromium.org, nyquist@chromium.org

[hinoka, 2015-02-17 20:34:01]

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py
File build/download_sdk_extras.py (right):

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:45: if not os.environ.get('CHROME_HEADLESS'):
Or just check if GSUTIL_PATH exists?

we try to avoid using CHROME_HEADLESS unless its really necessary.

[navabi, 2015-02-17 20:38:24]

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py
File build/download_sdk_extras.py (right):

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:45: if not os.environ.get('CHROME_HEADLESS'):
On 2015/02/17 20:34:01, hinoka wrote:
> Or just check if GSUTIL_PATH exists?
> 
> we try to avoid using CHROME_HEADLESS unless its really necessary.

I did that before. But this does not work on WebRTC and some other bots because
their directory structure is not the same. See:
https://codereview.chromium.org/799673003/
for more details.

[hinoka, 2015-02-17 20:40:59]

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py
File build/download_sdk_extras.py (right):

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:45: if not os.environ.get('CHROME_HEADLESS'):
On 2015/02/17 20:38:24, navabi wrote:
> On 2015/02/17 20:34:01, hinoka wrote:
> > Or just check if GSUTIL_PATH exists?
> > 
> > we try to avoid using CHROME_HEADLESS unless its really necessary.
> 
> I did that before. But this does not work on WebRTC and some other bots
because
> their directory structure is not the same. See:
> https://codereview.chromium.org/799673003/
> for more details.

Is switching to "find_depot_tools" sufficient?

[navabi, 2015-02-17 20:45:39]

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py
File build/download_sdk_extras.py (right):

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:45: if not os.environ.get('CHROME_HEADLESS'):
On 2015/02/17 20:40:59, hinoka wrote:
> On 2015/02/17 20:38:24, navabi wrote:
> > On 2015/02/17 20:34:01, hinoka wrote:
> > > Or just check if GSUTIL_PATH exists?
> > > 
> > > we try to avoid using CHROME_HEADLESS unless its really necessary.
> > 
> > I did that before. But this does not work on WebRTC and some other bots
> because
> > their directory structure is not the same. See:
> > https://codereview.chromium.org/799673003/
> > for more details.
> 
> Is switching to "find_depot_tools" sufficient?

No. Because it will find_depot_tools for developers. And then try to download
SDK extras. It will fail there because of authentication. I guess we could let
it get there, and if it doesn't have credentials it will fail anyway. But the
difference here is that it will only try to checkout on bots unless someone
explicitly sets CHROME_HEADLESS, in which case it will fail on authentication.

[hinoka, 2015-02-17 21:29:08]

lgtm

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py
File build/download_sdk_extras.py (right):

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:10: be extracted in the android_tools/sdk/extras
directory.
Add something about how this isn't for public consumption?

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:45: if not os.environ.get('CHROME_HEADLESS'):
On 2015/02/17 20:45:39, navabi wrote:
> On 2015/02/17 20:40:59, hinoka wrote:
> > On 2015/02/17 20:38:24, navabi wrote:
> > > On 2015/02/17 20:34:01, hinoka wrote:
> > > > Or just check if GSUTIL_PATH exists?
> > > > 
> > > > we try to avoid using CHROME_HEADLESS unless its really necessary.
> > > 
> > > I did that before. But this does not work on WebRTC and some other bots
> > because
> > > their directory structure is not the same. See:
> > > https://codereview.chromium.org/799673003/
> > > for more details.
> > 
> > Is switching to "find_depot_tools" sufficient?
> 
> No. Because it will find_depot_tools for developers. And then try to download
> SDK extras. It will fail there because of authentication. I guess we could let
> it get there, and if it doesn't have credentials it will fail anyway. But the
> difference here is that it will only try to checkout on bots unless someone
> explicitly sets CHROME_HEADLESS, in which case it will fail on authentication.

Oh I see, auth issue.

[kjellander_chromium, 2015-02-17 21:33:00]

lgtm with a nit. Thanks for fixing this!

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py
File build/download_sdk_extras.py (right):

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:20: script_dir =
os.path.dirname(os.path.realpath(__file__))
These variables are usually uppercase (constants), so let's make them uppercase
as the ones below are that too, for consistency.

[nyquist, 2015-02-17 21:37:05]

This not only changes the check to CHROME_HEADLESS, but also changes how the
GSUTIL_PATH is calculated. Could you maybe update the CL description with that
as well?

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py
File build/download_sdk_extras.py (right):

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:23: from pylib import constants
Nit: Could this be moved one line down? I find it easier to read to find all the
sys.path.inserts around the same place.

[navabi, 2015-02-17 21:40:07]

New patchsets have been uploaded after l-g-t-m from
hinoka@chromium.org,kjellander@chromium.org

[navabi, 2015-02-17 21:44:05]

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py
File build/download_sdk_extras.py (right):

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:10: be extracted in the android_tools/sdk/extras
directory.
On 2015/02/17 21:29:08, hinoka wrote:
> Add something about how this isn't for public consumption?

Done.

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:20: script_dir =
os.path.dirname(os.path.realpath(__file__))
On 2015/02/17 21:32:59, kjellander wrote:
> These variables are usually uppercase (constants), so let's make them
uppercase
> as the ones below are that too, for consistency.

Done.

https://codereview.chromium.org/934753007/diff/1/build/download_sdk_extras.py...
build/download_sdk_extras.py:23: from pylib import constants
On 2015/02/17 21:37:05, nyquist wrote:
> Nit: Could this be moved one line down? I find it easier to read to find all
the
> sys.path.inserts around the same place.

Done.

[navabi, 2015-02-17 21:47:57]

The CQ bit was checked by navabi@google.com

[commit-bot: I haz the power, 2015-02-17 21:49:07]

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

[commit-bot: I haz the power, 2015-02-17 21:55:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-17 21:55:59]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[cjhopman, 2015-02-17 22:27:47]

The CQ bit was checked by cjhopman@chromium.org

[cjhopman, 2015-02-17 22:27:48]

lgtm

[cjhopman, 2015-02-17 22:27:52]

The CQ bit was unchecked by cjhopman@chromium.org

[navabi, 2015-02-17 22:48:49]

The CQ bit was checked by navabi@google.com

[commit-bot: I haz the power, 2015-02-17 22:50:06]

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

[commit-bot: I haz the power, 2015-02-17 23:05:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-17 23:05:09]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[navabi, 2015-02-17 23:29:16]

The CQ bit was checked by navabi@google.com

[commit-bot: I haz the power, 2015-02-17 23:30:30]

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

[commit-bot: I haz the power, 2015-02-17 23:44:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-17 23:44:11]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[navabi, 2015-02-18 00:04:24]

New patchsets have been uploaded after l-g-t-m from cjhopman@chromium.org

[navabi, 2015-02-18 01:08:57]

The CQ bit was checked by navabi@google.com

[commit-bot: I haz the power, 2015-02-18 01:09:43]

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

[commit-bot: I haz the power, 2015-02-18 02:01:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-18 02:02:19]

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