Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11741030:
Add telemetry_bootstrap module (Closed)
|
Created:
7 years, 11 months ago by wiltzius Modified:
7 years, 11 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, pam+watch_chromium.org, telemetry+watch_chromium.org, dtu, sbasi1 Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdds a telemetry_bootstrap module that can fetch files from SVN or other WebDAV servers.
Also adds a complemetary DEPS file, in a subset of the gclient DEPS format, that specifies what files are needed for telemetry.
third_party/davclient is required to make interfacing with a WebDAV server simpler.
BUG=162301
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176078
Patch Set 1 #
Total comments: 37
Patch Set 2 : #Patch Set 3 : #
Total comments: 20
Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... File tools/telemetry/telemetry_bootstrap.py (right): https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:23: from optparse import OptionParser I think you can ditch main() entirely and thus OptionParser. https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:29: DEPS_FILE = "DEPS" This can go away https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:30: # link to file containing the 'davclient' WebDAV client library # Comments start with capitals end and end with a period. ^ here and elsewhere https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:31: #TODO(wiltzius) change this to point at Chromium SVN server after checkin # TODO(wiltzius): https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:32: DAVCLIENT_URL = 'http://svn.osafoundation.org/tools/davclient/trunk/src/davclient/davclient.py' _DAVCLIENT_URL since its private to the module. _-prefix all the methods and vars in this file that aren't intended to be visible to users of the file. https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:34: def bootstrap_davclient(): _bootstrap_davclient since this shouldn't be called by users of the module https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:35: """Dynamically import davclient helper library.""" _download_and_import_davclient_module()? https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:37: davclient_txt = urllib.urlopen(DAVCLIENT_URL).read() txt -> source? https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:93: def ParseOptions(): I think this can go away https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:104: def DownloadDEPS(deps_path): Should we have a TODO here for what revision of telemetry to pull? https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:110: exec deps_file.read() in deps.__dict__ unindent the exec stuff so you say with blah as blah: deps_contents = deps_file.read() deps = ... exec deps_contents in deps.__dict() https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:111: #TODO(wiltzius) in the future, make the destination directory configurable formatting https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:112: # as something other than the cwd this function should take in destination dir. E.g. the caller of it should give the destination directory, which could be cwd, but not done here. https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:116: root_url = parsed_url.scheme + '://' + parsed_url.netloc where will we handle "foo@181" type stuff? https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:119: os.path.join(destination_dir, dst_path)) after we clone dst_path, what if dst_path has a DEPS file? https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:122: def Main(): not needed I think. https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:131: if __name__ == "__main__": Ditch the __name__ bit as well as the #! bit. If someone wants to run this directly, we should add a script up one level. Not clear if it makes sense, anyway. https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:132: Main() Lets add the stuff that you had in __main__ into tools/perf/run_multipage_benchmark --- eg that should have the script to download telemetry_bootstrap.py https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_boo... tools/telemetry/telemetry_bootstrap.py:132: Main() Lets move this to tools/telemetry/tools/telemetry_bootstrap.py --- that will ensure that this can't reference any parts of telemetry itself and vice versa.
For future reference, you can use "git cl issue <ISSUE NUMBER>" to set the codereview issue number if you lose your checkout. On Fri, Jan 4, 2013 at 5:28 PM, <nduca@chromium.org> wrote: > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py> > File tools/telemetry/telemetry_**bootstrap.py (right): > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode23<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode23> > tools/telemetry/telemetry_**bootstrap.py:23: from optparse import > OptionParser > I think you can ditch main() entirely and thus OptionParser. > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode29<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode29> > tools/telemetry/telemetry_**bootstrap.py:29: DEPS_FILE = "DEPS" > This can go away > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode30<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode30> > tools/telemetry/telemetry_**bootstrap.py:30: # link to file containing the > 'davclient' WebDAV client library > # Comments start with capitals end and end with a period. > > ^ here and elsewhere > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode31<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode31> > tools/telemetry/telemetry_**bootstrap.py:31: #TODO(wiltzius) change this > to point at Chromium SVN server after checkin > # TODO(wiltzius): > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode32<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode32> > tools/telemetry/telemetry_**bootstrap.py:32: DAVCLIENT_URL = > 'http://svn.osafoundation.org/**tools/davclient/trunk/src/** > davclient/davclient.py<http://svn.osafoundation.org/tools/davclient/trunk/src/davclient/davclient.py> > ' > _DAVCLIENT_URL since its private to the module. _-prefix all the methods > and vars in this file that aren't intended to be visible to users of the > file. > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode34<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode34> > tools/telemetry/telemetry_**bootstrap.py:34: def bootstrap_davclient(): > _bootstrap_davclient since this shouldn't be called by users of the > module > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode35<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode35> > tools/telemetry/telemetry_**bootstrap.py:35: """Dynamically import > davclient helper library.""" > _download_and_import_**davclient_module()? > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode37<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode37> > tools/telemetry/telemetry_**bootstrap.py:37: davclient_txt = > urllib.urlopen(DAVCLIENT_URL).**read() > txt -> source? > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode93<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode93> > tools/telemetry/telemetry_**bootstrap.py:93: def ParseOptions(): > I think this can go away > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode104<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode104> > tools/telemetry/telemetry_**bootstrap.py:104: def DownloadDEPS(deps_path): > Should we have a TODO here for what revision of telemetry to pull? > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode110<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode110> > tools/telemetry/telemetry_**bootstrap.py:110: exec deps_file.read() in > deps.__dict__ > unindent the exec stuff > > so you say > > with blah as blah: > deps_contents = deps_file.read() > > deps = ... > exec deps_contents in deps.__dict() > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode111<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode111> > tools/telemetry/telemetry_**bootstrap.py:111: #TODO(wiltzius) in the > future, make the destination directory configurable > formatting > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode112<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode112> > tools/telemetry/telemetry_**bootstrap.py:112: # as something other than > the cwd > this function should take in destination dir. E.g. the caller of it > should give the destination directory, which could be cwd, but not done > here. > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode116<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode116> > tools/telemetry/telemetry_**bootstrap.py:116: root_url = parsed_url.scheme > + '://' + parsed_url.netloc > where will we handle "foo@181" type stuff? > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode119<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode119> > tools/telemetry/telemetry_**bootstrap.py:119: > os.path.join(destination_dir, dst_path)) > after we clone dst_path, what if dst_path has a DEPS file? > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode122<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode122> > tools/telemetry/telemetry_**bootstrap.py:122: def Main(): > not needed I think. > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode131<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode131> > tools/telemetry/telemetry_**bootstrap.py:131: if __name__ == "__main__": > Ditch the __name__ bit as well as the #! bit. If someone wants to run > this directly, we should add a script up one level. Not clear if it > makes sense, anyway. > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode132<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode132> > tools/telemetry/telemetry_**bootstrap.py:132: Main() > Lets add the stuff that you had in __main__ into > tools/perf/run_multipage_**benchmark --- eg that should have the script to > download telemetry_bootstrap.py > > https://codereview.chromium.**org/11741030/diff/1/tools/** > telemetry/telemetry_bootstrap.**py#newcode132<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode132> > tools/telemetry/telemetry_**bootstrap.py:132: Main() > Lets move this to tools/telemetry/tools/**telemetry_bootstrap.py --- that > will ensure that this can't reference any parts of telemetry itself and > vice versa. > > https://codereview.chromium.**org/11741030/<https://codereview.chromium.org/1... >
Partial patch up; don't review yet still need to make the DEPS recursive. On 2013/01/05 01:44:56, tonyg wrote: > For future reference, you can use "git cl issue <ISSUE NUMBER>" to set the > codereview issue number if you lose your checkout. > > > On Fri, Jan 4, 2013 at 5:28 PM, <mailto:nduca@chromium.org> wrote: > > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py> > > File tools/telemetry/telemetry_**bootstrap.py (right): > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode23<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode23> > > tools/telemetry/telemetry_**bootstrap.py:23: from optparse import > > OptionParser > > I think you can ditch main() entirely and thus OptionParser. > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode29<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode29> > > tools/telemetry/telemetry_**bootstrap.py:29: DEPS_FILE = "DEPS" > > This can go away > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode30<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode30> > > tools/telemetry/telemetry_**bootstrap.py:30: # link to file containing the > > 'davclient' WebDAV client library > > # Comments start with capitals end and end with a period. > > > > ^ here and elsewhere > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode31<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode31> > > tools/telemetry/telemetry_**bootstrap.py:31: #TODO(wiltzius) change this > > to point at Chromium SVN server after checkin > > # TODO(wiltzius): > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode32<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode32> > > tools/telemetry/telemetry_**bootstrap.py:32: DAVCLIENT_URL = > > 'http://svn.osafoundation.org/**tools/davclient/trunk/src/** > > > davclient/davclient.py<http://svn.osafoundation.org/tools/davclient/trunk/src/davclient/davclient.py> > > ' > > _DAVCLIENT_URL since its private to the module. _-prefix all the methods > > and vars in this file that aren't intended to be visible to users of the > > file. > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode34<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode34> > > tools/telemetry/telemetry_**bootstrap.py:34: def bootstrap_davclient(): > > _bootstrap_davclient since this shouldn't be called by users of the > > module > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode35<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode35> > > tools/telemetry/telemetry_**bootstrap.py:35: """Dynamically import > > davclient helper library.""" > > _download_and_import_**davclient_module()? > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode37<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode37> > > tools/telemetry/telemetry_**bootstrap.py:37: davclient_txt = > > urllib.urlopen(DAVCLIENT_URL).**read() > > txt -> source? > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode93<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode93> > > tools/telemetry/telemetry_**bootstrap.py:93: def ParseOptions(): > > I think this can go away > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode104<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode104> > > tools/telemetry/telemetry_**bootstrap.py:104: def DownloadDEPS(deps_path): > > Should we have a TODO here for what revision of telemetry to pull? > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode110<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode110> > > tools/telemetry/telemetry_**bootstrap.py:110: exec deps_file.read() in > > deps.__dict__ > > unindent the exec stuff > > > > so you say > > > > with blah as blah: > > deps_contents = deps_file.read() > > > > deps = ... > > exec deps_contents in deps.__dict() > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode111<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode111> > > tools/telemetry/telemetry_**bootstrap.py:111: #TODO(wiltzius) in the > > future, make the destination directory configurable > > formatting > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode112<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode112> > > tools/telemetry/telemetry_**bootstrap.py:112: # as something other than > > the cwd > > this function should take in destination dir. E.g. the caller of it > > should give the destination directory, which could be cwd, but not done > > here. > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode116<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode116> > > tools/telemetry/telemetry_**bootstrap.py:116: root_url = parsed_url.scheme > > + '://' + parsed_url.netloc > > where will we handle "foo@181" type stuff? > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode119<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode119> > > tools/telemetry/telemetry_**bootstrap.py:119: > > os.path.join(destination_dir, dst_path)) > > after we clone dst_path, what if dst_path has a DEPS file? > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode122<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode122> > > tools/telemetry/telemetry_**bootstrap.py:122: def Main(): > > not needed I think. > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode131<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode131> > > tools/telemetry/telemetry_**bootstrap.py:131: if __name__ == "__main__": > > Ditch the __name__ bit as well as the #! bit. If someone wants to run > > this directly, we should add a script up one level. Not clear if it > > makes sense, anyway. > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode132<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode132> > > tools/telemetry/telemetry_**bootstrap.py:132: Main() > > Lets add the stuff that you had in __main__ into > > tools/perf/run_multipage_**benchmark --- eg that should have the script to > > download telemetry_bootstrap.py > > > > https://codereview.chromium.**org/11741030/diff/1/tools/** > > > telemetry/telemetry_bootstrap.**py#newcode132<https://codereview.chromium.org/11741030/diff/1/tools/telemetry/telemetry_bootstrap.py#newcode132> > > tools/telemetry/telemetry_**bootstrap.py:132: Main() > > Lets move this to tools/telemetry/tools/**telemetry_bootstrap.py --- that > > will ensure that this can't reference any parts of telemetry itself and > > vice versa. > > > > > https://codereview.chromium.**org/11741030/%3Chttps://codereview.chromium.org...> > >
Partial patch https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... File tools/telemetry/telemetry_bootstrap.py (right): https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:23: from optparse import OptionParser On 2013/01/05 01:28:19, nduca wrote: > I think you can ditch main() entirely and thus OptionParser. Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:29: DEPS_FILE = "DEPS" On 2013/01/05 01:28:19, nduca wrote: > This can go away Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:29: DEPS_FILE = "DEPS" On 2013/01/05 01:28:19, nduca wrote: > This can go away Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:30: # link to file containing the 'davclient' WebDAV client library On 2013/01/05 01:28:19, nduca wrote: > # Comments start with capitals end and end with a period. > > ^ here and elsewhere Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:32: DAVCLIENT_URL = 'http://svn.osafoundation.org/tools/davclient/trunk/src/davclient/davclient.py' On 2013/01/05 01:28:19, nduca wrote: > _DAVCLIENT_URL since its private to the module. _-prefix all the methods and > vars in this file that aren't intended to be visible to users of the file. Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:34: def bootstrap_davclient(): On 2013/01/05 01:28:19, nduca wrote: > _bootstrap_davclient since this shouldn't be called by users of the module Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:35: """Dynamically import davclient helper library.""" On 2013/01/05 01:28:19, nduca wrote: > _download_and_import_davclient_module()? Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:37: davclient_txt = urllib.urlopen(DAVCLIENT_URL).read() On 2013/01/05 01:28:19, nduca wrote: > txt -> source? Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:37: davclient_txt = urllib.urlopen(DAVCLIENT_URL).read() On 2013/01/05 01:28:19, nduca wrote: > txt -> source? Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:93: def ParseOptions(): On 2013/01/05 01:28:19, nduca wrote: > I think this can go away Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:104: def DownloadDEPS(deps_path): On 2013/01/05 01:28:19, nduca wrote: > Should we have a TODO here for what revision of telemetry to pull? Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:110: exec deps_file.read() in deps.__dict__ On 2013/01/05 01:28:19, nduca wrote: > unindent the exec stuff > > so you say > > with blah as blah: > deps_contents = deps_file.read() > > deps = ... > exec deps_contents in deps.__dict() Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:111: #TODO(wiltzius) in the future, make the destination directory configurable On 2013/01/05 01:28:19, nduca wrote: > formatting Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:112: # as something other than the cwd On 2013/01/05 01:28:19, nduca wrote: > this function should take in destination dir. E.g. the caller of it should give > the destination directory, which could be cwd, but not done here. Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:116: root_url = parsed_url.scheme + '://' + parsed_url.netloc That's the "fetch at revision" TODO. This will need to go here somehow, but will require changes to the DAVClient wrapper too. On 2013/01/05 01:28:19, nduca wrote: > where will we handle "foo@181" type stuff? https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:122: def Main(): On 2013/01/05 01:28:19, nduca wrote: > not needed I think. Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:131: if __name__ == "__main__": On 2013/01/05 01:28:19, nduca wrote: > Ditch the __name__ bit as well as the #! bit. If someone wants to run this > directly, we should add a script up one level. Not clear if it makes sense, > anyway. Done. https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... tools/telemetry/telemetry_bootstrap.py:132: Main() I will do this in a follow-up patch after this bootstrap lands, don't want to risk messing up run_multipage_benchmark before I'm sure this script works :) On 2013/01/05 01:28:19, nduca wrote: > Lets add the stuff that you had in __main__ into > tools/perf/run_multipage_benchmark --- eg that should have the script to > download telemetry_bootstrap.py
All changes now addressed (a couple remaining TODOs but I don't believe they're necessary for the first landing). Please take another look, On 2013/01/08 02:53:34, wiltzius wrote: > Partial patch > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > File tools/telemetry/telemetry_bootstrap.py (right): > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:23: from optparse import OptionParser > On 2013/01/05 01:28:19, nduca wrote: > > I think you can ditch main() entirely and thus OptionParser. > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:29: DEPS_FILE = "DEPS" > On 2013/01/05 01:28:19, nduca wrote: > > This can go away > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:29: DEPS_FILE = "DEPS" > On 2013/01/05 01:28:19, nduca wrote: > > This can go away > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:30: # link to file containing the > 'davclient' WebDAV client library > On 2013/01/05 01:28:19, nduca wrote: > > # Comments start with capitals end and end with a period. > > > > ^ here and elsewhere > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:32: DAVCLIENT_URL = > 'http://svn.osafoundation.org/tools/davclient/trunk/src/davclient/davclient.py' > On 2013/01/05 01:28:19, nduca wrote: > > _DAVCLIENT_URL since its private to the module. _-prefix all the methods and > > vars in this file that aren't intended to be visible to users of the file. > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:34: def bootstrap_davclient(): > On 2013/01/05 01:28:19, nduca wrote: > > _bootstrap_davclient since this shouldn't be called by users of the module > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:35: """Dynamically import davclient > helper library.""" > On 2013/01/05 01:28:19, nduca wrote: > > _download_and_import_davclient_module()? > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:37: davclient_txt = > urllib.urlopen(DAVCLIENT_URL).read() > On 2013/01/05 01:28:19, nduca wrote: > > txt -> source? > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:37: davclient_txt = > urllib.urlopen(DAVCLIENT_URL).read() > On 2013/01/05 01:28:19, nduca wrote: > > txt -> source? > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:93: def ParseOptions(): > On 2013/01/05 01:28:19, nduca wrote: > > I think this can go away > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:104: def DownloadDEPS(deps_path): > On 2013/01/05 01:28:19, nduca wrote: > > Should we have a TODO here for what revision of telemetry to pull? > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:110: exec deps_file.read() in > deps.__dict__ > On 2013/01/05 01:28:19, nduca wrote: > > unindent the exec stuff > > > > so you say > > > > with blah as blah: > > deps_contents = deps_file.read() > > > > deps = ... > > exec deps_contents in deps.__dict() > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:111: #TODO(wiltzius) in the future, make > the destination directory configurable > On 2013/01/05 01:28:19, nduca wrote: > > formatting > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:112: # as something other than the cwd > On 2013/01/05 01:28:19, nduca wrote: > > this function should take in destination dir. E.g. the caller of it should > give > > the destination directory, which could be cwd, but not done here. > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:116: root_url = parsed_url.scheme + '://' > + parsed_url.netloc > That's the "fetch at revision" TODO. This will need to go here somehow, but will > require changes to the DAVClient wrapper too. > > On 2013/01/05 01:28:19, nduca wrote: > > where will we handle "foo@181" type stuff? > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:122: def Main(): > On 2013/01/05 01:28:19, nduca wrote: > > not needed I think. > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:131: if __name__ == "__main__": > On 2013/01/05 01:28:19, nduca wrote: > > Ditch the __name__ bit as well as the #! bit. If someone wants to run this > > directly, we should add a script up one level. Not clear if it makes sense, > > anyway. > > Done. > > https://chromiumcodereview.appspot.com/11741030/diff/1/tools/telemetry/teleme... > tools/telemetry/telemetry_bootstrap.py:132: Main() > I will do this in a follow-up patch after this bootstrap lands, don't want to > risk messing up run_multipage_benchmark before I'm sure this script works :) > > On 2013/01/05 01:28:19, nduca wrote: > > Lets add the stuff that you had in __main__ into > > tools/perf/run_multipage_benchmark --- eg that should have the script to > > download telemetry_bootstrap.py
lgtm with nits https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/DEPS File tools/telemetry/DEPS (right): https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/DE... tools/telemetry/DEPS:13: "https://src.chromium.org/chrome/trunk/src/build/android", Probably not relevant for this patchset, but we're going to need to come up with a place to dump adb and android forwarder2 binaries... https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... File tools/telemetry/tools/telemetry_bootstrap.py (right): https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:12: DEPS can be specified with --deps or defaults to ./DEPS no longer required, given removal of main() https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:90: destination_dir=os.path.join(os.getcwd(),'telemetry')): just require it to be provided. [also, probably better to make the folder not-relative to cwd --- e.g. if i run ./tools/perf/run_multipage_benchmarks, you'd not get the same result as cd tools/perf; ./run_mbpr. a solution for this is using os.path.dirname(__FILE__) which gives you the dirname of the current file, e.g. the location of *this* file --- but you'd want to do this in the run_multipage_benchmark script rather than here. Anyhooo --- for now, make the arguments mandatory. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:103: logging.basicConfig(level=logging.INFO) this shouldn't be here https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:104: # Dynamically import davclient library. this comment isnt adding much value, i think https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:106: with open(deps_path) as deps_file: space above this --- and the comment might be more like # LOAD deps file https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:108: deps = imp.new_module('deps') i think some whitespace between logical blocks may make this more readable one above the deps= line https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:110: for dst_path, src_path in deps.deps.iteritems(): newline above this line https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:114: dav_client = DAVClientWrapper(root_url) newline above this line https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:116: # Recursively fetch any DEPS defined in a subdirectory we just fetched. newline above this line
New patch fixing nits https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/DEPS File tools/telemetry/DEPS (right): https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/DE... tools/telemetry/DEPS:13: "https://src.chromium.org/chrome/trunk/src/build/android", Yes, but probably not relevant for the base bootstrap. I'm happy to update this DEPS file once we have those checked in somewhere. On 2013/01/09 11:19:57, nduca wrote: > Probably not relevant for this patchset, but we're going to need to come up with > a place to dump adb and android forwarder2 binaries... https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... File tools/telemetry/tools/telemetry_bootstrap.py (right): https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:12: DEPS can be specified with --deps or defaults to ./DEPS On 2013/01/09 11:19:57, nduca wrote: > no longer required, given removal of main() Done. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:90: destination_dir=os.path.join(os.getcwd(),'telemetry')): On 2013/01/09 11:19:57, nduca wrote: > just require it to be provided. > > [also, probably better to make the folder not-relative to cwd --- e.g. if i run > ./tools/perf/run_multipage_benchmarks, you'd not get the same result as cd > tools/perf; ./run_mbpr. a solution for this is using os.path.dirname(__FILE__) > which gives you the dirname of the current file, e.g. the location of *this* > file --- but you'd want to do this in the run_multipage_benchmark script rather > than here. Anyhooo --- for now, make the arguments mandatory. Done. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:103: logging.basicConfig(level=logging.INFO) On 2013/01/09 11:19:57, nduca wrote: > this shouldn't be here Done. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:104: # Dynamically import davclient library. On 2013/01/09 11:19:57, nduca wrote: > this comment isnt adding much value, i think Done. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:106: with open(deps_path) as deps_file: On 2013/01/09 11:19:57, nduca wrote: > space above this --- and the comment might be more like # LOAD deps file Done. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:108: deps = imp.new_module('deps') On 2013/01/09 11:19:57, nduca wrote: > i think some whitespace between logical blocks may make this more readable > > one above the deps= line Done. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:110: for dst_path, src_path in deps.deps.iteritems(): On 2013/01/09 11:19:57, nduca wrote: > newline above this line Done. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:114: dav_client = DAVClientWrapper(root_url) On 2013/01/09 11:19:57, nduca wrote: > newline above this line Done. https://chromiumcodereview.appspot.com/11741030/diff/11001/tools/telemetry/to... tools/telemetry/tools/telemetry_bootstrap.py:116: # Recursively fetch any DEPS defined in a subdirectory we just fetched. On 2013/01/09 11:19:57, nduca wrote: > newline above this line Done.
Fantastic. Land away.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wiltzius@chromium.org/11741030/17001
Presubmit check for 11741030-17001 failed and returned exit status 1. ************* Module telemetry_bootstrap C0301: 26,0: Line too long (96/80) W0601: 30,2:_download_and_import_davclient_module: Global variable 'davclient' undefined at the module level W0612:116,17:DownloadDEPS: Unused variable 'dirnames' Running presubmit commit checks ... Running pylint on 100 files. ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit ERRORS ** Fix pylint errors first. Presubmit checks took 5.7s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wiltzius@chromium.org/11741030/17001
Presubmit check for 11741030-17001 failed and returned exit status 1. ************* Module telemetry_bootstrap C0301: 26,0: Line too long (96/80) W0601: 30,2:_download_and_import_davclient_module: Global variable 'davclient' undefined at the module level W0612:116,17:DownloadDEPS: Unused variable 'dirnames' Running presubmit commit checks ... Running pylint on 100 files. ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit ERRORS ** Fix pylint errors first. Presubmit checks took 6.0s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wiltzius@chromium.org/11741030/19003
Retried try job too often on win_aura for step(s) content_browsertests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wiltzius@chromium.org/11741030/19003
Message was sent while issue was closed.
Change committed as 176078 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
