Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10837114:
Automate Chrome Endure setup process. (Closed)
|
Created:
8 years, 4 months ago by fdeng1 Modified:
8 years, 4 months ago CC:
chromium-reviews, anantha, dyu1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAutomate Chrome Endure setup process.
This script automates Chrome Endure setup process and
starts a local Http server to serve the results.
BUG=None
TEST=Ran the script on two Linux_x64 machines.
NOTRY=TRUE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152132
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : get rid of revision number option #
Total comments: 32
Patch Set 4 : Address comments #
Total comments: 45
Patch Set 5 : Not done yet. #Patch Set 6 : Address Dennis' Comments #
Total comments: 14
Patch Set 7 : Address Dennis' Comments #
Messages
Total messages: 22 (0 generated)
Hi Nirnimesh and Dennis, Here is the script I've been working on. It automates the setup process of Chrome Endure. I would like to see how you think about the current version. Thanks, Fang https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:332: class EndureHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler): I tried to reuse the TestServer in pyautolib.py. I found it a little complicated. This script is supposed to run from an empty directory. At that moment, pyauto binary is not ready for use so to use it I have to import it after pyauto binary has been downloaded. Another obstacle I had was that TestServer only serves a directory that is within "src" folder and do not allow user to specify an arbitrary directory. I figured here we really need a very basic HTTP server. So I tried BaseHTTPServer.HTTPServer with a customized request handler. I'd like to hear how you think about this. https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:335: def send_head(self): I wish I didn't have to override this function. But SimpleHTTPRequestHandler.send_head tries to append '/' to a URL if that URL is not ended with '/'. It then tries to redirect the browser to that new URL. This causes problems as '/' will become part of the value of the last query string. And this behavior is not expected by current graph code.
https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:164: sync_cmd.extend(['--revision', 'functional.DEPS@'+(revision)]) Here the script tries to run "gclient sync --revision functional.DEPS@REV_NUM". It will first fetch "functional.DEPS" and then fetch stuffs that are specified inside functional.DEPS. The problem here is that REV_NUM is only effective on "functional.DEP". For other stuffs that are specified in functional.DEPS, gclient sync will only fetch the latest version. I am trying to find a workaround and would like to get some feedback from you.
cool! i have some high-level comments https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:164: sync_cmd.extend(['--revision', 'functional.DEPS@'+(revision)]) On 2012/08/07 22:19:45, fdeng1 wrote: > Here the script tries to run "gclient sync --revision functional.DEPS@REV_NUM". > It will first fetch "functional.DEPS" and then fetch stuffs that are specified > inside functional.DEPS. The problem here is that REV_NUM is only effective on > "functional.DEP". For other stuffs that are specified in functional.DEPS, > gclient sync will only fetch the latest version. I am trying to find a > workaround and would like to get some feedback from you. As discussed offline, I think it'll be fine for now to just fetch the latest revision. No need to specify an older revision. https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:332: class EndureHTTPRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler): On 2012/08/07 22:09:43, fdeng1 wrote: > I tried to reuse the TestServer in pyautolib.py. I found it a little > complicated. This script is supposed to run from an empty directory. At that > moment, pyauto binary is not ready for use so to use it I have to import it > after pyauto binary has been downloaded. Another obstacle I had was that > TestServer only serves a directory that is within "src" folder and do not allow > user to specify an arbitrary directory. I figured here we really need a very > basic HTTP server. So I tried BaseHTTPServer.HTTPServer with a customized > request handler. I'd like to hear how you think about this. I think it's fine to have our own basic test server; there's not too much code here that we need to implement it, and it removes a dependency on their TestServer code. https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:335: def send_head(self): On 2012/08/07 22:09:43, fdeng1 wrote: > I wish I didn't have to override this function. But > SimpleHTTPRequestHandler.send_head tries to append '/' to a URL if that URL is > not ended with '/'. It then tries to redirect the browser to that new URL. This > causes problems as '/' will become part of the value of the last query string. > And this behavior is not expected by current graph code. As discussed offline, let's no longer override this function. It might not be necessary to do so with the latest graph code, and even if it is, we can likely handle it with a much smaller code change in the graphing code itself. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:24: """ in this docstring, also list the dependencies that this module has. For example, it requires fetch_prebuilt_pyauto.py, among other things. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:103: print 'Checking depot tools...' i recommend using the "logging" module instead of print so we can choose/control the verbosity level with which we output logging messages. We may want some of these messages printed on every run, but there might be other less-important messages that we may only want to output when debugging or when we explicitly want verbose output https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:121: # without checking OSError, a better way? it's possible depot_tools may already be installed on the system somewhere, but the "gclient" command may just not be in the path. so it's possible you may end up installing a second copy of depot tools here. Why not skip the check entirely and just install depot tools into the known location in self._depot_dir regardless of whether it might already be installed on the system? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:126: subprocess.call([self._gclient, '--version']) is there any way to verify that we were successful in fetching depot tools? Maybe check to make sure the expected directory exists? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:133: config_cmd = [self._gclient, 'config', self._URLS['pyauto']] Did Nirnimesh say it's not sufficient to use pyauto's functional.DEPS here? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:142: os.chdir(cur_dir) should we also check to see whether an expected directory exists, to verify that pyauto was likely fetched properly? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:155: code = subprocess.call(cmd) if this is successful, maybe we could log a message saying which version of chrome was downloaded. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:165: print 'Cleaning %s, ' % self._graph_dir, Do we need to do all this work to clean the old directory? What happens if we just extract the new graph files into the destination directory? If it simply overwrites the files that need to be overwritten and ignores the other files already there (e.g., .dat files), then that's likely what we want. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:204: os.chmod(name, info.external_attr >> 16L) did you get this code from somewhere? should we cite the source if so? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:209: """Start an http server which serves the Chrome Endure test results. 'test results' --> 'graphs'. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:329: def Main(argv): maybe we should split up this script into 2 separate scripts. One could be called endure_setup.py, which will just do the "fetch" command. Another could be called endure_graph_server.py, which will just do the "server" command. I think it's worthwhile to make 2 different scripts for both of those primary functions, since they're independent of each other anyway. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:335: cmds = [CmdFetch, CmdServer, CmdHelp] I think it's easier to use optparse for parsing args. That'll eliminate the need for the CmdHelp class too. If you split up this file into 2 separate scripts, we can just use optparse to parse the args in both scripts.
Hi Dennis, Thanks for taking a look at this CL. I have some comments about the server function. I'll address other comments later. Fang https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:335: def send_head(self): On 2012/08/08 23:41:07, dennis_jeffrey wrote: > On 2012/08/07 22:09:43, fdeng1 wrote: > > I wish I didn't have to override this function. But > > SimpleHTTPRequestHandler.send_head tries to append '/' to a URL if that URL is > > not ended with '/'. It then tries to redirect the browser to that new URL. > This > > causes problems as '/' will become part of the value of the last query string. > > And this behavior is not expected by current graph code. > > As discussed offline, let's no longer override this function. It might not be > necessary to do so with the latest graph code, and even if it is, we can likely > handle it with a much smaller code change in the graphing code itself. I am able to solve the problem by modifying the second line in "common.js":ParseParams() Change "var s = window.location.search.substring(1).split('&');" to the following: var query = window.location.search.substring(1) if (query.charAt(query.length-1) =='/') query = query.substring(0, query.length-1) var s = query.split('&'); https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:329: def Main(argv): On 2012/08/08 23:41:07, dennis_jeffrey wrote: > maybe we should split up this script into 2 separate scripts. One could be > called endure_setup.py, which will just do the "fetch" command. Another could > be called endure_graph_server.py, which will just do the "server" command. I > think it's worthwhile to make 2 different scripts for both of those primary > functions, since they're independent of each other anyway. Since we are able to solve the problem brought by extra '/' (by modifying the graph code), the user can simply call "python -m SimpleHTTPServer 8000" under the graph dir to start a server. The difference is that he needs to go inside that folder as SimpleHTTPServer don't taken a directory as input. He also need to pick a free port by himself. Maybe we can get rid of the server feature and just document it somewhere(e.g. the help message)?
https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/4001/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:335: def send_head(self): On 2012/08/09 03:26:16, fdeng1 wrote: > On 2012/08/08 23:41:07, dennis_jeffrey wrote: > > On 2012/08/07 22:09:43, fdeng1 wrote: > > > I wish I didn't have to override this function. But > > > SimpleHTTPRequestHandler.send_head tries to append '/' to a URL if that URL > is > > > not ended with '/'. It then tries to redirect the browser to that new URL. > > This > > > causes problems as '/' will become part of the value of the last query > string. > > > And this behavior is not expected by current graph code. > > > > As discussed offline, let's no longer override this function. It might not be > > necessary to do so with the latest graph code, and even if it is, we can > likely > > handle it with a much smaller code change in the graphing code itself. > > I am able to solve the problem by modifying the second line in > "common.js":ParseParams() > Change "var s = window.location.search.substring(1).split('&');" to the > following: > var query = window.location.search.substring(1) > if (query.charAt(query.length-1) =='/') > query = query.substring(0, query.length-1) > var s = query.split('&'); Great! I made the change on the live chrome endure graphs, and also in the local_graphs.zip file. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:329: def Main(argv): On 2012/08/09 03:26:16, fdeng1 wrote: > On 2012/08/08 23:41:07, dennis_jeffrey wrote: > > maybe we should split up this script into 2 separate scripts. One could be > > called endure_setup.py, which will just do the "fetch" command. Another could > > be called endure_graph_server.py, which will just do the "server" command. I > > think it's worthwhile to make 2 different scripts for both of those primary > > functions, since they're independent of each other anyway. > > Since we are able to solve the problem brought by extra '/' (by modifying the > graph code), the user can simply call "python -m SimpleHTTPServer 8000" under > the graph dir to start a server. The difference is that he needs to go inside > that folder as SimpleHTTPServer don't taken a directory as input. He also need > to pick a free port by himself. Maybe we can get rid of the server feature and > just document it somewhere(e.g. the help message)? > I like the feature of having a free port automatically selected for the user though, and not having to go into that directory manually to start the server. It might still be worthwhile to write a small, separate script that will automatically launch the server for the user and tell him/her the URL to go to, to see the graphs.
https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:1: """Automate the setup process of Chrome Endure environment. license header? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:56: python endure_setup.py fetch [options] leave a blank line
https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:1: """Automate the setup process of Chrome Endure environment. On 2012/08/09 17:28:05, Nirnimesh wrote: > license header? Done. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:24: """ Add some text about dependencies on depot_tools and fetch_prebuilt_pyauto.py. On 2012/08/08 23:41:07, dennis_jeffrey wrote: > in this docstring, also list the dependencies that this module has. For > example, it requires fetch_prebuilt_pyauto.py, among other things. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:56: python endure_setup.py fetch [options] On 2012/08/09 17:28:05, Nirnimesh wrote: > leave a blank line Done. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:103: print 'Checking depot tools...' Switch from "print" to "logging". On 2012/08/08 23:41:07, dennis_jeffrey wrote: > i recommend using the "logging" module instead of print so we can choose/control > the verbosity level with which we output logging messages. We may want some of > these messages printed on every run, but there might be other less-important > messages that we may only want to output when debugging or when we explicitly > want verbose output https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:121: # without checking OSError, a better way? On 2012/08/08 23:41:07, dennis_jeffrey wrote: > it's possible depot_tools may already be installed on the system somewhere, but > the "gclient" command may just not be in the path. so it's possible you may end > up installing a second copy of depot tools here. > > Why not skip the check entirely and just install depot tools into the known > location in self._depot_dir regardless of whether it might already be installed > on the system? Done. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:126: subprocess.call([self._gclient, '--version']) Added code to check that gclient/gclient.bat(win) and gclient.py exist. On 2012/08/08 23:41:07, dennis_jeffrey wrote: > is there any way to verify that we were successful in fetching depot tools? > Maybe check to make sure the expected directory exists? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:133: config_cmd = [self._gclient, 'config', self._URLS['pyauto']] I think we were talking about webpagereplay was not added to functinoal.DEPS. I can submit a separate CL about this. I'll confirm with him whether there is anything else. On 2012/08/08 23:41:07, dennis_jeffrey wrote: > Did Nirnimesh say it's not sufficient to use pyauto's functional.DEPS here? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:142: os.chdir(cur_dir) Add _CheckPyAuto() Check src/chrome/test/pyatuolib/pyauto.py and src/chrome/test/pyatuolib/fetch_prebuilt_pyauto.py exist. I was thinking checking more files such as perf.py perf_endure.py too but I remember we had a plan to move them in to functional/perf? On 2012/08/08 23:41:07, dennis_jeffrey wrote: > should we also check to see whether an expected directory exists, to verify that > pyauto was likely fetched properly? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:155: code = subprocess.call(cmd) On 2012/08/08 23:41:07, dennis_jeffrey wrote: > if this is successful, maybe we could log a message saying which version of > chrome was downloaded. Done. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:165: print 'Cleaning %s, ' % self._graph_dir, On 2012/08/08 23:41:07, dennis_jeffrey wrote: > Do we need to do all this work to clean the old directory? What happens if we > just extract the new graph files into the destination directory? If it simply > overwrites the files that need to be overwritten and ignores the other files > already there (e.g., .dat files), then that's likely what we want. Done. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:204: os.chmod(name, info.external_attr >> 16L) Mentioned it in docstring that it is adapted from fetch_prebuilt_pyauto.py On 2012/08/08 23:41:07, dennis_jeffrey wrote: > did you get this code from somewhere? should we cite the source if so? https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:209: """Start an http server which serves the Chrome Endure test results. On 2012/08/08 23:41:07, dennis_jeffrey wrote: > 'test results' --> 'graphs'. Done. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:329: def Main(argv): On 2012/08/09 17:16:27, dennis_jeffrey wrote: > On 2012/08/09 03:26:16, fdeng1 wrote: > > On 2012/08/08 23:41:07, dennis_jeffrey wrote: > > > maybe we should split up this script into 2 separate scripts. One could be > > > called endure_setup.py, which will just do the "fetch" command. Another > could > > > be called endure_graph_server.py, which will just do the "server" command. > I > > > think it's worthwhile to make 2 different scripts for both of those primary > > > functions, since they're independent of each other anyway. > > > > Since we are able to solve the problem brought by extra '/' (by modifying the > > graph code), the user can simply call "python -m SimpleHTTPServer 8000" under > > the graph dir to start a server. The difference is that he needs to go inside > > that folder as SimpleHTTPServer don't taken a directory as input. He also need > > to pick a free port by himself. Maybe we can get rid of the server feature and > > just document it somewhere(e.g. the help message)? > > > > I like the feature of having a free port automatically selected for the user > though, and not having to go into that directory manually to start the server. > It might still be worthwhile to write a small, separate script that will > automatically launch the server for the user and tell him/her the URL to go to, > to see the graphs. That sounds reasonable. I've moved it to a separate script. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:335: cmds = [CmdFetch, CmdServer, CmdHelp] On 2012/08/08 23:41:07, dennis_jeffrey wrote: > I think it's easier to use optparse for parsing args. That'll eliminate the > need for the CmdHelp class too. If you split up this file into 2 separate > scripts, we can just use optparse to parse the args in both scripts. Done.
https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:106: self._gclient = os.path.join(self._depot_dir, 'gclient.bat') I haven't got a chance to test this script on other platforms. Is there a way to do it or do we assume it runs only on linux?
Thanks for the refactoring. More comments below. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:133: config_cmd = [self._gclient, 'config', self._URLS['pyauto']] On 2012/08/14 19:11:40, fdeng1 wrote: > I think we were talking about webpagereplay was not added to functinoal.DEPS. I > can submit a separate CL about this. I'll confirm with him whether there is > anything else. > > On 2012/08/08 23:41:07, dennis_jeffrey wrote: > > Did Nirnimesh say it's not sufficient to use pyauto's functional.DEPS here? > Ok. I thought there was some problem where the functional.DEPS won't be able to pull in the correct version of WPR. If that's the case, we'll have to figure out another way to do it. https://chromiumcodereview.appspot.com/10837114/diff/3/chrome/test/functional... chrome/test/functional/perf/endure_setup.py:142: os.chdir(cur_dir) On 2012/08/14 19:11:40, fdeng1 wrote: > Add _CheckPyAuto() > Check src/chrome/test/pyatuolib/pyauto.py > and src/chrome/test/pyatuolib/fetch_prebuilt_pyauto.py exist. I think this is good. I just thought a simple sanity check would be good to make sure things were likely successful in checking out the code. > I was thinking checking more files such as perf.py perf_endure.py too but I > remember we had a plan to move them in to functional/perf? > We probably don't need to check for those. And you're right, eventually I'd like to move the other perf files into the perf folder. > On 2012/08/08 23:41:07, dennis_jeffrey wrote: > > should we also check to see whether an expected directory exists, to verify > that > > pyauto was likely fetched properly? > https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... File chrome/test/functional/perf/endure_server.py (right): https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:17: python endure_server.py add a ">" at the beginning of this line to highlight the fact that it's a command to be typed at the command line: > python endure_server.py https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:21: python endure_server.py --graph-dir=/home/user/Document/graph_dir same comment as line 17 above https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:25: import BaseHTTPServer nit: add a blank line above this to separate the docstring above from the imports https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:36: def format_description(self, description): add a comment to explain why you're overriding this function https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:40: return '' i think this could be combined into a one-liner: return description + '\n' if description else '' https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:18: TEST_LENGTH=30 LOCAL_GRAPH="<ENDURE_DIR>/chrome_graph" \\ should LOCAL_GRAPH be LOCAL_PERF_DIR? https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:56: def format_description(self, description): add a comment explaining why you're overriding this function https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:60: return '' similar comment as in the other file: i think this can be re-written as a single line https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:63: class CmdFetch(object): this probably doesn't need to be a class. There's no longer two separate "commands" in this file since the server functionality got moved to another file. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:70: python endure_setup.py similar comment as in the other file: add a > at the start of this line to make it clear that this is something to type in at the command line https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:74: python endure_setup.py --endure-dir=/home/user/endure_dir same comment as line 70 above https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:86: '~dennisjeffrey/chrome_perf/local_graphs.zip'), for now, let's remove this directory from the script so that the internal URL is not checked in with this external file. I recommend we do this: for now, add a new option to this script called "graph_zip_url" which will be the URL to a zip file containing the chrome graphs (along with it, add a TODO saying to remove it once the Chrome Endure graphing code is checked in). Then when we give this script to people to run, we'll separately tell them the internal URL from which they can download the graphing code, then they can specify that as a command-line option to this script. As soon as the chrome endure graphing code is checked in, we'll simplify this and the script will just check out the graphing code from the chrome tree along with everything else it checks out. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:106: self._gclient = os.path.join(self._depot_dir, 'gclient.bat') On 2012/08/14 19:14:20, fdeng1 wrote: > I haven't got a chance to test this script on other platforms. Is there a way to > do it or do we assume it runs only on linux? Let's assume for now that it's only going to run on linux. I haven't tried running the endurance tests on Win or Mac yet. It should work in theory, but in practice there may be a few kinks to iron out to get it to work. Let's just focus on linux right now. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:133: else: maybe we can just remove the above 3 lines and always check out the depot tools. Even if the depot tools are already checked out, then checking them out again shouldn't hurt. In fact, if the depot tools are updated since the last time we ran this setup script, then checking the files out again will give us the latest version, which is probably a good thing. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:163: if code != 0: can just get rid of the "code" variable: if subprocess.call(config_cmd) != 0: https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:168: if code != 0: same comment as line 163 above https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:194: if code == 0: same comment as line 163 above https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:210: """Get a string representation for the current os. nit: os --> OS https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:214: Raises: add a blank line right above this https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:215: RuntimeError: if os can't be identified. nit: os --> OS https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:246: raise SetupError('Unable to get the latest revision number from %s' % remove one of the spaces in "get the" https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:251: def _UnzipFilenameToDir(cls, filename, directory): This function will probably go away as soon as the Chrome Endure graphs are checked in, because then the graph files can be directly checked out from the chrome tree (no need to unzip anything). We should add a TODO here saying to remove this function as soon as the Chrome Endure graphing code is checked into the chrome tree.
Hi Dennis, Thanks for the comments. I addressed them. Please take another look. Fang https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... File chrome/test/functional/perf/endure_server.py (right): https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:17: python endure_server.py On 2012/08/15 17:53:08, dennis_jeffrey wrote: > add a ">" at the beginning of this line to highlight the fact that it's a > command to be typed at the command line: > > > python endure_server.py Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:21: python endure_server.py --graph-dir=/home/user/Document/graph_dir On 2012/08/15 17:53:08, dennis_jeffrey wrote: > same comment as line 17 above Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:25: import BaseHTTPServer On 2012/08/15 17:53:08, dennis_jeffrey wrote: > nit: add a blank line above this to separate the docstring above from the > imports Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:36: def format_description(self, description): On 2012/08/15 17:53:08, dennis_jeffrey wrote: > add a comment to explain why you're overriding this function Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_server.py:40: return '' On 2012/08/15 17:53:08, dennis_jeffrey wrote: > i think this could be combined into a one-liner: > > return description + '\n' if description else '' Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:18: TEST_LENGTH=30 LOCAL_GRAPH="<ENDURE_DIR>/chrome_graph" \\ On 2012/08/15 17:53:08, dennis_jeffrey wrote: > should LOCAL_GRAPH be LOCAL_PERF_DIR? Thanks for catching it. Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:56: def format_description(self, description): On 2012/08/15 17:53:08, dennis_jeffrey wrote: > add a comment explaining why you're overriding this function Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:60: return '' On 2012/08/15 17:53:08, dennis_jeffrey wrote: > similar comment as in the other file: i think this can be re-written as a single > line Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:63: class CmdFetch(object): On 2012/08/15 17:53:08, dennis_jeffrey wrote: > this probably doesn't need to be a class. There's no longer two separate > "commands" in this file since the server functionality got moved to another > file. Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:70: python endure_setup.py On 2012/08/15 17:53:08, dennis_jeffrey wrote: > similar comment as in the other file: add a > at the start of this line to make > it clear that this is something to type in at the command line Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:74: python endure_setup.py --endure-dir=/home/user/endure_dir On 2012/08/15 17:53:08, dennis_jeffrey wrote: > same comment as line 70 above Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:86: '~dennisjeffrey/chrome_perf/local_graphs.zip'), Add an option --graph-zip-url. If not specified, will log a message saying that "Skip fetching chrome graphs since --graph-zip-url is not set" On 2012/08/15 17:53:08, dennis_jeffrey wrote: > for now, let's remove this directory from the script so that the internal URL is > not checked in with this external file. > > I recommend we do this: for now, add a new option to this script called > "graph_zip_url" which will be the URL to a zip file containing the chrome graphs > (along with it, add a TODO saying to remove it once the Chrome Endure graphing > code is checked in). Then when we give this script to people to run, we'll > separately tell them the internal URL from which they can download the graphing > code, then they can specify that as a command-line option to this script. > > As soon as the chrome endure graphing code is checked in, we'll simplify this > and the script will just check out the graphing code from the chrome tree along > with everything else it checks out. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:106: self._gclient = os.path.join(self._depot_dir, 'gclient.bat') On 2012/08/15 17:53:08, dennis_jeffrey wrote: > On 2012/08/14 19:14:20, fdeng1 wrote: > > I haven't got a chance to test this script on other platforms. Is there a way > to > > do it or do we assume it runs only on linux? > > Let's assume for now that it's only going to run on linux. I haven't tried > running the endurance tests on Win or Mac yet. It should work in theory, but in > practice there may be a few kinks to iron out to get it to work. Let's just > focus on linux right now. Ok, it now checks whether the system is Linux and I mention we only support Linux in doc string. Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:133: else: On 2012/08/15 17:53:08, dennis_jeffrey wrote: > maybe we can just remove the above 3 lines and always check out the depot tools. > Even if the depot tools are already checked out, then checking them out again > shouldn't hurt. In fact, if the depot tools are updated since the last time we > ran this setup script, then checking the files out again will give us the latest > version, which is probably a good thing. Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:163: if code != 0: On 2012/08/15 17:53:08, dennis_jeffrey wrote: > can just get rid of the "code" variable: > > if subprocess.call(config_cmd) != 0: Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:168: if code != 0: On 2012/08/15 17:53:08, dennis_jeffrey wrote: > same comment as line 163 above Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:194: if code == 0: On 2012/08/15 17:53:08, dennis_jeffrey wrote: > same comment as line 163 above Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:210: """Get a string representation for the current os. On 2012/08/15 17:53:08, dennis_jeffrey wrote: > nit: os --> OS Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:214: Raises: On 2012/08/15 17:53:08, dennis_jeffrey wrote: > add a blank line right above this Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:215: RuntimeError: if os can't be identified. On 2012/08/15 17:53:08, dennis_jeffrey wrote: > nit: os --> OS Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:246: raise SetupError('Unable to get the latest revision number from %s' % On 2012/08/15 17:53:08, dennis_jeffrey wrote: > remove one of the spaces in "get the" Done. https://chromiumcodereview.appspot.com/10837114/diff/14001/chrome/test/functi... chrome/test/functional/perf/endure_setup.py:251: def _UnzipFilenameToDir(cls, filename, directory): On 2012/08/15 17:53:08, dennis_jeffrey wrote: > This function will probably go away as soon as the Chrome Endure graphs are > checked in, because then the graph files can be directly checked out from the > chrome tree (no need to unzip anything). We should add a TODO here saying to > remove this function as soon as the Chrome Endure graphing code is checked into > the chrome tree. Done.
Looking good - few more comments. https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:59: ' Fetch the latest version of Chrome Endure to /home/user/endure_dir.\n') It might be easier to format the above within """ since that way you don't need to explicitly have all the \n's and stuff: DESCRIPTION = """ Fetch Chrome Endure. Usage: python endure_setup.py [options] ... """ https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:86: description=DESCRIPTION) probably can move this to the previous line https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:120: # TODO(fdeng): remove this after it is check into the chrome tree. check --> checked https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:176: # gclient config probably don't need this comment. we can see in the next line below that you're running gclient config https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:180: # gclient sync similar comment as line 176 above https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:193: endure_dir: Direcotry of Chrome Endure and its dependencies. Direcotry --> Directory https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:230: logging.info('Binaries at revision %s', revision) before logging this, should we also make sure that binary_dir exists?
Thank you Dennis, I addressed your comments. Do you think it is now in good shape to let Loreena try it? Fang https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... File chrome/test/functional/perf/endure_setup.py (right): https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:59: ' Fetch the latest version of Chrome Endure to /home/user/endure_dir.\n') On 2012/08/17 00:55:03, dennis_jeffrey wrote: > It might be easier to format the above within """ since that way you don't need > to explicitly have all the \n's and stuff: > > DESCRIPTION = """ > Fetch Chrome Endure. > > Usage: > python endure_setup.py [options] > ... > """ Done. https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:86: description=DESCRIPTION) On 2012/08/17 00:55:03, dennis_jeffrey wrote: > probably can move this to the previous line Done. https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:120: # TODO(fdeng): remove this after it is check into the chrome tree. On 2012/08/17 00:55:03, dennis_jeffrey wrote: > check --> checked Done. https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:176: # gclient config On 2012/08/17 00:55:03, dennis_jeffrey wrote: > probably don't need this comment. we can see in the next line below that you're > running gclient config Done. https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:180: # gclient sync On 2012/08/17 00:55:03, dennis_jeffrey wrote: > similar comment as line 176 above Done. https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:193: endure_dir: Direcotry of Chrome Endure and its dependencies. On 2012/08/17 00:55:03, dennis_jeffrey wrote: > Direcotry --> Directory Done. https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functio... chrome/test/functional/perf/endure_setup.py:230: logging.info('Binaries at revision %s', revision) On 2012/08/17 00:55:03, dennis_jeffrey wrote: > before logging this, should we also make sure that binary_dir exists? Done.
I am also thinking updating go/endure -> Local Setup. Maybe we can move the current instructions to a section called 'Manual setup' and add another one e.g. 'Automated setup'? Fang On Fri, Aug 17, 2012 at 12:05 AM, <fdeng@chromium.org> wrote: > > Thank you Dennis, > > I addressed your comments. Do you think it is now in good shape to let > Loreena > try it? > > Fang > > > > https://chromiumcodereview.**appspot.com/10837114/diff/** > 8004/chrome/test/functional/**perf/endure_setup.py<https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functional/perf/endure_setup.py> > File chrome/test/functional/perf/**endure_setup.py (right): > > https://chromiumcodereview.**appspot.com/10837114/diff/** > 8004/chrome/test/functional/**perf/endure_setup.py#newcode59<https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functional/perf/endure_setup.py#newcode59> > chrome/test/functional/perf/**endure_setup.py:59: ' Fetch the latest > version of Chrome Endure to /home/user/endure_dir.\n') > On 2012/08/17 00:55:03, dennis_jeffrey wrote: > >> It might be easier to format the above within """ since that way you >> > don't need > >> to explicitly have all the \n's and stuff: >> > > DESCRIPTION = """ >> Fetch Chrome Endure. >> > > Usage: >> python endure_setup.py [options] >> ... >> """ >> > > Done. > > > https://chromiumcodereview.**appspot.com/10837114/diff/** > 8004/chrome/test/functional/**perf/endure_setup.py#newcode86<https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functional/perf/endure_setup.py#newcode86> > chrome/test/functional/perf/**endure_setup.py:86: description=DESCRIPTION) > On 2012/08/17 00:55:03, dennis_jeffrey wrote: > >> probably can move this to the previous line >> > > Done. > > > https://chromiumcodereview.**appspot.com/10837114/diff/** > 8004/chrome/test/functional/**perf/endure_setup.py#**newcode120<https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functional/perf/endure_setup.py#newcode120> > chrome/test/functional/perf/**endure_setup.py:120: # TODO(fdeng): remove > this after it is check into the chrome tree. > On 2012/08/17 00:55:03, dennis_jeffrey wrote: > >> check --> checked >> > > Done. > > > https://chromiumcodereview.**appspot.com/10837114/diff/** > 8004/chrome/test/functional/**perf/endure_setup.py#**newcode176<https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functional/perf/endure_setup.py#newcode176> > chrome/test/functional/perf/**endure_setup.py:176: # gclient config > On 2012/08/17 00:55:03, dennis_jeffrey wrote: > >> probably don't need this comment. we can see in the next line below >> > that you're > >> running gclient config >> > > Done. > > > https://chromiumcodereview.**appspot.com/10837114/diff/** > 8004/chrome/test/functional/**perf/endure_setup.py#**newcode180<https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functional/perf/endure_setup.py#newcode180> > chrome/test/functional/perf/**endure_setup.py:180: # gclient sync > On 2012/08/17 00:55:03, dennis_jeffrey wrote: > >> similar comment as line 176 above >> > > Done. > > > https://chromiumcodereview.**appspot.com/10837114/diff/** > 8004/chrome/test/functional/**perf/endure_setup.py#**newcode193<https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functional/perf/endure_setup.py#newcode193> > chrome/test/functional/perf/**endure_setup.py:193: endure_dir: Direcotry > of Chrome Endure and its dependencies. > On 2012/08/17 00:55:03, dennis_jeffrey wrote: > >> Direcotry --> Directory >> > > Done. > > > https://chromiumcodereview.**appspot.com/10837114/diff/** > 8004/chrome/test/functional/**perf/endure_setup.py#**newcode230<https://chromiumcodereview.appspot.com/10837114/diff/8004/chrome/test/functional/perf/endure_setup.py#newcode230> > chrome/test/functional/perf/**endure_setup.py:230: logging.info('Binaries > at revision %s', revision) > On 2012/08/17 00:55:03, dennis_jeffrey wrote: > >> before logging this, should we also make sure that binary_dir exists? >> > > Done. > > https://chromiumcodereview.**appspot.com/10837114/<https://chromiumcodereview... >
LGTM Yes, let's show this to Loreena and see if it works for her, and what feedback she has. Also, yes, it's a good idea to update the documentation at go/endure for this script. But let's wait until we check it in first (maybe we'll make some changes based on any feedback from Loreena before we check this in). I agree it might be helpful to keep the "manual setup" instructions around. But let's not highlight it in the left-hand menu of the site, since we should steer everyone to the automated script first. Maybe we can just have a small link to the manual setup steps from the automated setup page only. That way people know there are back-up manual steps just in case they need them.
I think this should be checked in. Loreena's feedback (if any) can be accommodated in follow up CLs. Let's get to the habit of committing in small chunks and committing often. Passing around files is difficult to maintain. Consumers can checkout from the repo directly.
On 2012/08/17 18:34:05, Nirnimesh wrote: > I think this should be checked in. Loreena's feedback (if any) can be > accommodated in follow up CLs. Let's get to the habit of committing in small > chunks and committing often. Passing around files is difficult to maintain. > Consumers can checkout from the repo directly. Sounds good - I'm ok with checking in the script now. Fang, go ahead!
Ok thank you! I'll write a email to Loreena later. Fang On Fri, Aug 17, 2012 at 11:37 AM, <dennisjeffrey@chromium.org> wrote: > > On 2012/08/17 18:34:05, Nirnimesh wrote: > >> I think this should be checked in. Loreena's feedback (if any) can be >> accommodated in follow up CLs. Let's get to the habit of committing in >> small >> chunks and committing often. Passing around files is difficult to >> maintain. >> Consumers can checkout from the repo directly. >> > > Sounds good - I'm ok with checking in the script now. Fang, go ahead! > > https://chromiumcodereview.**appspot.com/10837114/<https://chromiumcodereview... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fdeng@chromium.org/10837114/4
Change committed as 152132
On 2012/08/17 19:31:55, I haz the power (commit-bot) wrote: > Change committed as 152132 This change causes check_perms failures, which would have been caught by trybots. Why did you use NOTRY? That should never be used.
On Fri, Aug 17, 2012 at 1:56 PM, <akalin@chromium.org> wrote: > On 2012/08/17 19:31:55, I haz the power (commit-bot) wrote: > >> Change committed as 152132 >> > > This change causes check_perms failures, which would have been caught by > trybots. Why did you use NOTRY? That should never be used. > > That was my fault. I typically use NOTRY when submitting python test changes that shouldn't affect how chrome builds or how tests on the try bots run. I thought that should be fine here, but didn't realize that permissions checks are performed on files that may end up closing the tree (and this CL includes new files). Is this something that could be caught at the presubmit stage rather than during try bot runs? > https://chromiumcodereview.**appspot.com/10837114/<https://chromiumcodereview... >
For python changes, we use NOTRY=true, because otherwise it's a waste of time and resources. afaik, trybots do not check_perms so it would not have helped. For such cases, sheriffs typically give a leeway of 30 mins for the CL author to fix perms, and revert only when the author doesn't fix in the time. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
