Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196653010:
Reads bench expectations from skia-autogen svn repo. (Closed)
|
Created:
6 years, 9 months ago by benchen Modified:
6 years, 9 months ago CC:
skia-review_googlegroups.com, skiabot_google.com Base URL:
https://skia.googlesource.com/buildbot.git@master Visibility:
Public. |
DescriptionReads bench expectations from skia-autogen svn repo.
BUG=skia:2225
NOTRY=true
Committed: https://skia.googlesource.com/buildbot/+/a6239ec37c751a3c531edd73f296d5b5fe07cd2e
Patch Set 1 #
Total comments: 10
Patch Set 2 : patch 1 to address comments. #
Total comments: 2
Patch Set 3 : switch to using shell_utils. #
Total comments: 1
Patch Set 4 : uses join to create path. #
Messages
Total messages: 25 (0 generated)
The latest bench expectations are already at http://skia-autogen.googlecode.com/svn/bench/
https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/bui... File slave/skia_slave_scripts/build_step.py (right): https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/bui... slave/skia_slave_scripts/build_step.py:222: args['perf_output_basedir'], self._builder_name, 'autogen_upload') These will soon be used by other steps than check_for_regressions, right? If not, we should put them in that step. https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... File slave/skia_slave_scripts/check_for_regressions.py (right): https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... slave/skia_slave_scripts/check_for_regressions.py:38: print 'Skip due to missing expectations: %s' % url You never check again to see whether http_response is None... Can we just return here? https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... slave/skia_slave_scripts/check_for_regressions.py:42: raise Exception('URLError while reading expectations: %s' % e.reason) I would very strongly prefer to perform a source code checkout (or even just "svn cat") instead of using HTTP. The codesite HTTP interface has proven to be extremely unreliable in the past. https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... slave/skia_slave_scripts/check_for_regressions.py:53: os.makedirs(self._perf_range_input_dir) Won't the below write replace the file if it already exists? I think we could probably change the above four lines to just: os.makedirs(self._perf_range_input_dir) https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... slave/skia_slave_scripts/check_for_regressions.py:56: file_handle.close() I'd think the "with" syntax is cleaner: with open(path_to_bench_expectations, 'w') as file_handle: file_handle.write(http_response.read())
addressed all comments. Please take another look. Thanks! https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/bui... File slave/skia_slave_scripts/build_step.py (right): https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/bui... slave/skia_slave_scripts/build_step.py:222: args['perf_output_basedir'], self._builder_name, 'autogen_upload') Yes, it's clearer that way. Deleted perf_range_autogen_upload_dir for now. On 2014/03/13 20:45:37, borenet wrote: > These will soon be used by other steps than check_for_regressions, right? If > not, we should put them in that step. https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... File slave/skia_slave_scripts/check_for_regressions.py (right): https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... slave/skia_slave_scripts/check_for_regressions.py:38: print 'Skip due to missing expectations: %s' % url I must've deleted the line by mistake.. On 2014/03/13 20:45:37, borenet wrote: > You never check again to see whether http_response is None... Can we just return > here? https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... slave/skia_slave_scripts/check_for_regressions.py:42: raise Exception('URLError while reading expectations: %s' % e.reason) I think svn cat is a good idea. Switched to using it. On 2014/03/13 20:45:37, borenet wrote: > I would very strongly prefer to perform a source code checkout (or even just > "svn cat") instead of using HTTP. The codesite HTTP interface has proven to be > extremely unreliable in the past. https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... slave/skia_slave_scripts/check_for_regressions.py:53: os.makedirs(self._perf_range_input_dir) On 2014/03/13 20:45:37, borenet wrote: > Won't the below write replace the file if it already exists? I think we could > probably change the above four lines to just: > > os.makedirs(self._perf_range_input_dir) oh I wasn't sure about it. Done. https://codereview.chromium.org/196653010/diff/1/slave/skia_slave_scripts/che... slave/skia_slave_scripts/check_for_regressions.py:56: file_handle.close() On 2014/03/13 20:45:37, borenet wrote: > I'd think the "with" syntax is cleaner: > > with open(path_to_bench_expectations, 'w') as file_handle: > file_handle.write(http_response.read()) Done.
Just one remaining comment... https://codereview.chromium.org/196653010/diff/20001/slave/skia_slave_scripts... File slave/skia_slave_scripts/check_for_regressions.py (right): https://codereview.chromium.org/196653010/diff/20001/slave/skia_slave_scripts... slave/skia_slave_scripts/check_for_regressions.py:35: (stdout, stderr) = proc.communicate() Please use shell_utils.run() for this: try: output = shell_utils.run([svn_binary, 'cat', url]) except shell_utils.CommandFailedException: print 'Skip due to missing expectations: %s' % url return
Done. This is more convenient. https://codereview.chromium.org/196653010/diff/20001/slave/skia_slave_scripts... File slave/skia_slave_scripts/check_for_regressions.py (right): https://codereview.chromium.org/196653010/diff/20001/slave/skia_slave_scripts... slave/skia_slave_scripts/check_for_regressions.py:35: (stdout, stderr) = proc.communicate() Done. Thanks for the suggestion. On 2014/03/14 12:06:46, borenet wrote: > Please use shell_utils.run() for this: > > try: > output = shell_utils.run([svn_binary, 'cat', url]) > except shell_utils.CommandFailedException: > print 'Skip due to missing expectations: %s' % url > return
LGTM https://codereview.chromium.org/196653010/diff/40001/slave/skia_slave_scripts... File slave/skia_slave_scripts/check_for_regressions.py (right): https://codereview.chromium.org/196653010/diff/40001/slave/skia_slave_scripts... slave/skia_slave_scripts/check_for_regressions.py:29: url = '%s/%s/%s' % (AUTOGEN_SVN_BASEURL, 'bench', expectations_filename) Optional: '/'.join(AUTOGEN_SVN_BASEURL, ....)
The CQ bit was checked by bensong@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bensong@google.com/196653010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by bensong@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bensong@google.com/196653010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was checked by bensong@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bensong@google.com/196653010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was checked by bensong@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bensong@google.com/196653010/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was checked by agable@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bensong@google.com/196653010/60001
Message was sent while issue was closed.
Change committed as a6239ec37c751a3c531edd73f296d5b5fe07cd2e |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
