Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
799673003:
Always download google_play_services zip package instead of rsync. (Closed)
|
Created:
5 years, 11 months ago by navabi Modified:
5 years, 10 months ago Reviewers:
cjhopman, kjellander_chromium, hinoka, Raphael Kubo da Costa (rakuco), nyquist, aurimas (slooooooooow), Feng Qian CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlways download google_play_services zip package instead of rsync.
On the bots the rsync technique to download packages (see bug for more details)
takes too long. It takes almost 30 seconds even when the package is already on
the bot. Instead always download the zip and unzip it.
BUG=350151
Committed: https://crrev.com/24606e2106ec9029e042353feaa84555708aefd0
Cr-Commit-Position: refs/heads/master@{#310135}
Patch Set 1 #Patch Set 2 : Fix comment length to be > 80 chars. #
Total comments: 13
Patch Set 3 : Only download if file does not exist, always extract. #Patch Set 4 : Add clean to remove existing package. #
Total comments: 8
Patch Set 5 : Addressed cjhopman's review. #
Total comments: 4
Patch Set 6 : Remove unnecessary comment. #Patch Set 7 : Don't manually check returncode. #
Total comments: 1
Messages
Total messages: 29 (4 generated)
navabi@google.com changed reviewers: + aurimas@chromium.org, cjhopman@chromium.org, feng@chromium.org, hinoka@chromium.org, nyquist@chromium.org
hinoka@ indicated that rsync may take a long time. I tested locally and it did not take long (i.e. ~3 seconds) when the package was already downloaded. That was not the case on the bots. E.g. http://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28db... Note: Hook '/usr/bin/python src/build/download_sdk_extras.py google_google_play_services_21.0.0' took 31.19 secs This changes it so that it always downloads and unzips a zip file for the package. This will also make it be easy to remove other packages from the repo (by adding packages to zip files).
> This changes it so that it always downloads and unzips a zip file for the > package. This will also make it be easy to remove other packages from the repo > (by adding packages to zip files). Both local and testing on bots show this will take ~3 seconds always.
https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:36: # If successfully read bucket, then this must be a bot with permissions Not always. Sometimes its possible to download a file (object permissions) but not list the bucket (bucket permissions, which are separate). The best way to ensure there is correct permissions is just to try to download the desired file and check the error code (or run a "stat" on the file) https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:43: # Package zip is named <folder>_<package_name>_<version>.zip. Unzipping We shouldn't re-download the zip if we have all the right files already.
https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:36: # If successfully read bucket, then this must be a bot with permissions On 2014/12/31 00:32:55, hinoka wrote: > Not always. Sometimes its possible to download a file (object permissions) but > not list the bucket (bucket permissions, which are separate). The best way to > ensure there is correct permissions is just to try to download the desired file > and check the error code (or run a "stat" on the file) Acknowledged. I'll fix this. https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:43: # Package zip is named <folder>_<package_name>_<version>.zip. Unzipping On 2014/12/31 00:32:55, hinoka wrote: > We shouldn't re-download the zip if we have all the right files already. How do we check if the files contain the same contents? It is not enough that we have the same files, as we may have different versions. Thus, we will need to actually check the contents. That is what took rsync so long.
https://codereview.chromium.org/799673003/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/799673003/diff/20001/DEPS#newcode561 DEPS:561: # Add package and zip file to .gitignore in third_party/android_tools. What does this comment mean? Does this action add those things to .gitignore in third_party/android_tools? https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:9: format: <dir_in_sdk_extras>_<package_name>_<version>. There will be a The expected argument is slightly different now https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:48: _, rc = GetCmdOutputAndStatus(cmd_lst) How about: subprocess.check_output(cmd_lst) or, if you aren't intentionally suppressing the output: subprocess.check_call(cmd_lst) same below.
https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:43: # Package zip is named <folder>_<package_name>_<version>.zip. Unzipping On 2014/12/31 00:37:59, navabi wrote: > On 2014/12/31 00:32:55, hinoka wrote: > > We shouldn't re-download the zip if we have all the right files already. > > How do we check if the files contain the same contents? It is not enough that we > have the same files, as we may have different versions. Thus, we will need to > actually check the contents. That is what took rsync so long. Can we assume that the version number actually works? i.e. if it's the same version number, don't download it. https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:46: local_path = '%s/' % SDK_EXTRAS_PATH Could there be stuff in local_path from previous runs? Should local_path be cleared out first?
https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:43: # Package zip is named <folder>_<package_name>_<version>.zip. Unzipping On 2014/12/31 00:37:59, navabi wrote: > On 2014/12/31 00:32:55, hinoka wrote: > > We shouldn't re-download the zip if we have all the right files already. > > How do we check if the files contain the same contents? It is not enough that we > have the same files, as we may have different versions. Thus, we will need to > actually check the contents. That is what took rsync so long. How about using 'gsutil hash' or 'gsutil stat' like we discussed before, but do that just on the zip-file?
An alternate solution is to use download_from_google_storage.py, where you'd check in a (or a series of) .sha1 files containing sha1 hashes (uploaded using upload_to_google_storage), and the script takes care of ensuring the files are correct (by hashing them) and downloading if they're incorrect. But it seems like a quick optimization for this script would be to say that each version has a unique filename, and rely on that.
> But it seems like a quick optimization for this script would be to say that each > version has a unique filename, and rely on that. I don't think that would work. I'm not sure what you mean each version have a unique filename, but what we download and extract is a directory with multiple files. Each one can't have a unique version name. And just checking in a file with a name, doesn't ensure that someone doesn't modify one of the files (or a file gets corrupted). I think nyquist's suggestion is the best strategy. I'll do it that way.
On 2015/01/02 21:39:27, navabi wrote: > > But it seems like a quick optimization for this script would be to say that > each > > version has a unique filename, and rely on that. > > I don't think that would work. I'm not sure what you mean each version have a > unique filename, but what we download and extract is a directory with multiple > files. Each one can't have a unique version name. And just checking in a file > with a name, doesn't ensure that someone doesn't modify one of the files (or a > file gets corrupted). I think nyquist's suggestion is the best strategy. I'll do > it that way. I think the suggestion was that, if we have a file already (like google_google_play_services_21.0.0.zip), we would assume that that file never changes in the bucket. If it does change, it would get a new version number (i.e. google_google_play_services_21.0.1.zip). It's that the zip files have unique version names. I think that we can assume that there aren't upstream changes without a change to the version number, but the `gsutil hash` approach is also safe against us accidentally having a bad (corrupt, misnamed, etc) zip file in the bucket.
PTAL. The newest patch will download the zip file only if it doesn't already exist. It will always clean the directory and extract the zip file. If the zip file is not local, it takes ~5 seconds. That should only happen the first time on the bots. If the zip is there, it takes ~.2 seconds to clean and extract. First I need this to land: https://gerrit.chromium.org/gerrit/#/c/73359/ https://codereview.chromium.org/799673003/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/799673003/diff/20001/DEPS#newcode561 DEPS:561: # Add package and zip file to .gitignore in third_party/android_tools. On 2014/12/31 00:39:28, cjhopman wrote: > What does this comment mean? Does this action add those things to .gitignore in > third_party/android_tools? The comment is instructing the person that adds arguments to the call to download_sdk_extras.py to update the .gitignore file according. https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:9: format: <dir_in_sdk_extras>_<package_name>_<version>. There will be a On 2014/12/31 00:39:29, cjhopman wrote: > The expected argument is slightly different now Done.
https://codereview.chromium.org/799673003/diff/20001/DEPS File DEPS (right): https://codereview.chromium.org/799673003/diff/20001/DEPS#newcode561 DEPS:561: # Add package and zip file to .gitignore in third_party/android_tools. On 2015/01/06 00:32:09, navabi wrote: > On 2014/12/31 00:39:28, cjhopman wrote: > > What does this comment mean? Does this action add those things to .gitignore > in > > third_party/android_tools? > > The comment is instructing the person that adds arguments to the call to > download_sdk_extras.py to update the .gitignore file according. Oh, that makes sense. Could you maybe add: "When changing this, make sure to..." or some such. https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:32: cmd_lst = ['rm', '-rf', local_dir] instead of subbing out to 'rm -rf', you can do: if os.path.exists(local_dir): shutil.rmtree(local_dir) but either is fine with me. https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:33: rc = subprocess.check_call(cmd_lst) check_call throws an exception if the returncode is non-zero, so you don't need to handle that case for all of these https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:37: cmd_lst = ['unzip', '-u', local_zip, '-d', SDK_EXTRAS_PATH] You could use python's zipfile here something like: with zipfile.ZipFile(local_zip) as z: z.extractall(path=SDK_EXTRAS_PATH) https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:48: if os.path.exists(local_zip): this if/else can just be: if not os.path.exists(local_zip): package_zip = ... subprocess.check_call(...stat...) subprocess.check_call(...cp...) return clean_and_extract(arg)
https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:32: cmd_lst = ['rm', '-rf', local_dir] On 2015/01/06 02:04:04, cjhopman wrote: > instead of subbing out to 'rm -rf', you can do: > > if os.path.exists(local_dir): > shutil.rmtree(local_dir) > > but either is fine with me. Done. https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:33: rc = subprocess.check_call(cmd_lst) On 2015/01/06 02:04:04, cjhopman wrote: > check_call throws an exception if the returncode is non-zero, so you don't need > to handle that case for all of these Acknowledged. https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:37: cmd_lst = ['unzip', '-u', local_zip, '-d', SDK_EXTRAS_PATH] On 2015/01/06 02:04:04, cjhopman wrote: > You could use python's zipfile here > > something like: > > with zipfile.ZipFile(local_zip) as z: > z.extractall(path=SDK_EXTRAS_PATH) Done. https://codereview.chromium.org/799673003/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:48: if os.path.exists(local_zip): On 2015/01/06 02:04:04, cjhopman wrote: > this if/else can just be: > > if not os.path.exists(local_zip): > package_zip = ... > subprocess.check_call(...stat...) > subprocess.check_call(...cp...) > return clean_and_extract(arg) Done. (and there is really no point in doing the stat. It's to check if cp is going to work, but we may as well just try to cp).
Thanks cjhopman for the reviews. PTAL.
lgtm https://codereview.chromium.org/799673003/diff/80001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/80001/build/download_sdk_extra... build/download_sdk_extras.py:51: rc = subprocess.check_call([GSUTIL_PATH, '--force-version', '4.7', 'cp', don't need to manually check returncode
https://codereview.chromium.org/799673003/diff/80001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/80001/build/download_sdk_extra... build/download_sdk_extras.py:51: rc = subprocess.check_call([GSUTIL_PATH, '--force-version', '4.7', 'cp', Do you need an except CalledProcessError here, to ensure the other things in the loop continue, and that you clean and extract the right thing? (https://docs.python.org/2/library/subprocess.html#subprocess.check_call)
https://codereview.chromium.org/799673003/diff/80001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/80001/build/download_sdk_extra... build/download_sdk_extras.py:51: rc = subprocess.check_call([GSUTIL_PATH, '--force-version', '4.7', 'cp', On 2015/01/06 19:14:55, nyquist wrote: > Do you need an except CalledProcessError here, to ensure the other things in > the loop continue, and that you clean and extract the right thing? > (https://docs.python.org/2/library/subprocess.html#subprocess.check_call) I'd just let it go uncaught personally.
https://codereview.chromium.org/799673003/diff/80001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/80001/build/download_sdk_extra... build/download_sdk_extras.py:51: rc = subprocess.check_call([GSUTIL_PATH, '--force-version', '4.7', 'cp', On 2015/01/06 19:16:42, cjhopman wrote: > On 2015/01/06 19:14:55, nyquist wrote: > > Do you need an except CalledProcessError here, to ensure the other things in > > the loop continue, and that you clean and extract the right thing? > > (https://docs.python.org/2/library/subprocess.html#subprocess.check_call) > > I'd just let it go uncaught personally. If the zip file does not exist and we are unable to copy it from google storage, I'm not sure what we would extract, and since we will need the packages to build on Android, I'll let it go uncaught. When we add new packages and zips we may consider changing this.
lgtm
The CQ bit was checked by navabi@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/799673003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/24606e2106ec9029e042353feaa84555708aefd0 Cr-Commit-Position: refs/heads/master@{#310135}
Message was sent while issue was closed.
kjellander@chromium.org changed reviewers: + kjellander@chromium.org
Message was sent while issue was closed.
Adding a comment about the bad assumption of having the depot tools four directories above src would be a proof of that the user is a buildbot. Also provided a suggestion on how to fix it. https://codereview.chromium.org/799673003/diff/120001/build/download_sdk_extr... File build/download_sdk_extras.py (right): https://codereview.chromium.org/799673003/diff/120001/build/download_sdk_extr... build/download_sdk_extras.py:43: # This is not a buildbot checkout. This is a really bad assumption to make. There are probably many developers that have their depot_tools dir in something like ~/depot_tools (I do) and then their checkout in ~/development/whatever/chromium/specialproject/src That would make them a buildbot and this script fails. This happened to a dev in my team recently, making me track this down. I think a much better check is to use: if os.environ.get('CHROME_HEADLESS') == '1' That's what we use for detecting running as a buildbot in WebRTC. I got that tip from iannucci@. Please fix this!
Message was sent while issue was closed.
raphael.kubo.da.costa@intel.com changed reviewers: + raphael.kubo.da.costa@intel.com
Message was sent while issue was closed.
For the record, this is also affecting our downstream bots in Crosswalk, as the directory structure is the one described by kjellander. They are bots, but they are not Google bots.
Message was sent while issue was closed.
On 2015/01/29 00:21:06, Raphael Kubo da Costa (rakuco) wrote: > For the record, this is also affecting our downstream bots in Crosswalk, as the > directory structure is the one described by kjellander. They are bots, but they > are not Google bots. I just filed https://code.google.com/p/chromium/issues/detail?id=454220 to track this. It really needs to be fixed. |
