Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
103803006:
Checkout tarball for chrome and blink for initial sync on a bot (Closed)
|
Created:
7 years ago by Ryan Tseng Modified:
6 years, 11 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. |
DescriptionCheckout tarball for chrome and blink for initial sync on a bot
BUG=327494
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=243221
Patch Set 1 #Patch Set 2 : Added comments #Patch Set 3 : Reverted no-op change #
Total comments: 16
Patch Set 4 : Review fix #Patch Set 5 : Switched from tarfile for extraction to calling tar #Patch Set 6 : Switched to using gclient_utils.rmtree #Patch Set 7 : Indents and todo #
Total comments: 6
Patch Set 8 : Review fix #Patch Set 9 : Review fix #
Total comments: 1
Patch Set 10 : nit comma #Messages
Total messages: 16 (0 generated)
So everytime we reimage the ccompute fleet, or when the trybot blow away their svn checkout, the svn mirrors end up melting and we lose an entire day of trybot usage. This is my proposed solution to the problem - Just check out an SVN tarball for Chrome and Blink. WDYT? There would be another cron job somewhere that updates the svn tarball.
Tested locally and on a bot, seems to work
Friendly ping :)
https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1183 gclient_scm.py:1183: 'gs_path': '/chromium-svn-checkout/chrome/' Prepend gs:/ to this and on line 1186. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1191 gclient_scm.py:1191: # Split out the revision number, its not useful for us. number, its -> number since it's https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1199 gclient_scm.py:1199: boto_path=os.devnull) Indent needs to be fixed. It would be better as: gsutil = download_from_google_storage.Gsutil( GSUTIL_DEFAULT_PATH, boto_path=os.devnull) https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1203 gclient_scm.py:1203: _, out, _ = gsutil.check_call('ls', 'gs:/%s' % gs_path) Just use gs_path here (no string shenanigans req'd). https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1218 gclient_scm.py:1218: f.extractall(self.checkout_path) Use a local executable to unpack rather than Python. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1234 gclient_scm.py:1234: local_root, self.checkout_path], Indent needs to be less one character on line 1234.
Ryan, is this ready to go?
https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1183 gclient_scm.py:1183: 'gs_path': '/chromium-svn-checkout/chrome/' On 2013/12/26 19:25:45, cmp wrote: > Prepend gs:/ to this and on line 1186. Done. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1191 gclient_scm.py:1191: # Split out the revision number, its not useful for us. On 2013/12/26 19:25:45, cmp wrote: > number, its -> number since it's Done. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1199 gclient_scm.py:1199: boto_path=os.devnull) On 2013/12/26 19:25:45, cmp wrote: > Indent needs to be fixed. It would be better as: > > gsutil = download_from_google_storage.Gsutil( > GSUTIL_DEFAULT_PATH, boto_path=os.devnull) Done. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1203 gclient_scm.py:1203: _, out, _ = gsutil.check_call('ls', 'gs:/%s' % gs_path) On 2013/12/26 19:25:45, cmp wrote: > Just use gs_path here (no string shenanigans req'd). Done. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1218 gclient_scm.py:1218: f.extractall(self.checkout_path) On 2013/12/26 19:25:45, cmp wrote: > Use a local executable to unpack rather than Python. This would make gclient_scm pretty unportable :( To do this we'd need to detect the user's platform, and download the appropriate version of 7zip into depot_tools, as we do with svn/git. I'm not convince we'd get anything out of that. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1234 gclient_scm.py:1234: local_root, self.checkout_path], On 2013/12/26 19:25:45, cmp wrote: > Indent needs to be less one character on line 1234. Done.
https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1218 gclient_scm.py:1218: f.extractall(self.checkout_path) On 2014/01/07 00:57:25, hinoka wrote: > On 2013/12/26 19:25:45, cmp wrote: > > Use a local executable to unpack rather than Python. > > This would make gclient_scm pretty unportable :( > To do this we'd need to detect the user's platform, and download the appropriate > version of 7zip into depot_tools, as we do with svn/git. I'm not convince we'd > get anything out of that. You're assuming too much about how well an in-process Python module works to unpack large files. If you've tested this on compressed archives that are the size of the Chrome and Blink checkouts packed, okay. But if you haven't tested it, I can tell you that we used to use in-process Python modules for compression and decompression and it did not work well. We switched to use 7z. Such a change to use a separate executable wouldn't make gclient_scm unportable in general. It would make it unportable just for the bots, and in those environments we have control over what's available. We already have a code branch here that is Linux specific. In that branch, why not call out to gzip? In the Windows code branch, why not call out to 7z? etc. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1220 gclient_scm.py:1220: shutil.rmtree(tempdir) We found rmtree() to cause problems on Windows. We used rd instead. The same may be true on other platforms. This is the same issue as the compress/decompress executable problem (something we hit a long time ago, solved with a separate executable, and it's been working great ever since implementing that decision).
ptal https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1218 gclient_scm.py:1218: f.extractall(self.checkout_path) On 2014/01/07 01:09:52, cmp wrote: > On 2014/01/07 00:57:25, hinoka wrote: > > On 2013/12/26 19:25:45, cmp wrote: > > > Use a local executable to unpack rather than Python. > > > > This would make gclient_scm pretty unportable :( > > To do this we'd need to detect the user's platform, and download the > appropriate > > version of 7zip into depot_tools, as we do with svn/git. I'm not convince > we'd > > get anything out of that. > > You're assuming too much about how well an in-process Python module works to > unpack large files. If you've tested this on compressed archives that are the > size of the Chrome and Blink checkouts packed, okay. But if you haven't tested > it, I can tell you that we used to use in-process Python modules for compression > and decompression and it did not work well. We switched to use 7z. > > Such a change to use a separate executable wouldn't make gclient_scm unportable > in general. It would make it unportable just for the bots, and in those > environments we have control over what's available. We already have a code > branch here that is Linux specific. In that branch, why not call out to gzip? > In the Windows code branch, why not call out to 7z? etc. How big of a tarball and what sort of problems are we talking about? Chrome is 500mb compressed. I'll use "tar -zxf" for linux, since I havn't seen a distro that doesn't include tar, and add a todo for windows. https://codereview.chromium.org/103803006/diff/40001/gclient_scm.py#newcode1220 gclient_scm.py:1220: shutil.rmtree(tempdir) On 2014/01/07 01:09:52, cmp wrote: > We found rmtree() to cause problems on Windows. We used rd instead. The same > may be true on other platforms. This is the same issue as the > compress/decompress executable problem (something we hit a long time ago, solved > with a separate executable, and it's been working great ever since implementing > that decision). Just realized gclient_utils.rmtree does exactly what you said, switched to using that.
https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py#newcode1215 gclient_scm.py:1215: if sys.platform == 'linux2': This code exists in a branch that already tested sys.platform for if they compare to 'linux2', so I think line 1215 and lines 1222-1224 should be deleted, and lines 1216-1221 should be de-indented.
https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py#newcode1181 gclient_scm.py:1181: 'gs_path': 'gs://chromium-svn-checkout/chrome/' indent mismatch (+2 on line 1180, +4 on line 1181), check the style guide for the correct indent append a comma to line 1181 and line 1184 Come to think of it, what else would you ever put into this dict? If the answer is 'nothing else', then remove the dict and have /chrome/trunk/src just point to the string GS URL gs_path points to on line 1181, and then the same change for line 1183. https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py#newcode1183 gclient_scm.py:1183: '/blink/trunk' : { remove space before :
https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py#newcode1181 gclient_scm.py:1181: 'gs_path': 'gs://chromium-svn-checkout/chrome/' On 2014/01/07 02:10:14, cmp wrote: > indent mismatch (+2 on line 1180, +4 on line 1181), check the style guide for > the correct indent > > append a comma to line 1181 and line 1184 > > Come to think of it, what else would you ever put into this dict? If the answer > is 'nothing else', then remove the dict and have /chrome/trunk/src just point to > the string GS URL gs_path points to on line 1181, and then the same change for > line 1183. Done. https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py#newcode1183 gclient_scm.py:1183: '/blink/trunk' : { On 2014/01/07 02:10:14, cmp wrote: > remove space before : Done. https://codereview.chromium.org/103803006/diff/180001/gclient_scm.py#newcode1215 gclient_scm.py:1215: if sys.platform == 'linux2': On 2014/01/07 02:06:25, cmp wrote: > This code exists in a branch that already tested sys.platform for if they > compare to 'linux2', so I think line 1215 and lines 1222-1224 should be deleted, > and lines 1216-1221 should be de-indented. Done.
lgtm with nit Before you land this, okease put this on a Linux bot in GCompute, remove the local hack gclient shell script, and verify that what happens is what you expect. Maybe would be good to do this on a Golo Linux VM, too. https://codereview.chromium.org/103803006/diff/240001/gclient_scm.py File gclient_scm.py (right): https://codereview.chromium.org/103803006/diff/240001/gclient_scm.py#newcode1181 gclient_scm.py:1181: '/blink/trunk': 'gs://chromium-svn-checkout/blink/' append a comma
ssh-ed into slave302-c4 rm ~/slavebin/gclient cd /b/depot_tools wget https://codereview.chromium.org/download/issue103803006_280001.diff patch < issue103803006_280001.diff rm /b/build/slave/linux_aura/build/src chrome-bot@slave302-c4:/b/build/slave/linux_aura/build$ CHROME_HEADLESS=1 gclient sync Downloading gs://chromium-svn-checkout/chrome/chrome-src-239156.tar.gz... Unpacking into /mnt/scratch0/b_used/build/slave/linux_aura/build/src... Deleting temp file ________ running 'svn checkout svn://svn-mirror.golo.chromium.org/chrome/trunk/src /mnt/scratch0/b_used/build/slave/linux_aura/build/src --ignore-externals' in '/mnt/scratch0/b_used/build/slave/linux_aura/build' .... Working as expected (The base url switch wasn't done because the original tarfile was checked out from svn-mirror.golo.chromium.org) I'll wait for the entire gclient sync to finish, and then deploy
Tested on golo bot too, seems to work, gonna deploy
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hinoka@google.com/103803006/280001
Message was sent while issue was closed.
Change committed as 243221 |
