Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
148613003:
Pull toolchain isos from gs:// when on bots (Closed)
|
Created:
6 years, 11 months ago by scottmg Modified:
6 years, 10 months ago CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, M-A Ruel, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Visibility:
Public. |
DescriptionPull toolchain isos from gs:// when on bots
VS2013 isos are mirrored to gs://chrome-wintoolchain/ (which is googler-
only accessible). This is used to isolate bots from external dependency
as detected by CHROME_HEADLESS=1 in the environment.
TBR=iannucci@chromium.org
R=hinoka@chromium.org
BUG=324987
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=247398
Patch Set 1 #
Total comments: 2
Patch Set 2 : use download_from_google_storage instead #
Total comments: 2
Patch Set 3 : boto_path=None #Patch Set 4 : switch to call so that there's output during download #
Total comments: 1
Patch Set 5 : fix return of call #
Total comments: 6
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/148613003/diff/1/win_toolchain/toolchain2013.py File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/148613003/diff/1/win_toolchain/toolchain2013.... win_toolchain/toolchain2013.py:163: RunOrDie([ can use import download_from_google_storage ... gsutil = download_from_google_storage.Gsutil( download_from_google_storage.GSUTIL_DEFAULT_PATH) gsutil.check_call('cp', '...', '...') code, _, err = gsutil.check_call('cp', '-q', file_url, output_filename) if code != 0: raise SystemExit('Gsutil error: %s' % err) That allows this to deal with some .boto.depot_tools shenanigans that we do on some of our bots, and centralizes gsutil calls within depot_tools through that method.
https://codereview.chromium.org/148613003/diff/1/win_toolchain/toolchain2013.py File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/148613003/diff/1/win_toolchain/toolchain2013.... win_toolchain/toolchain2013.py:163: RunOrDie([ On 2014/01/28 00:19:43, Ryan T. wrote: > can use > import download_from_google_storage > ... > > gsutil = download_from_google_storage.Gsutil( > download_from_google_storage.GSUTIL_DEFAULT_PATH) > gsutil.check_call('cp', '...', '...') > code, _, err = gsutil.check_call('cp', '-q', file_url, output_filename) > if code != 0: > raise SystemExit('Gsutil error: %s' % err) > > That allows this to deal with some .boto.depot_tools shenanigans that we do on > some of our bots, and centralizes gsutil calls within depot_tools through that > method. Thanks, done. Is os.devnull OK for boto_path? That's the best guess I could find here: https://code.google.com/p/chromium/codesearch#search/&q=download_from_google_... FWIW, I would have misused Gsutil.check_call without your example code, as it matches subprocess.check_call in name, but doesn't check != 0 and raise CalledProcessError as subprocess does.
Yeah, call and check_call should probably be renamed to call_interactive and call_and_get_output (Not related to this CL, more of something for me to fix at some point). https://codereview.chromium.org/148613003/diff/40001/win_toolchain/toolchain2... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/148613003/diff/40001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:168: download_from_google_storage.GSUTIL_DEFAULT_PATH, os.devnull) Don't use os.devnull. I assume this is an authenticated bucket, correct? Passing in os.devnull will cause gsutil to be an unauthenticated instance. Just leave it blank or pass in boto=None.
https://codereview.chromium.org/148613003/diff/40001/win_toolchain/toolchain2... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/148613003/diff/40001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:168: download_from_google_storage.GSUTIL_DEFAULT_PATH, os.devnull) On 2014/01/28 00:55:58, Ryan T. wrote: > Don't use os.devnull. I assume this is an authenticated bucket, correct? > Passing in os.devnull will cause gsutil to be an unauthenticated instance. Just > leave it blank or pass in boto=None. Done. (It's not optional in __init__.)
On 2014/01/28 01:11:43, scottmg wrote: > https://codereview.chromium.org/148613003/diff/40001/win_toolchain/toolchain2... > File win_toolchain/toolchain2013.py (right): > > https://codereview.chromium.org/148613003/diff/40001/win_toolchain/toolchain2... > win_toolchain/toolchain2013.py:168: > download_from_google_storage.GSUTIL_DEFAULT_PATH, os.devnull) > On 2014/01/28 00:55:58, Ryan T. wrote: > > Don't use os.devnull. I assume this is an authenticated bucket, correct? > > Passing in os.devnull will cause gsutil to be an unauthenticated instance. > Just > > leave it blank or pass in boto=None. > > Done. (It's not optional in __init__.) Hm, this doesn't output any progress now, I guess because it's capturing output. That's probably not so good as it might get killed by buildbot being impatient. Is just 'call' ok?
call() works. https://codereview.chromium.org/148613003/diff/80001/win_toolchain/toolchain2... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/148613003/diff/80001/win_toolchain/toolchain2... win_toolchain/toolchain2013.py:169: code, _, err = gsutil.call( call() just returns the returncode, rather than a triplet tuple https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/subp...
On 2014/01/28 01:31:07, Ryan T. wrote: > call() works. > > https://codereview.chromium.org/148613003/diff/80001/win_toolchain/toolchain2... > File win_toolchain/toolchain2013.py (right): > > https://codereview.chromium.org/148613003/diff/80001/win_toolchain/toolchain2... > win_toolchain/toolchain2013.py:169: code, _, err = gsutil.call( > call() just returns the returncode, rather than a triplet tuple > https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/subp... Oops, done. I actually re-ran it this time to, you know, test. :/
I don't have a windows box to test on, but it looks right and I'll take your word for that it runs correctly. lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/148613003/100001
Presubmit check for 148613003-100001 failed and returned exit status 1.
Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
depot_tools/win_toolchain/toolchain2013.py
Presubmit checks took 121.7s to calculate.
Was the presubmit check useful? If not, run "git cl presubmit -v"
to figure out which PRESUBMIT.py was run, then run git blame
on the file to figure out who to ask for help.
Bah. TBR=iannucci
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/148613003/100001
Message was sent while issue was closed.
Change committed as 247398
Message was sent while issue was closed.
Cool TBR, bro :P https://codereview.chromium.org/148613003/diff/100001/win_toolchain/toolchain... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/148613003/diff/100001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:23: sys.path.append(os.path.join(BASEDIR, '..')) ARGH! THE PAIN! (no there's not a better alternative, but that doesn't make it less painful :((((() https://codereview.chromium.org/148613003/diff/100001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:164: temp_dir = TempDir() I really wish that TempDir was a contextlib.contextmanager that deleted the directory... Right now this dir will always leak (especially b/c of the exit later in this function). Another way to do it would be to do: TEMP_DIR = None def TempDir(): if TEMP_DIR is None: TEMP_DIR = mkdtemp atexit.register(nuke_it_dead, TEMP_DIR) return os.path.join(TEMP_DIR, <random junk>) https://codereview.chromium.org/148613003/diff/100001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:171: raise SystemExit('gsutil failed') sys.exit('gsutil failed')
Message was sent while issue was closed.
https://codereview.chromium.org/148613003/diff/100001/win_toolchain/toolchain... File win_toolchain/toolchain2013.py (right): https://codereview.chromium.org/148613003/diff/100001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:23: sys.path.append(os.path.join(BASEDIR, '..')) On 2014/01/28 16:38:33, iannucci wrote: > ARGH! THE PAIN! > > (no there's not a better alternative, but that doesn't make it less painful > :((((() Our 'environment' could use "some work". :( https://codereview.chromium.org/148613003/diff/100001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:164: temp_dir = TempDir() On 2014/01/28 16:38:33, iannucci wrote: > I really wish that TempDir was a contextlib.contextmanager that deleted the > directory... Right now this dir will always leak (especially b/c of the exit > later in this function). Could you explain why it will leak? Any finally blocks (main) will still execute, surely? > > Another way to do it would be to do: > > TEMP_DIR = None > > def TempDir(): > if TEMP_DIR is None: > TEMP_DIR = mkdtemp > atexit.register(nuke_it_dead, TEMP_DIR) > return os.path.join(TEMP_DIR, <random junk>) https://codereview.chromium.org/148613003/diff/100001/win_toolchain/toolchain... win_toolchain/toolchain2013.py:171: raise SystemExit('gsutil failed') On 2014/01/28 16:38:33, iannucci wrote: > sys.exit('gsutil failed') I guess? https://codereview.chromium.org/148523009 http://hg.python.org/cpython/file/ca5431a434d6/Python/sysmodule.c#l207 http://docs.python.org/2/library/sys.html#sys.exit |
