Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12261026:
Added separate checkout and config file generation to bisect script (Closed)
|
Created:
7 years, 10 months ago by shatch Modified:
7 years, 10 months ago CC:
chromium-reviews, cmp Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded separate checkout and config file generation to bisect script.
BUG=
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183827
Patch Set 1 #
Total comments: 16
Patch Set 2 : Changes from review. #Patch Set 3 : Misc fixes and changes. #
Total comments: 10
Patch Set 4 : Changes from review. #Patch Set 5 : Missed a comment. #Patch Set 6 : Changes from review. #
Total comments: 18
Patch Set 7 : Changes from review. #Patch Set 8 : Changes from review. #
Total comments: 3
Patch Set 9 : Changes from review. #Patch Set 10 : Forgot this. #Patch Set 11 : Removed files. #Patch Set 12 : Moved files. #
Total comments: 4
Patch Set 13 : Changes from review. #
Total comments: 6
Patch Set 14 : Changes from review. #
Messages
Total messages: 20 (0 generated)
Wasn't quite sure where to put these files.
I'm not totally sure where these files should live, but I'm leaning towards tools/ so that they are by the bisect script itself. Chase, do you have any ideas on where to check these in? https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... File build/util/run-bisect-perf-regression.cfg (right): https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.cfg:7: This script is used by the run-bisect-perf-regression.py script. Recommend beefing up this description to describe exactly how this file is meant to be used (since it is a little unusual). e.g. mention to edit, run git try <bot>, and never submit changes. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.cfg:22: config =\ I think we should have a PRESUBMIT.py check that verifies that this config contains all empty fields. Otherwise, someone might accidentally check in their change that is meant only for the tryservers. Search for PRESUBMIT.py to see lots of examples in the codebase. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.cfg:23: { Recommend bringing this up to the line above here and in the sample https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.cfg:24: 'command':'', Recommend space after colons in this block and in the sample https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... File build/util/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.py:17: import os alphabetize please https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.py:24: os.chdir('../../') os.chdir(os.path.join('..', '..')) https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.py:26: cmd = ['./tools/bisect-perf-regression.py', Don't you need to pass --output_buildbot_annotations? https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.py:26: cmd = ['./tools/bisect-perf-regression.py', Please use os.path.join for the path to the bisect script.
New snapshot uploaded. Also added a PRESUBMIT.py. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... File build/util/run-bisect-perf-regression.cfg (right): https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.cfg:7: This script is used by the run-bisect-perf-regression.py script. On 2013/02/14 01:03:43, tonyg wrote: > Recommend beefing up this description to describe exactly how this file is meant > to be used (since it is a little unusual). e.g. mention to edit, run git try > <bot>, and never submit changes. Done. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.cfg:22: config =\ On 2013/02/14 01:03:43, tonyg wrote: > I think we should have a PRESUBMIT.py check that verifies that this config > contains all empty fields. Otherwise, someone might accidentally check in their > change that is meant only for the tryservers. > > Search for PRESUBMIT.py to see lots of examples in the codebase. Done. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.cfg:23: { On 2013/02/14 01:03:43, tonyg wrote: > Recommend bringing this up to the line above here and in the sample Done. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.cfg:24: 'command':'', On 2013/02/14 01:03:43, tonyg wrote: > Recommend space after colons in this block and in the sample Done. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... File build/util/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.py:17: import os On 2013/02/14 01:03:43, tonyg wrote: > alphabetize please Done. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.py:24: os.chdir('../../') On 2013/02/14 01:03:43, tonyg wrote: > os.chdir(os.path.join('..', '..')) Done. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.py:26: cmd = ['./tools/bisect-perf-regression.py', On 2013/02/14 01:03:43, tonyg wrote: > Don't you need to pass --output_buildbot_annotations? Done. https://codereview.chromium.org/12261026/diff/1/build/util/run-bisect-perf-re... build/util/run-bisect-perf-regression.py:26: cmd = ['./tools/bisect-perf-regression.py', On 2013/02/14 01:03:43, tonyg wrote: > Please use os.path.join for the path to the bisect script. Done. https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py File build/util/PRESUBMIT.py (right): https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... build/util/PRESUBMIT.py:34: return [output_api.PresubmitError( Should this be just a warning or an error?
I'm pretty sure tools/ would be a better location than build/. If you take a look at the stuff in build, it is all related to actually building chrome. This seems more like an peripheral tool to me. But I'm still curious to hear if Chase has an opinion about where to store this thing. https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py File build/util/PRESUBMIT.py (right): https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... build/util/PRESUBMIT.py:5: """Top-level presubmit script for testing. Comment needs updating https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... build/util/PRESUBMIT.py:21: valid_keys = ['command', 'good_revision', 'bad_revision', 'metric'] I'm not sure I'd lock down the key names. We might add a new argument in the future and it would be totally valid to submit that. https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... build/util/PRESUBMIT.py:34: return [output_api.PresubmitError( On 2013/02/14 23:00:34, shatch wrote: > Should this be just a warning or an error? error sounds good https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... build/util/PRESUBMIT.py:47: return CommonChecks(input_api, output_api) I'm not sure I'd perform this check on upload. Just on commit. https://codereview.chromium.org/12261026/diff/9001/build/util/run-bisect-perf... File build/util/run-bisect-perf-regression.cfg (right): https://codereview.chromium.org/12261026/diff/9001/build/util/run-bisect-perf... build/util/run-bisect-perf-regression.cfg:18: 'bad_revision': An svn revision sometime after the metric had regressed. Aren't these svn or git?
New snapshot uploaded. Yeah I can definitely move these, just submitted it to the same place as the other 2 files for the time being until Chase responds. https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py File build/util/PRESUBMIT.py (right): https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... build/util/PRESUBMIT.py:5: """Top-level presubmit script for testing. On 2013/02/14 23:30:06, tonyg wrote: > Comment needs updating Done. https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... build/util/PRESUBMIT.py:21: valid_keys = ['command', 'good_revision', 'bad_revision', 'metric'] On 2013/02/14 23:30:06, tonyg wrote: > I'm not sure I'd lock down the key names. We might add a new argument in the > future and it would be totally valid to submit that. If you added a new key name, would you not want PRESUBMIT.py updated with it? https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... build/util/PRESUBMIT.py:47: return CommonChecks(input_api, output_api) On 2013/02/14 23:30:06, tonyg wrote: > I'm not sure I'd perform this check on upload. Just on commit. I'm a little unfamiliar with the PRESUBMIT scripts, but since CheckChangeOnUpload covers the 'git cl upload' case, wouldn't you want to throw out an error in that event too? https://codereview.chromium.org/12261026/diff/9001/build/util/run-bisect-perf... File build/util/run-bisect-perf-regression.cfg (right): https://codereview.chromium.org/12261026/diff/9001/build/util/run-bisect-perf... build/util/run-bisect-perf-regression.cfg:18: 'bad_revision': An svn revision sometime after the metric had regressed. On 2013/02/14 23:30:06, tonyg wrote: > Aren't these svn or git? Done.
> https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... > build/util/PRESUBMIT.py:21: valid_keys = ['command', 'good_revision', > 'bad_revision', 'metric'] > On 2013/02/14 23:30:06, tonyg wrote: > > I'm not sure I'd lock down the key names. We might add a new argument in the > > future and it would be totally valid to submit that. > > If you added a new key name, would you not want PRESUBMIT.py updated with it? I was looking at the presubmit as a way of preventing accidental submissions. Without it, it seems really easy to submit a change that was only intended for the tryservers. However, I don't see it being likely that a new field would accidentally be added. So I don't see the need to have to update the presubmit any time the list of fields changes. Seems better to just leave it open. > > https://codereview.chromium.org/12261026/diff/9001/build/util/PRESUBMIT.py#ne... > build/util/PRESUBMIT.py:47: return CommonChecks(input_api, output_api) > On 2013/02/14 23:30:06, tonyg wrote: > > I'm not sure I'd perform this check on upload. Just on commit. > > I'm a little unfamiliar with the PRESUBMIT scripts, but since > CheckChangeOnUpload covers the 'git cl upload' case, wouldn't you want to throw > out an error in that event too? Yeah, I suppose you are right. There's no need to upload a patch to that file. Only to send it to the tryservers. Good call.
New snapshot uploaded. Added Mike's suggestions as well.
https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... File build/util/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:33: "url" : "ssh://simonhatch@gerrit-int.chromium.org:29419/" + I don't think we want to check in your user id here. Maybe Mike knows how the existing bots manage a src-internal checkout. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:53: 2 line breaks between top level definitions (throughout the file) https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:129: '--debug_ignore_sync', Is this supposed to be checked in? If so, please add a comment explaining. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:151: os.chdir(os.path.join(working_directory)) why the join call? Isn't working_directory just a string? https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:155: if e.errno != errno.EEXIST: To make sure I understand this: the directory is not deleted between runs and we expect that on the bot it will typically stay there and this script will be able to avoid creating a checkout from scratch every time? If so, nice :) If not, can we make that the case so that it will run faster? https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:161: nit: extra line break https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:170: 'the bisection.') This description doesn't seem quite right. It is the path to create the src directory, but it makes it sound like it should already exist. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:190: if not SetupGitDepot(): Did you consider making the git checkout creation an optional part of the main script instead of in this wrapper script. Then if QA has to run it manually for some reason, it will be all self contained for them (they don't have chromium checkouts typically).
New snapshot uploaded. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... File build/util/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:33: "url" : "ssh://simonhatch@gerrit-int.chromium.org:29419/" + On 2013/02/19 18:01:25, tonyg wrote: > I don't think we want to check in your user id here. Maybe Mike knows how the > existing bots manage a src-internal checkout. Ah darn, forgot about that. Q for Mike: Do I need to include any particular user id here? https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:53: On 2013/02/19 18:01:25, tonyg wrote: > 2 line breaks between top level definitions (throughout the file) Done. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:129: '--debug_ignore_sync', On 2013/02/19 18:01:25, tonyg wrote: > Is this supposed to be checked in? If so, please add a comment explaining. Done. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:151: os.chdir(os.path.join(working_directory)) On 2013/02/19 18:01:25, tonyg wrote: > why the join call? Isn't working_directory just a string? Done. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:155: if e.errno != errno.EEXIST: On 2013/02/19 18:01:25, tonyg wrote: > To make sure I understand this: the directory is not deleted between runs and we > expect that on the bot it will typically stay there and this script will be able > to avoid creating a checkout from scratch every time? If so, nice :) If not, can > we make that the case so that it will run faster? Yep the intention is that the directory sticks around between runs. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:161: On 2013/02/19 18:01:25, tonyg wrote: > nit: extra line break Done. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:170: 'the bisection.') On 2013/02/19 18:01:25, tonyg wrote: > This description doesn't seem quite right. It is the path to create the src > directory, but it makes it sound like it should already exist. Done. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:190: if not SetupGitDepot(): On 2013/02/19 18:01:25, tonyg wrote: > Did you consider making the git checkout creation an optional part of the main > script instead of in this wrapper script. Then if QA has to run it manually for > some reason, it will be all self contained for them (they don't have chromium > checkouts typically). Yeah I was a bit undecided on whether to put the functionality here or in the main script. It just felt a little cleaner to separate out the functionality of grabbing the depot from the actual bisection, but I don't have strong feelings about it either way. If QA were to run it, why could they not just run the wrapper with the appropriate parameters?
https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... File build/util/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:190: if not SetupGitDepot(): On 2013/02/19 18:33:51, shatch wrote: > On 2013/02/19 18:01:25, tonyg wrote: > > Did you consider making the git checkout creation an optional part of the main > > script instead of in this wrapper script. Then if QA has to run it manually > for > > some reason, it will be all self contained for them (they don't have chromium > > checkouts typically). > > Yeah I was a bit undecided on whether to put the functionality here or in the > main script. It just felt a little cleaner to separate out the functionality of > grabbing the depot from the actual bisection, but I don't have strong feelings > about it either way. > > If QA were to run it, why could they not just run the wrapper with the > appropriate parameters? The thing about running the wrapper locally is that it is awkward to edit the config file with the params instead of just passing them on the command line.
New snapshot uploaded. Moved the git checkout creation to the main script. https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... File build/util/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/13001/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:190: if not SetupGitDepot(): On 2013/02/19 19:45:57, tonyg wrote: > On 2013/02/19 18:33:51, shatch wrote: > > On 2013/02/19 18:01:25, tonyg wrote: > > > Did you consider making the git checkout creation an optional part of the > main > > > script instead of in this wrapper script. Then if QA has to run it manually > > for > > > some reason, it will be all self contained for them (they don't have > chromium > > > checkouts typically). > > > > Yeah I was a bit undecided on whether to put the functionality here or in the > > main script. It just felt a little cleaner to separate out the functionality > of > > grabbing the depot from the actual bisection, but I don't have strong feelings > > about it either way. > > > > If QA were to run it, why could they not just run the wrapper with the > > appropriate parameters? > > The thing about running the wrapper locally is that it is awkward to edit the > config file with the params instead of just passing them on the command line. Done.
Since this calls gclient explicitly, it should be in tools/build/scripts/slave/. This looks really good so far, good idea to put the presubmit check that it's always empty. https://codereview.chromium.org/12261026/diff/21002/build/util/run-bisect-per... File build/util/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/21002/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:20: extra line here https://codereview.chromium.org/12261026/diff/21002/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:32: cfg_file = imp.load_source('config', 'run-bisect-perf-regression.cfg') I'm not sure if imp.load_source() is needed here. We have stuff in tools/build/scripts/common/ that loads config files and returns dictionaries. Right now they're tailored to work with slaves.cfg, but there is a CL out to have them return the entire dict of whatever is created in the file. See ParsePythonCFG() in https://chromiumcodereview.appspot.com/12300004/diff/3001/scripts/common/chro...
Removed the run-bisect files and will upload a new change with them. https://codereview.chromium.org/12261026/diff/21002/build/util/run-bisect-per... File build/util/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/21002/build/util/run-bisect-per... build/util/run-bisect-perf-regression.py:20: On 2013/02/20 01:04:44, Mike Stipicevic wrote: > extra line here Done.
New snapshot uploaded. Moved files back to src depot into src/tools
lgtm Please wait for Mike to approve too. https://codereview.chromium.org/12261026/diff/29002/tools/bisect-perf-regress... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/29002/tools/bisect-perf-regress... tools/bisect-perf-regression.py:1318: if len(metric_values) < 2: Nice test. Should it be '!= 2' instead of '< 2'? https://codereview.chromium.org/12261026/diff/29002/tools/run-bisect-perf-reg... File tools/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/29002/tools/run-bisect-perf-reg... tools/run-bisect-perf-regression.py:68: return 0 Why not just return return_code in all cases?
New snapshot uploaded. https://codereview.chromium.org/12261026/diff/29002/tools/bisect-perf-regress... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/29002/tools/bisect-perf-regress... tools/bisect-perf-regression.py:1318: if len(metric_values) < 2: On 2013/02/20 23:19:41, tonyg wrote: > Nice test. Should it be '!= 2' instead of '< 2'? Done. https://codereview.chromium.org/12261026/diff/29002/tools/run-bisect-perf-reg... File tools/run-bisect-perf-regression.py (right): https://codereview.chromium.org/12261026/diff/29002/tools/run-bisect-perf-reg... tools/run-bisect-perf-regression.py:68: return 0 On 2013/02/20 23:19:41, tonyg wrote: > Why not just return return_code in all cases? Done.
lgtm with nit/change https://chromiumcodereview.appspot.com/12261026/diff/37001/tools/bisect-perf-... File tools/bisect-perf-regression.py (right): https://chromiumcodereview.appspot.com/12261026/diff/37001/tools/bisect-perf-... tools/bisect-perf-regression.py:157: print '@@@STEP_STARTED@@@' check if this shows up OK on the waterfall. if it's all blank steps, then you'll need to add a STEP_TEXT with the step name here. https://chromiumcodereview.appspot.com/12261026/diff/37001/tools/bisect-perf-... tools/bisect-perf-regression.py:1218: if output_buildbot_annotations: probably want to use OutputAnnotationStepStart() here https://chromiumcodereview.appspot.com/12261026/diff/37001/tools/bisect-perf-... tools/bisect-perf-regression.py:1230: print OutputAnnotationStepClosed()
New snapshot uploaded. https://chromiumcodereview.appspot.com/12261026/diff/37001/tools/bisect-perf-... File tools/bisect-perf-regression.py (right): https://chromiumcodereview.appspot.com/12261026/diff/37001/tools/bisect-perf-... tools/bisect-perf-regression.py:157: print '@@@STEP_STARTED@@@' On 2013/02/21 02:15:32, Mike Stipicevic wrote: > check if this shows up OK on the waterfall. if it's all blank steps, then you'll > need to add a STEP_TEXT with the step name here. Seems to show up ok on my local buildbot. https://chromiumcodereview.appspot.com/12261026/diff/37001/tools/bisect-perf-... tools/bisect-perf-regression.py:1218: if output_buildbot_annotations: On 2013/02/21 02:15:32, Mike Stipicevic wrote: > probably want to use OutputAnnotationStepStart() here Done. https://chromiumcodereview.appspot.com/12261026/diff/37001/tools/bisect-perf-... tools/bisect-perf-regression.py:1230: print On 2013/02/21 02:15:32, Mike Stipicevic wrote: > OutputAnnotationStepClosed() Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonhatch@chromium.org/12261026/40001
Message was sent while issue was closed.
Change committed as 183827 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
