Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
492853002:
bisect-builds.py: Fix official Google Chrome build bisection. (Closed)
|
Created:
6 years, 4 months ago by pshenoy Modified:
6 years, 4 months ago CC:
chromium-reviews, anantha Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionbisect-builds.py: Fix official Google Chrome build bisection.
Bisecting Official Google Chrome builds was broken due to change of
base URL from http://master.chrome.corp.google.com/official_builds/
to https://console.developers.google.com/storage/chrome-unsigned/desktop-W15K3Y/.
BUG=384336
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291099
Patch Set 1 #
Total comments: 11
Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 18
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Messages
Total messages: 13 (0 generated)
I'm fine with using gsutil via cli, so it's up to you if you want to luck at importing. https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:39: # GSUtil download URL. gsutil comes with depot_tools. Do you really want to redownload it? Last I checked, you can also import its libraries directly into python instead of shelling out to use it. https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:415: def RunGsutilCommand(args): Add a line between definitions. https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:432: def Gsutil_List(bucket): No underscores in names. https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:453: connection = urllib.urlopen(path) Google Storage latency is much higher than the previous corp url. You should include the HEAD test from the patch I posted on the bug to speed this up by 2-4x.
https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:39: # GSUtil download URL. On 2014/08/20 19:54:43, DaleCurtis wrote: > gsutil comes with depot_tools. Do you really want to redownload it? Last I > checked, you can also import its libraries directly into python instead of > shelling out to use it. I don't think we should download it as part of this script. You should either require it to be installed separately or find it through depot_tools.
https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:39: # GSUtil download URL. On 2014/08/20 19:54:43, DaleCurtis wrote: > gsutil comes with depot_tools. Do you really want to redownload it? Last I > checked, you can also import its libraries directly into python instead of > shelling out to use it. Testing team run this as standalone script and depot_tools is not installed on all machines which testing team uses. Also downloading gsutil happens only once if the script is run from the same location. https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:415: def RunGsutilCommand(args): On 2014/08/20 19:54:43, DaleCurtis wrote: > Add a line between definitions. Done. https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:432: def Gsutil_List(bucket): On 2014/08/20 19:54:43, DaleCurtis wrote: > No underscores in names. Done. https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:453: connection = urllib.urlopen(path) On 2014/08/20 19:54:43, DaleCurtis wrote: > Google Storage latency is much higher than the previous corp url. You should > include the HEAD test from the patch I posted on the bug to speed this up by > 2-4x. Done.
https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:39: # GSUtil download URL. On 2014/08/20 20:33:55, pshenoy wrote: > On 2014/08/20 19:54:43, DaleCurtis wrote: > > gsutil comes with depot_tools. Do you really want to redownload it? Last I > > checked, you can also import its libraries directly into python instead of > > shelling out to use it. > > Testing team run this as standalone script and depot_tools is not installed on > all machines which testing team uses. Also downloading gsutil happens only once > if the script is run from the same location. How about checking for depot_tools / gsutil in the path and if missing point them to downloading it manually? I also wonder if google storage apis expose a json list that would avoid needing to use gsutil at all for this: https://developers.google.com/storage/docs/json_api/ The problem might be that this is unauthenticated usage. https://codereview.chromium.org/492853002/diff/40001/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/40001/tools/bisect-builds.py#n... tools/bisect-builds.py:442: revision_re = re.compile(r'(\d\d\.\d\.\d{4}\.\d+)') No point in using re.compile inside the function. You should compile this and pass the compiled re into filter (i.e. drop this function) if you want this optimization. Alternatively just drop it if you want.
https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/1/tools/bisect-builds.py#newco... tools/bisect-builds.py:39: # GSUtil download URL. On 2014/08/20 20:46:43, DaleCurtis wrote: > On 2014/08/20 20:33:55, pshenoy wrote: > > On 2014/08/20 19:54:43, DaleCurtis wrote: > > > gsutil comes with depot_tools. Do you really want to redownload it? Last I > > > checked, you can also import its libraries directly into python instead of > > > shelling out to use it. > > > > Testing team run this as standalone script and depot_tools is not installed on > > all machines which testing team uses. Also downloading gsutil happens only > once > > if the script is run from the same location. > > How about checking for depot_tools / gsutil in the path and if missing point > them to downloading it manually? > > I also wonder if google storage apis expose a json list that would avoid needing > to use gsutil at all for this: > > https://developers.google.com/storage/docs/json_api/ > > The problem might be that this is unauthenticated usage. Done. https://codereview.chromium.org/492853002/diff/40001/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/40001/tools/bisect-builds.py#n... tools/bisect-builds.py:442: revision_re = re.compile(r'(\d\d\.\d\.\d{4}\.\d+)') On 2014/08/20 20:46:43, DaleCurtis wrote: > No point in using re.compile inside the function. You should compile this and > pass the compiled re into filter (i.e. drop this function) if you want this > optimization. Alternatively just drop it if you want. Done.
https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:39: # GSUtil download URL. Remove? https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:404: def CheckDeoptToolsInPath(): Name is wrong. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:405: if os.environ['PATH'].find('depot_tools') != -1: Might as well just split and look for it instead of double-checking. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:406: delimiter = ';' if sys.platform.startswith('win') else ':' Hmm, does this syntax actually work? https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:426: if ('status=403' in stderr or 'status 403' in stderr or just re.findall(r'status[ |=]40[1|3]', stderr) ? https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:446: build_numbers = GsutilList('chrome-unsigned/desktop-W15K3Y') Constant. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:452: connection = httplib.HTTPConnection('commondatastorage.googleapis.com') Put this as a constant with the official_url ? https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:458: path = ('/chrome-unsigned/desktop-W15K3Y/' + str(build_number) + '/' + Ditto. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:467: return sorted(list(set(final_list))) I think I had a bug in my patch previously, this shouldn't be necessary. LooseVersion should return 1:1 with the directory listing, so there shouldn't be duplicates and they should already be sorted.
https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:39: # GSUtil download URL. On 2014/08/20 22:56:19, DaleCurtis wrote: > Remove? Done. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:404: def CheckDeoptToolsInPath(): On 2014/08/20 22:56:18, DaleCurtis wrote: > Name is wrong. Done. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:405: if os.environ['PATH'].find('depot_tools') != -1: On 2014/08/20 22:56:19, DaleCurtis wrote: > Might as well just split and look for it instead of double-checking. Done. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:406: delimiter = ';' if sys.platform.startswith('win') else ':' On 2014/08/20 22:56:18, DaleCurtis wrote: > Hmm, does this syntax actually work? Yes. It does :-) https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:426: if ('status=403' in stderr or 'status 403' in stderr or On 2014/08/20 22:56:18, DaleCurtis wrote: > just re.findall(r'status[ |=]40[1|3]', stderr) ? Done. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:446: build_numbers = GsutilList('chrome-unsigned/desktop-W15K3Y') On 2014/08/20 22:56:19, DaleCurtis wrote: > Constant. Done. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:452: connection = httplib.HTTPConnection('commondatastorage.googleapis.com') On 2014/08/20 22:56:19, DaleCurtis wrote: > Put this as a constant with the official_url ? Done. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:458: path = ('/chrome-unsigned/desktop-W15K3Y/' + str(build_number) + '/' + On 2014/08/20 22:56:19, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/492853002/diff/50001/tools/bisect-builds.py#n... tools/bisect-builds.py:467: return sorted(list(set(final_list))) On 2014/08/20 22:56:19, DaleCurtis wrote: > I think I had a bug in my patch previously, this shouldn't be necessary. > LooseVersion should return 1:1 with the directory listing, so there shouldn't be > duplicates and they should already be sorted. Done.
lgtm % nit https://codereview.chromium.org/492853002/diff/70001/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/70001/tools/bisect-builds.py#n... tools/bisect-builds.py:24: OFFICIAL_BASE_URL = ('http://commondatastorage.googleapis.com/' Construct the official base_url using the bucket_name and google apis url constants?
https://codereview.chromium.org/492853002/diff/70001/tools/bisect-builds.py File tools/bisect-builds.py (right): https://codereview.chromium.org/492853002/diff/70001/tools/bisect-builds.py#n... tools/bisect-builds.py:24: OFFICIAL_BASE_URL = ('http://commondatastorage.googleapis.com/' On 2014/08/21 00:18:55, DaleCurtis wrote: > Construct the official base_url using the bucket_name and google apis url > constants? Done.
The CQ bit was checked by pshenoy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pshenoy@chromium.org/492853002/90001
Message was sent while issue was closed.
Committed patchset #6 (90001) as 291099 |
