Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
709663002:
Add gclient hook to download SDK extras on bots. (Closed)
|
Created:
6 years, 1 month ago by navabi Modified:
5 years, 11 months ago Reviewers:
hinoka, avayvod1, Ryan Tseng, yfriedman, nyquist, aruslan, cjhopman, aurimas (slooooooooow) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd gclient hook to download SDK extras on bots.
BUG=350151
TBR=cjhopman@chromium.org
Committed: https://crrev.com/6f0bb7619afcdd28daba7e763dacd7035504be4c
Cr-Commit-Position: refs/heads/master@{#309763}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove GSUTIL_PATH for local testing. #
Total comments: 4
Patch Set 3 : Use gclient rsync and introduce versioning. #
Total comments: 3
Patch Set 4 : Fix assignment to rc and check for android_tools/sdk/extras dir. #
Total comments: 14
Patch Set 5 : Add '-d' to rysnc and corresponding changes. #Patch Set 6 : Use depot_tools/gsutil.py --force-version 4.6 for rsync. #Patch Set 7 : Remove -m option. At the moment gsutil.py does not properly support it. #Patch Set 8 : Add -m option back. hinoka fixed it quick. #Patch Set 9 : Create directory to rsync. #Patch Set 10 : Only make directory if it does not exist. #
Total comments: 6
Patch Set 11 : Nits and comments explaining. #Patch Set 12 : is_buildbot_checkout -> is_android_buildbot_checkout #
Total comments: 5
Messages
Total messages: 43 (7 generated)
https://codereview.chromium.org/709663002/diff/1/build/download_sdk_extras.py File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/1/build/download_sdk_extras.py... build/download_sdk_extras.py:19: GSUTIL_PATH = '/usr/local/google/code/gsutil/gsutil' oops. i put this in to test locally.
navabi@google.com changed reviewers: + aruslan@chromium.org, avayvod@google.com, hinoka@chromium.org, nyquist@chromium.org, yfriedman@google.com
https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:35: cmd_lst = [GSUTIL_PATH, 'cp', '-r', SDK_EXTRAS_BUCKET, SDK_EXTRAS_PATH] The idea is to put directories in the google bucket like this: google/google_play_services/* And whatever else we need? I see that we currently have gcm in there (or at least my checkout does)? nyquist@ can you remind me how that package got installed? We can now do everything this way for the bots.
PTAL. Discussed comment from latest patch offline with nquist.
the premise of this CL looks reasonable to me. https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:15: GSUTIL_PATH = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, is gsutils checkout out into //third_party/gsutil/gsutil? If so, this seemed like a lot of os.pardir entries from //build.
https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:15: GSUTIL_PATH = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, On 2014/11/11 19:50:31, nyquist wrote: > is gsutils checkout out into //third_party/gsutil/gsutil? If so, this seemed > like a lot of os.pardir entries from //build. No. On the bots gsutils is in /b/build/third_party/gsutil/gsutil, an the clankium checkout is in /b/build/slave/<botname>/build/src, thus this script is in /b/build/slave/<botname>/build/src/build/ so the directory /b/build/slave/<botname>/build/src/build/../../../../../third_party/gsutil/gsutil is correct.
lgtm https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... build/download_sdk_extras.py:15: GSUTIL_PATH = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, On 2014/11/11 19:50:31, nyquist wrote: > is gsutils checkout out into //third_party/gsutil/gsutil? If so, this seemed > like a lot of os.pardir entries from //build. Is there no other way for referring to buildbot paths than os.pardir a bunch of times?
On 2014/11/11 19:56:53, nyquist wrote: > lgtm > > https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... > File build/download_sdk_extras.py (right): > > https://codereview.chromium.org/709663002/diff/20001/build/download_sdk_extra... > build/download_sdk_extras.py:15: GSUTIL_PATH = > os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, > On 2014/11/11 19:50:31, nyquist wrote: > > is gsutils checkout out into //third_party/gsutil/gsutil? If so, this seemed > > like a lot of os.pardir entries from //build. > > Is there no other way for referring to buildbot paths than os.pardir a bunch of > times? No, I don't think so. If you look at clank's build/android/pylib/constants.py it also finds things like the source directory by using os.pardir a bunch of times relative to where the constants.py script is. The only thing I can think of is to require a new environment variable to be set on all the bots (e.g. BUILD_BOT_ROOT) and then checking if that exists.
PTAL nyquist: Thanks for the pointer to rsync. It requires gsutil 4.0, which we will need to upgrade to use this. Please take a look at how the new patch does versioning of the sdk/extras packages. hinoka: Do you seen any potential problems with upgrading the gsutil in build/third_party/gsutil if we take this route? https://codereview.chromium.org/709663002/diff/40001/DEPS File DEPS (right): https://codereview.chromium.org/709663002/diff/40001/DEPS#newcode559 DEPS:559: 'google_google_play_services_21.0.0'], Added this as the versioning scheme. In the google_google_play_services_21.0.0 bucket there is the google/google_play_services/* directory for revision 21.0.0. This way new revisions can be added to the google storage bucket (e.g. gs://chrome-sdk-extras/google_google_play_services_22.0.0), and then the "roll" would happen by changing this command line argument in this DEPS file. This also allows adding other packages as the command line arguments for download_sdk_extras.py takes a list of buckets. https://codereview.chromium.org/709663002/diff/40001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/40001/build/download_sdk_extra... build/download_sdk_extras.py:36: package_bucket = '%s/%s' % (SDK_EXTRAS_BUCKET, arg) list of packages to download into third_party/android_tools/sdk/extras/ https://codereview.chromium.org/709663002/diff/40001/build/download_sdk_extra... build/download_sdk_extras.py:37: cmd_lst = [GSUTIL_PATH, '-m', 'rsync', '-r', package_bucket, This will require us to upgrade the gsutil used on the bots. Currently we use gsutil version 3._ and it does not have the 'rsync' command. We would need to upgrade to version gsutil 4.0 to do rsync. We use rsync to save time when the bot already has the correct version of the package(s). My local tests show that when the package is not in the sdk/extras/ directory it takes ~12 seconds to download. If it there it will take ~5.5 seconds. This may still be too long, as it will mean an extra ~5.5 seconds for every 'gclient sync' on the bots. Note that developers will not suffer this time. When not on a bot, download_sdk_extras.py will not take any time.
hinoka@google.com changed reviewers: + hinoka@google.com
re updating gsutil: we've absolutely had trouble upgrading gsutil in the past. The issue is we really don't have a good idea all of the project that uses gsutil, and how they depend on it. Updating gsutil there will inevitably cause some project somewhere to break, Instead, I will land https://codereview.chromium.org/742173002/ soon, which lets you call depot_tools/gsutil.py --force_version 4.0 <commands>, which will download the exact version of gsutil you tell it to, and run it with the given commands. I suggest using that instead.
navabi@google.com changed reviewers: + aurimas@chromium.org
Thanks Ryan for the info about using gsutil 4.0. What you suggest should work. Adding aurimas@ as nyquist@ mentioned he may know about what parts of the extra packages we need. To save time I removed: /google/google_play_services/docs/ /google/google_play_services/samples/ This saves a significant ~2 secs (from 5.5 secs to 3.5 secs) to run the hook when the bot already has the right checkout. I assume the bot does not need the docs or samples directories, right?
On 2014/11/21 23:00:13, navabi wrote: > Thanks Ryan for the info about using gsutil 4.0. What you suggest should work. > > Adding aurimas@ as nyquist@ mentioned he may know about what parts of the extra > packages we need. To save time I removed: > /google/google_play_services/docs/ > /google/google_play_services/samples/ > > This saves a significant ~2 secs (from 5.5 secs to 3.5 secs) to run the hook > when the bot already has the right checkout. I assume the bot does not need the > docs or samples directories, right? Sorry for the delay. We definitely don't need docs and samples. Where can I see what else we would be pulling in?
On 2014/11/25 17:46:40, aurimas wrote: > On 2014/11/21 23:00:13, navabi wrote: > > Thanks Ryan for the info about using gsutil 4.0. What you suggest should work. > > > > Adding aurimas@ as nyquist@ mentioned he may know about what parts of the > extra > > packages we need. To save time I removed: > > /google/google_play_services/docs/ > > /google/google_play_services/samples/ > > > > This saves a significant ~2 secs (from 5.5 secs to 3.5 secs) to run the hook > > when the bot already has the right checkout. I assume the bot does not need > the > > docs or samples directories, right? > > Sorry for the delay. We definitely don't need docs and samples. Where can I see > what else we would be pulling in? No problem. Here is what we pull in: navabi@yeezus:/usr/local/google/code/clankium-master/src$ ll third_party/android_tools/sdk/extras/google/google_play_services/ total 32 drwxr-x--- 3 navabi eng 4096 Nov 24 22:00 ./ drwxr-x--- 4 navabi eng 4096 Nov 24 22:00 ../ drwxr-x--- 3 navabi eng 4096 Nov 24 22:00 libproject/ -rw-r----- 1 navabi eng 16747 Nov 24 22:00 source.properties To see what is in libproject, just download the google play services extra package using src/third_party/android_tools/sdk/tools/android.
aurimas: would we need to tweak the google play services jar a bit? or should we maybe just do that in our own checkout downstream?
https://codereview.chromium.org/709663002/diff/60001/DEPS File DEPS (right): https://codereview.chromium.org/709663002/diff/60001/DEPS#newcode558 DEPS:558: 'action': ['python', 'src/build/download_sdk_extras.py', Would this overwrite what's already in the checkout in sdk/extras on the build after this lands? https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:17: SDK_EXTRAS_BUCKET = 'gs://chrome-sdk-extras' Does this already have the exact same content (except from samples and docs) as https://chromium.googlesource.com/android_tools.git/+/master/sdk/extras/ ? https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:18: SDK_EXTRAS_PATH = os.path.join(constants.ANDROID_SDK_ROOT, 'extras') It feels like there should be a .gitignore in https://chromium.googlesource.com/android_tools.git that ignores sdk/extras. Is that in place? https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:26: def is_buildbot_checkout(): this is a little bit odd to me, but I don't have a good suggestion. Does all buildbots set BUILDBOT_BUILDERNAME by the way? Maybe checking os.environ.get('BUILDBOT_BUILDERNAME', '') or something? It might be fine as is though. https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:38: cmd_lst = [GSUTIL_PATH, '-m', 'rsync', '-r', package_bucket, would you want '-d' here to delete the files that do not exist anymore on the server?
thanks for the reviews Tommy and Aurimas. https://codereview.chromium.org/709663002/diff/60001/DEPS File DEPS (right): https://codereview.chromium.org/709663002/diff/60001/DEPS#newcode558 DEPS:558: 'action': ['python', 'src/build/download_sdk_extras.py', On 2014/11/25 20:08:26, nyquist wrote: > Would this overwrite what's already in the checkout in sdk/extras on the build > after this lands? Yes it will. Because it uses rsync, so it will sync the local directory to what is in the google storage bucket. https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:17: SDK_EXTRAS_BUCKET = 'gs://chrome-sdk-extras' On 2014/11/25 20:08:26, nyquist wrote: > Does this already have the exact same content (except from samples and docs) as > https://chromium.googlesource.com/android_tools.git/+/master/sdk/extras/ ? Yes. https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:18: SDK_EXTRAS_PATH = os.path.join(constants.ANDROID_SDK_ROOT, 'extras') On 2014/11/25 20:08:26, nyquist wrote: > It feels like there should be a .gitignore in > https://chromium.googlesource.com/android_tools.git that ignores sdk/extras. Is > that in place? Yes. It is in place, in that the head of the android_tools dir has sdk/extras/* in the .gitignore. I still need to roll DEPS to check out the newly pushed revision before this lands. https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:26: def is_buildbot_checkout(): On 2014/11/25 20:08:26, nyquist wrote: > this is a little bit odd to me, but I don't have a good suggestion. > Does all buildbots set BUILDBOT_BUILDERNAME by the way? > Maybe checking os.environ.get('BUILDBOT_BUILDERNAME', '') or something? > > It might be fine as is though. I'll consider it. But even if we checked some other way, we still want to check if these paths exist. And then at that point, checking for anything else would not be necessary, except maybe it would be more clear we are checking if we are on a bot ? Even then, a comment would do. https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:38: cmd_lst = [GSUTIL_PATH, '-m', 'rsync', '-r', package_bucket, On 2014/11/25 20:08:26, nyquist wrote: > would you want '-d' here to delete the files that do not exist anymore on the > server? No. That would remove other packages the way we currently have it. i.e. if locally you have: sdk/extras/google/package1 sdk/extras/google/package2 And you do an rsync -d of a bucket that looks like this: sdk/extras/google/package1 right now we just do an rsync -d into sdk/packages which would mean google/package2 would get deleted. To use the -d, we would need to either split the name of package name "google_google_play_services_21.0.0" to come up with the directory to download to (i.e. google/google_play_services/), or we would need to do a sequence of gsutil ls calls to determine that directory, which would take up more time because gsutil calls are expensive. I did name the package such that the name of the directory is clear just to avoid doing a gsutil ls to find the directory name, but I'm not sure we need to delete things that don't exist on the bot. I could be convinced otherwise though.
https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/60001/build/download_sdk_extra... build/download_sdk_extras.py:18: SDK_EXTRAS_PATH = os.path.join(constants.ANDROID_SDK_ROOT, 'extras') On 2014/11/25 20:40:49, navabi wrote: > On 2014/11/25 20:08:26, nyquist wrote: > > It feels like there should be a .gitignore in > > https://chromium.googlesource.com/android_tools.git that ignores sdk/extras. > Is > > that in place? > > Yes. It is in place, in that the head of the android_tools dir has sdk/extras/* > in the .gitignore. I still need to roll DEPS to check out the newly pushed > revision before this lands. So you also cleaned out that folder? Or are you saying there's a .gitignore in there, but the repo also contains files in the location the .gitignore refers to?
> So you also cleaned out that folder? Or are you saying there's a > .gitignore in there, but the repo also contains files in the > location the .gitignore refers to? I have to fix the .gitignore. It includes everything in sdk/extras/*, so it contains files in the location that .gitignore refers to. This is not intended. I will just list the specific directories for each extras package in the .gitignore.
https://codereview.chromium.org/709663002/diff/60001/DEPS File DEPS (right): https://codereview.chromium.org/709663002/diff/60001/DEPS#newcode559 DEPS:559: 'google_google_play_services_21.0.0'], does this directory structure have to be flattened? could we instead checkout everything here under each branch somehow? so you'd have one directory for 2171, and one for master, etc.? and then the folder structure within there would be correct? if you do that you don't have to be worried about keeping everything in the external repo.
Looking for some clarification. https://codereview.chromium.org/709663002/diff/60001/DEPS File DEPS (right): https://codereview.chromium.org/709663002/diff/60001/DEPS#newcode559 DEPS:559: 'google_google_play_services_21.0.0'], On 2014/11/26 04:34:51, nyquist wrote: > does this directory structure have to be flattened? > could we instead checkout everything here under each branch somehow? > > so you'd have one directory for 2171, and one for master, etc.? > and then the folder structure within there would be correct? > > if you do that you don't have to be worried about keeping everything in the > external repo. Sorry Tommy, but I don't know what you mean. The directory structure does not and is not flattened. The contents of google_google_play_services_21.0.0 is google/google_play_services/<otherstuff>. Maybe I am misunderstanding you, but flattened to me means all the contents is one directory. I'm not sure what you mean by one directory for 2171 and one for master. And when you say "external repo", are you referring to the google storage bucket as a repo?
https://codereview.chromium.org/709663002/diff/60001/DEPS File DEPS (right): https://codereview.chromium.org/709663002/diff/60001/DEPS#newcode559 DEPS:559: 'google_google_play_services_21.0.0'], On 2014/11/26 18:52:35, navabi wrote: > On 2014/11/26 04:34:51, nyquist wrote: > > does this directory structure have to be flattened? > > could we instead checkout everything here under each branch somehow? > > > > so you'd have one directory for 2171, and one for master, etc.? > > and then the folder structure within there would be correct? > > > > if you do that you don't have to be worried about keeping everything in the > > external repo. > > Sorry Tommy, but I don't know what you mean. > > The directory structure does not and is not flattened. The contents of > google_google_play_services_21.0.0 is google/google_play_services/<otherstuff>. > Maybe I am misunderstanding you, but flattened to me means all the contents is > one directory. > > I'm not sure what you mean by one directory for 2171 and one for master. > > And when you say "external repo", are you referring to the google storage bucket > as a repo? Sorry. Okay; my thought was keeping a folder in the bucket named 'master' and '2171'. In that folder you would have the full extras directory, including google/google_play_services/<otherstuff>. However, thinking about it, to ensure we have consistent builds, what you are proposing might be better. At that point you also definitely do not want to delete anything using rsync. One thing comes up then though; if you 'roll' google play services to some new version after 21, how would you clean up old files in third_party/android_tools/sdk/extras/google/google_play_services? There might be files that are removed in the newer version, that with this approach might stick around.
> One thing comes up then though; if you 'roll' google play services to some new > version after 21, how would you clean up old files in > third_party/android_tools/sdk/extras/google/google_play_services? > There might be files that are removed in the newer version, that with this > approach might stick around. That's a good point. WDYT about using the name of the bucket (i.e. google_goog_play_service_...) to then do rsync -r -d on google/google_play_services. I think the directory structure of all extras package is the same (e.g. google/<package_name> or android/<package_name>, etc.). That just means we have be consistent in the naming of the buckets.
On 2014/11/26 19:29:20, navabi wrote:
> That's a good point. WDYT about using the name of the bucket (i.e.
> google_goog_play_service_...) to then do rsync -r -d on
> google/google_play_services. I think the directory structure of all extras
> package is the same (e.g. google/<package_name> or android/<package_name>,
> etc.). That just means we have be consistent in the naming of the buckets.
Yeah, that sounds reasonable.
It's a little bit concering to use '_' for both path separator and a word
separator though, since if one package gets another level deep or something, we
might fail. But for now it should do.
Another thing we could do is have a file within the bucket:
gs://.../google_google_play_services_21.0.0/METADATA
Containing something like:
{
'folders': ['google/google_play_services'],
}
That file could contain metadata about what do do, for example a list of all
folders to rsync?
It would require a parser of this checked into chromium naturally.
I don't think it's necessary now though.
> I don't think it's necessary now though. I also don't think it is necessary now. The parsing of the metadata file is not really a problem, as I would make it a json file and use python's json libraries to read the file. The bigger problem is that it would require an extra gsutil command to download the metadata file before parsing it. It is very important that this hook does not add too much time to the bots, especially for the fast path (i.e. the bot already has the latest version of the packages). Even for small files gsutil takes a significant amount of time to download. I think for now it is ok to assume all packages will be installed in the sdk/extras directory as <folder>_<package_name> and name the gsutil bucket appropriately, so we can determine the directory to rsync (with -r -d) based on the name of the package. You will notice that I put a comment in the download_sdk_extras.py script to explain the naming convention.
On 2014/12/01 20:00:01, navabi wrote: > > I don't think it's necessary now though. > > I also don't think it is necessary now. The parsing of the metadata file is not > really a problem, as I would make it a json file and use python's json libraries > to read the file. The bigger problem is that it would require an extra gsutil > command to download the metadata file before parsing it. It is very important > that this hook does not add too much time to the bots, especially for the fast > path (i.e. the bot already has the latest version of the packages). Even for > small files gsutil takes a significant amount of time to download. > > I think for now it is ok to assume all packages will be installed in the > sdk/extras directory as <folder>_<package_name> and name the gsutil bucket > appropriately, so we can determine the directory to rsync (with -r -d) based on > the name of the package. You will notice that I put a comment in the > download_sdk_extras.py script to explain the naming convention. Currently this is blocked on the following two CL's: https://gerrit.chromium.org/gerrit/#/c/72270/1 https://codereview.chromium.org/742173002/
On 2014/12/01 20:03:21, navabi wrote: > On 2014/12/01 20:00:01, navabi wrote: > > > I don't think it's necessary now though. > > > > I also don't think it is necessary now. The parsing of the metadata file is > not > > really a problem, as I would make it a json file and use python's json > libraries > > to read the file. The bigger problem is that it would require an extra gsutil > > command to download the metadata file before parsing it. It is very important > > that this hook does not add too much time to the bots, especially for the fast > > path (i.e. the bot already has the latest version of the packages). Even for > > small files gsutil takes a significant amount of time to download. > > > > I think for now it is ok to assume all packages will be installed in the > > sdk/extras directory as <folder>_<package_name> and name the gsutil bucket > > appropriately, so we can determine the directory to rsync (with -r -d) based > on > > the name of the package. You will notice that I put a comment in the > > download_sdk_extras.py script to explain the naming convention. > > Currently this is blocked on the following two CL's: > https://gerrit.chromium.org/gerrit/#/c/72270/1 > https://codereview.chromium.org/7421730 Looks like both blocking CLs were committed. This is ready to go.
> Looks like both blocking CLs were committed. This is ready to go. Now we are blocked on https://code.google.com/p/chromium/issues/detail?id=443441&thanks=443441&ts=1.... I uploaded a patch that doesn't use '-m' just to see if it works, but I think this will take considerably more time. May be ok though.
https://codereview.chromium.org/709663002/diff/180001/build/download_sdk_extr... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/180001/build/download_sdk_extr... build/download_sdk_extras.py:2: # Copyright (c) 2014 The Chromium Authors. All rights reserved. Nit: Remove (c) https://codereview.chromium.org/709663002/diff/180001/build/download_sdk_extr... build/download_sdk_extras.py:6: """Script to download sdk/extras packages on the bots from google storage.""" Could you add a short comment about the assumptions about the folder name in the bucket here (the underscore-magic)? https://codereview.chromium.org/709663002/diff/180001/build/download_sdk_extr... build/download_sdk_extras.py:42: package_bucket = '%s/%s/%s/%s' % (SDK_EXTRAS_BUCKET, arg, folder, package) Could you add an example comment for package_bucket and package_dir like you did for the |package| variable? possibly with something short for the SDK_EXTRAS_BUCKET
Thanks Tommy. Done. https://codereview.chromium.org/709663002/diff/180001/build/download_sdk_extr... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/180001/build/download_sdk_extr... build/download_sdk_extras.py:2: # Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/12/19 16:19:17, nyquist wrote: > Nit: Remove (c) Done. https://codereview.chromium.org/709663002/diff/180001/build/download_sdk_extr... build/download_sdk_extras.py:6: """Script to download sdk/extras packages on the bots from google storage.""" On 2014/12/19 16:19:17, nyquist wrote: > Could you add a short comment about the assumptions about the folder name in the > bucket here (the underscore-magic)? Done. https://codereview.chromium.org/709663002/diff/180001/build/download_sdk_extr... build/download_sdk_extras.py:42: package_bucket = '%s/%s/%s/%s' % (SDK_EXTRAS_BUCKET, arg, folder, package) On 2014/12/19 16:19:17, nyquist wrote: > Could you add an example comment for package_bucket and package_dir like you did > for the |package| variable? possibly with something short for the > SDK_EXTRAS_BUCKET Done.
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/709663002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
navabi@google.com changed reviewers: + cjhopman@chromium.org
Adding cjhopman@ for OWNERS.
We have talked about this a while, and I'd like to land it today so I can make sure it lands cleanly and does not cause any problems.
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/709663002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/6f0bb7619afcdd28daba7e763dacd7035504be4c Cr-Commit-Position: refs/heads/master@{#309763}
Message was sent while issue was closed.
https://codereview.chromium.org/709663002/diff/220001/build/download_sdk_extr... File build/download_sdk_extras.py (right): https://codereview.chromium.org/709663002/diff/220001/build/download_sdk_extr... build/download_sdk_extras.py:9: format: <dir_in_sdk_extras>_<package_name>_<version>. There will be a this comment implies that I can have underscores in dir_in_sdk_extras, but that's not actually the case. https://codereview.chromium.org/709663002/diff/220001/build/download_sdk_extr... build/download_sdk_extras.py:10: correpsonding bucket in google storage with that name, and it will be downloaded nit: correpsonding/corresponding https://codereview.chromium.org/709663002/diff/220001/build/download_sdk_extr... build/download_sdk_extras.py:44: first_underscore = arg.find('_') should this use a different separator that wouldn't be used in any of the three parts? google:google_play_services:1.0.0 https://codereview.chromium.org/709663002/diff/220001/build/download_sdk_extr... build/download_sdk_extras.py:50: package_bucket = '%s/%s/%s/%s' % (SDK_EXTRAS_BUCKET, arg, folder, package) This with the comment above is a little confusing because the value for package_bucket doesn't match what the comment says "Package bucket" is, instead it it that with the folder appended. maybe do: package_bucket = '%s/%s' % (SDK_EXTRAS_BUCKET, arg) package_dir = '%s/%s' % (folder, package) bucket_dir = '%s/%s' % (package_bucket, package_dir) local_dir = '%s/%s' % (SDK_EXTRAS_PATH, package_dir) https://codereview.chromium.org/709663002/diff/220001/build/download_sdk_extr... build/download_sdk_extras.py:57: stdout, rc = GetCmdOutputAndStatus(cmd_lst) how about just: stdout = subprocess.check_output(cmd_lst) ? and since stdout isn't used (unless you intentionally are suppressing it): subprocess.check_call(cmd_lst) |
