Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
548233002:
Add a module to fetch builds from different types of builders. (Closed)
|
Created:
6 years, 3 months ago by qyearsley Modified:
6 years, 2 months ago Reviewers:
prasadv, szager1, ghost stip (do not use), eseidel, Paweł Hajdan Jr., agable, Sergiy Byelozyorov, tonyg, ojan CC:
ojan, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd a module to fetch builds from different types of builders.
This module and its contents will replace the following functions and methods
in bisect-perf-regression.py:
Most of DownloadCurrentBuild
GetBuildArchiveForRevision
GetRemoteBuildPath
GetGSRootFolderName
GetZipFileName
FetchFromCloudStorage
ExtractZip
MaybeMakeDirectory
In this module, we want to be able to support downloading from different
places with possibly different naming conventions in a clean way.
As-is, this module works for downloading and extracting full builds that are
archived by the builders on the main waterfall. Example invocation:
$ python fetch_build.py full 9f11d3bc7c31cc4c10efa15e4bfddf12705908ed outdir
BUG=402669
Committed: https://crrev.com/0d9779113ad21ce858abdaaefe240bab950e435a
Cr-Commit-Position: refs/heads/master@{#297669}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Respond to comments. #
Total comments: 24
Patch Set 3 : Updated comments #
Total comments: 22
Patch Set 4 : Respond to comments (including: use dicts to select bucket/dirname) #Patch Set 5 : #Patch Set 6 : Remove mock_wrapper (mock should be in sys.path when run by builder) #Patch Set 7 : mock should also be PYTHONPATH on builder; import mock directly and add it to PYTHONPATH for run_te… #Patch Set 8 : Rebased #
Total comments: 4
Patch Set 9 : Remove print in run_tests and move changes to sys.path into main. #
Messages
Total messages: 26 (4 generated)
qyearsley@chromium.org changed reviewers: + prasadv@chromium.org, tonyg@chromium.org
Tony and Prasad PTAL and give advice :-) Ojan, could you also take a look and give advice, e.g. on parts where comments or names could be made clearer?
qyearsley@chromium.org changed reviewers: + sergiyb@chromium.org
On 2014/09/08 05:53:21, qyearsley wrote: > Tony and Prasad PTAL and give advice :-) > > Ojan, could you also take a look and give advice, e.g. on parts where comments > or names could be made clearer? Friendly ping
https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... File tools/auto_bisect/fetch_build.py (right): https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:39: revision: Revision string, e.g. a git commit hash. Before Git migration builds were stored as SVN revisions, so it can be git commit hash or svn revision. https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:47: The directory path where the build has been extracted to. What will this function return in case of any failure? https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:53: filename = FetchFromCloudStorage(bucket, remote_path, output_dir) Shouldn't we validate return values from FetchFromCloudStorage and Unzip? FetchFromCloudStorage returns None when file is not found. https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:54: Unzip(filename, output_dir) This function should return directory path right? https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:158: def _Directory(self): s/_Directory/_ArchiveDirectory? https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:302: parser.add_argument('--target-arch', default='x64') I think default should be ia32.
https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... File tools/auto_bisect/fetch_build.py (right): https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:39: revision: Revision string, e.g. a git commit hash. On 2014/09/11 17:04:10, prasadv wrote: > Before Git migration builds were stored as SVN revisions, so it can be git > commit hash or svn revision. Right, comment updated. https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:47: The directory path where the build has been extracted to. On 2014/09/11 17:04:10, prasadv wrote: > What will this function return in case of any failure? It could return None, but another approach would be to raise an exception. Then the caller who's trying to get the build would catch this exception and possibly skip this revision. https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:53: filename = FetchFromCloudStorage(bucket, remote_path, output_dir) On 2014/09/11 17:04:10, prasadv wrote: > Shouldn't we validate return values from FetchFromCloudStorage and Unzip? > FetchFromCloudStorage returns None when file is not found. Good point. In the case of Unzip, it may raise various types of exceptions (even ones that aren't explicitly listed. For example, zipfile.BadZipfile or KeyError could be raised. If we're using exceptions to indicate failure of FetchBuild, then we don't need to check here, we can just let the exception be raised and caught further up the call stack. https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:54: Unzip(filename, output_dir) On 2014/09/11 17:04:10, prasadv wrote: > This function should return directory path right? That's what I was thinking, but the directory path is always going to be output_dir here, no need to return it I think. https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:158: def _Directory(self): On 2014/09/11 17:04:10, prasadv wrote: > s/_Directory/_ArchiveDirectory? Done. https://codereview.chromium.org/548233002/diff/1/tools/auto_bisect/fetch_buil... tools/auto_bisect/fetch_build.py:302: parser.add_argument('--target-arch', default='x64') On 2014/09/11 17:04:10, prasadv wrote: > I think default should be ia32. Done.
+some infra folks who may be able to suggest a more general way to map to a google storage bucket I don't have strong opinions about the module naming. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/bisect... File tools/auto_bisect/bisect_utils.py (right): https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/bisect... tools/auto_bisect/bisect_utils.py:544: """Checks whether or not the script is running on Windows.""" I would remove these comments entirely. They're saying what the function name clearly says. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/bisect... tools/auto_bisect/bisect_utils.py:562: """Checks whether or not the script is running on Linux.""" Ditto https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/bisect... tools/auto_bisect/bisect_utils.py:567: """Checks whether or not the script is running on Mac.""" Ditto https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... File tools/auto_bisect/fetch_build.py (right): https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:7: The builds may be stored in different places by different types builders; types of builders. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:64: # Delete downloaded zip file. That's pretty clearly what the code does. No need for the comment. Use comments for high-level documentation or to explain non-obvious things. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:80: """Creates a BuildArchive instance for fetching builds of some type.""" Ditto https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:92: """Returns the Google Cloud Storage bucket name to download from.""" Ditto https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:143: """Provides information about where perf builds are stored.""" I'll stop commenting about removing comments...but most of the ones in this file just say what the function name clearly does. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:191: if bisect_utils.Is64BitWindows() and self._target_arch == 'x64': This sort of manual mapping isn't going to work long-term. We have dozens of configurations on the main waterfall. We need a more generic solution. I don't have a good suggestion for how this should work though. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... File tools/auto_bisect/fetch_build_test.py (right): https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:20: """Test case for functions related to fetching builds.""" Useless comment. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:49: """Test case for the BuildArchive class and subclasses.""" Ditto https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:137: """Test case for the Unzip function and subsidiary functions.""" You guessed it. :)
Thanks for the review :-) I think I probably have an tendency to over-comment, because I was trying to add a docstring to every function and every class. Anyway, "use comments for high-level documentation or to explain non-obvious things" seems like a good guideline. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/bisect... File tools/auto_bisect/bisect_utils.py (right): https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/bisect... tools/auto_bisect/bisect_utils.py:544: """Checks whether or not the script is running on Windows.""" On 2014/09/12 00:36:27, ojan-only-code-yellow-reviews wrote: > I would remove these comments entirely. They're saying what the function name > clearly says. Fair enough :-) https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/bisect... tools/auto_bisect/bisect_utils.py:562: """Checks whether or not the script is running on Linux.""" On 2014/09/12 00:36:27, ojan-only-code-yellow-reviews wrote: > Ditto Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/bisect... tools/auto_bisect/bisect_utils.py:567: """Checks whether or not the script is running on Mac.""" On 2014/09/12 00:36:27, ojan-only-code-yellow-reviews wrote: > Ditto Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... File tools/auto_bisect/fetch_build.py (right): https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:7: The builds may be stored in different places by different types builders; On 2014/09/12 00:36:28, ojan-only-code-yellow-reviews wrote: > types of builders. Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:64: # Delete downloaded zip file. On 2014/09/12 00:36:27, ojan-only-code-yellow-reviews wrote: > That's pretty clearly what the code does. No need for the comment. Use comments > for high-level documentation or to explain non-obvious things. Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:80: """Creates a BuildArchive instance for fetching builds of some type.""" On 2014/09/12 00:36:28, ojan-only-code-yellow-reviews wrote: > Ditto Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:92: """Returns the Google Cloud Storage bucket name to download from.""" On 2014/09/12 00:36:27, ojan-only-code-yellow-reviews wrote: > Ditto Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:143: """Provides information about where perf builds are stored.""" On 2014/09/12 00:36:28, ojan-only-code-yellow-reviews wrote: > I'll stop commenting about removing comments...but most of the ones in this file > just say what the function name clearly does. Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:191: if bisect_utils.Is64BitWindows() and self._target_arch == 'x64': On 2014/09/12 00:36:28, ojan-only-code-yellow-reviews wrote: > This sort of manual mapping isn't going to work long-term. We have dozens of > configurations on the main waterfall. We need a more generic solution. I don't > have a good suggestion for how this should work though. On first thought, the only way I could think of besides a manual mapping would be (1) if there were always a systematic way of naming the directories based on platform. But this may vary with master name, and android doesn't fit in with the others. (2) if there were some way that this code could depend on and call some function somewhere else in the code that determines where the builds are stored. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... File tools/auto_bisect/fetch_build_test.py (right): https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:20: """Test case for functions related to fetching builds.""" On 2014/09/12 00:36:28, ojan-only-code-yellow-reviews wrote: > Useless comment. Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:49: """Test case for the BuildArchive class and subclasses.""" On 2014/09/12 00:36:28, ojan-only-code-yellow-reviews wrote: > Ditto Done. https://codereview.chromium.org/548233002/diff/20001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:137: """Test case for the Unzip function and subsidiary functions.""" On 2014/09/12 00:36:28, ojan-only-code-yellow-reviews wrote: > You guessed it. :) Done.
Hi, does anyone else have any comments about this CL? This CL doesn't yet affect the main bisect script -- I'll make a follow-up CL which uses this in the main bisect script.
generally lg with a few comments. I'd remove the wrappers and ignore pylint for now (we should fix those import warnings in a more systematic way). https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... File tools/auto_bisect/bisect_utils.py (right): https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/bisect_utils.py:533: """Chhecks whether or not Windows is a 64-bit version.""" nit: Checks https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... File tools/auto_bisect/cloud_storage_wrapper.py (right): https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/cloud_storage_wrapper.py:18: from telemetry.util.cloud_storage import * usually importing * is a bad idea. https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... File tools/auto_bisect/fetch_build.py (right): https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/fetch_build.py:50: NotImplementedError: The given builder type was not recognized. probably don't need to have NotImplementedError or RuntimeError here https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/fetch_build.py:182: if bisect_utils.Is64BitWindows() and self._target_arch == 'x64': I recommend making these a separate data structure: full_build_buckets = { 'mac': chromium.mac/Mac Builder', 'win64': 'chromium.win/Win x64 Builder', } data-driven programs ftw! https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/fetch_build.py:216: if os.path.exists(target_file): when would this be false? https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... File tools/auto_bisect/fetch_build_test.py (right): https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/fetch_build_test.py:20: remove blank line https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/fetch_build_test.py:23: sys.stdout = open(os.devnull, 'w') I don't think this is needed -- we want those debugging statements! https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/fetch_build_test.py:25: @mock.patch('fetch_build.os.path.exists') I'm not sure if it makes it easier but I used patcher.start() and patcher.stop() in setUp in https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave.... The decorator is probably fine though. https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/fetch_build_test.py:48: remove blank line https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... File tools/auto_bisect/mock_wrapper.py (right): https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/mock_wrapper.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. this is just a workaround to get pylint to stop complaining? we can probably remove this and cloud_storage_wrapper.py and just fix our pylint invocation
Thanks for the review stip! Made some changes to fetch_build.py, and I plan to try more changes o fetch_build_test.py. https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/bisect... File tools/auto_bisect/bisect_utils.py (right): https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/bisect... tools/auto_bisect/bisect_utils.py:533: """Chhecks whether or not Windows is a 64-bit version.""" On 2014/09/17 00:12:07, stip wrote: > nit: Checks Done. https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/cloud_... File tools/auto_bisect/cloud_storage_wrapper.py (right): https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/cloud_... tools/auto_bisect/cloud_storage_wrapper.py:18: from telemetry.util.cloud_storage import * On 2014/09/17 00:12:08, stip wrote: > usually importing * is a bad idea. Changed this to import just the functions that are being used from cloud_storage: Exists and Get. https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... File tools/auto_bisect/fetch_build.py (right): https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:50: NotImplementedError: The given builder type was not recognized. On 2014/09/17 00:12:08, stip wrote: > probably don't need to have NotImplementedError or RuntimeError here Alright - although those are also exceptions that could be thrown from within this function. https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:182: if bisect_utils.Is64BitWindows() and self._target_arch == 'x64': On 2014/09/17 00:12:08, stip wrote: > I recommend making these a separate data structure: > > full_build_buckets = { > 'mac': chromium.mac/Mac Builder', > 'win64': 'chromium.win/Win x64 Builder', > } > > data-driven programs ftw! Hey, what about this? In this version, a private member variable _platform is set in __init__, which is used as a key for a dictionary of possible platforms to directories or bucket-names. If this is what you had in mind -- do you think it would be better to put these dicts here or put them as globals at the top of the file? https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build.py:216: if os.path.exists(target_file): On 2014/09/17 00:12:08, stip wrote: > when would this be false? Not 100% sure, but if something goes wrong with cloud_storage.Get, it might conceivably return without raising an exception but without having successfully written the file to the filesystem in the right place. Then, this function would return None to indicate that something went wrong. Do you think it might be better to raise an exception? https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... File tools/auto_bisect/fetch_build_test.py (right): https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:20: On 2014/09/17 00:12:08, stip wrote: > remove blank line Google style guide says: "One blank line ... between the class line and the first method." https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank... PEP 8 doesn't say anything explicitly about it, but they use a blank line after the class line in one example. http://legacy.python.org/dev/peps/pep-0008/#blank-lines https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:23: sys.stdout = open(os.devnull, 'w') On 2014/09/17 00:12:08, stip wrote: > I don't think this is needed -- we want those debugging statements! Done. https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:25: @mock.patch('fetch_build.os.path.exists') On 2014/09/17 00:12:08, stip wrote: > I'm not sure if it makes it easier but I used patcher.start() and patcher.stop() > in setUp in > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave.... > The decorator is probably fine though. Using patcher.start() and patcher.stop() looks like it will be a little bit nicer, because it means that I wouldn't need to repeat the decorator over every method name; I plan to try this and upload another patch to this CL. https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/fetch_... tools/auto_bisect/fetch_build_test.py:48: On 2014/09/17 00:12:08, stip wrote: > remove blank line Same as comment above. https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/mock_w... File tools/auto_bisect/mock_wrapper.py (right): https://codereview.chromium.org/548233002/diff/40001/tools/auto_bisect/mock_w... tools/auto_bisect/mock_wrapper.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/09/17 00:12:08, stip wrote: > this is just a workaround to get pylint to stop complaining? we can probably > remove this and cloud_storage_wrapper.py and just fix our pylint invocation This was intended as a way to make the import sections at the top of fetch_build.py and fetch_build_test.py look nicer; otherwise I think I'd have to modify sys.path at the top of those files. Doing it this way, mock can be imported in all of the tests in this directory without having to add a statement modifying sys.path at the top of all of the test files. (I'm not sure if this is the best way to do this though.)
https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... File tools/auto_bisect/mock_wrapper.py (right): https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/mock_wrapper.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/09/18 22:58:59, qyearsley wrote: > On 2014/09/17 00:12:08, stip wrote: > > this is just a workaround to get pylint to stop complaining? we can probably > > remove this and cloud_storage_wrapper.py and just fix our pylint invocation > > This was intended as a way to make the import sections at the top of > fetch_build.py and fetch_build_test.py look nicer; otherwise I think I'd have to > modify sys.path at the top of those files. > > Doing it this way, mock can be imported in all of the tests in this directory > without having to add a statement modifying sys.path at the top of all of the > test files. > > (I'm not sure if this is the best way to do this though.) Stuff will either be run with a proper PYTHONPATH, and if it's not you should look at add_build_paths() in https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/tools.... See it in action in https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave...
On 2014/09/18 23:05:53, stip wrote: > https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... > File tools/auto_bisect/mock_wrapper.py (right): > > https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... > tools/auto_bisect/mock_wrapper.py:1: # Copyright 2014 The Chromium Authors. All > rights reserved. > On 2014/09/18 22:58:59, qyearsley wrote: > > On 2014/09/17 00:12:08, stip wrote: > > > this is just a workaround to get pylint to stop complaining? we can probably > > > remove this and cloud_storage_wrapper.py and just fix our pylint invocation > > > > This was intended as a way to make the import sections at the top of > > fetch_build.py and fetch_build_test.py look nicer; otherwise I think I'd have > to > > modify sys.path at the top of those files. > > > > Doing it this way, mock can be imported in all of the tests in this directory > > without having to add a statement modifying sys.path at the top of all of the > > test files. > > > > (I'm not sure if this is the best way to do this though.) > > Stuff will either be run with a proper PYTHONPATH, and if it's not you should > look at add_build_paths() in > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/tools.... > See it in action in > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... http://xkcd.com/927/ have you talked with hinoka@ and dnj@ about our efforts to standardize the various way builders upload and download artifacts from google storage?
https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... File tools/auto_bisect/mock_wrapper.py (right): https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... tools/auto_bisect/mock_wrapper.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2014/09/18 23:05:52, stip wrote: > On 2014/09/18 22:58:59, qyearsley wrote: > > On 2014/09/17 00:12:08, stip wrote: > > > this is just a workaround to get pylint to stop complaining? we can probably > > > remove this and cloud_storage_wrapper.py and just fix our pylint invocation > > > > This was intended as a way to make the import sections at the top of > > fetch_build.py and fetch_build_test.py look nicer; otherwise I think I'd have > to > > modify sys.path at the top of those files. > > > > Doing it this way, mock can be imported in all of the tests in this directory > > without having to add a statement modifying sys.path at the top of all of the > > test files. > > > > (I'm not sure if this is the best way to do this though.) > > Stuff will either be run with a proper PYTHONPATH, and if it's not you should > look at add_build_paths() in > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/tools.... > See it in action in > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... Hey, I didn't really understand what you're suggesting I do here. I don't think that the PYTHONPATH is set up to include telemetry and pymock when the bisect script is run on the bisect bots, although I'm not fully sure. Are you suggesting that I should change something somewhere else and then assume that the python path includes pymock and telemetry here?
On 2014/09/23 12:40:44, agable wrote: > > http://xkcd.com/927/ > > have you talked with hinoka@ and dnj@ about our efforts to standardize the > various way builders upload and download artifacts from google storage? Nope, I haven't. Might there be something here accessible by the bisect script (which is in chromium/src/tools currently) which simplify the build artifact downloading?
On 2014/09/24 03:35:48, qyearsley wrote: > On 2014/09/23 12:40:44, agable wrote: > > > > http://xkcd.com/927/ > > > > have you talked with hinoka@ and dnj@ about our efforts to standardize the > > various way builders upload and download artifacts from google storage? > > Nope, I haven't. Might there be something here accessible by the bisect script > (which is in chromium/src/tools currently) which simplify the build artifact > downloading? You should ask them :) I'm not caught up on their current progress.
On 2014/09/18 23:05:53, stip wrote: > https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... > File tools/auto_bisect/mock_wrapper.py (right): > > https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... > tools/auto_bisect/mock_wrapper.py:1: # Copyright 2014 The Chromium Authors. All > rights reserved. > On 2014/09/18 22:58:59, qyearsley wrote: > > On 2014/09/17 00:12:08, stip wrote: > > > this is just a workaround to get pylint to stop complaining? we can probably > > > remove this and cloud_storage_wrapper.py and just fix our pylint invocation > > > > This was intended as a way to make the import sections at the top of > > fetch_build.py and fetch_build_test.py look nicer; otherwise I think I'd have > to > > modify sys.path at the top of those files. > > > > Doing it this way, mock can be imported in all of the tests in this directory > > without having to add a statement modifying sys.path at the top of all of the > > test files. > > > > (I'm not sure if this is the best way to do this though.) > > Stuff will either be run with a proper PYTHONPATH, and if it's not you should > look at add_build_paths() in > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/tools.... > See it in action in > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... Hey, you're right -- runit.py adds both telemetry and mock to the path, it looks like both of these things can be imported directly. In case telemetry and mock aren't necessarily set up locally, I added these things to sys.path for unit tests in run_tests and for pylint in PRESUBMIT.py. Although, this means that in order to run the unit tests or pylint directly on our workstations, we must set up PYTHONPATH to include telemetry and mock (e.g. in ~/.bashrc).
On 2014/09/25 23:02:52, qyearsley wrote: > On 2014/09/18 23:05:53, stip wrote: > > > https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... > > File tools/auto_bisect/mock_wrapper.py (right): > > > > > https://chromiumcodereview.appspot.com/548233002/diff/40001/tools/auto_bisect... > > tools/auto_bisect/mock_wrapper.py:1: # Copyright 2014 The Chromium Authors. > All > > rights reserved. > > On 2014/09/18 22:58:59, qyearsley wrote: > > > On 2014/09/17 00:12:08, stip wrote: > > > > this is just a workaround to get pylint to stop complaining? we can > probably > > > > remove this and cloud_storage_wrapper.py and just fix our pylint > invocation > > > > > > This was intended as a way to make the import sections at the top of > > > fetch_build.py and fetch_build_test.py look nicer; otherwise I think I'd > have > > to > > > modify sys.path at the top of those files. > > > > > > Doing it this way, mock can be imported in all of the tests in this > directory > > > without having to add a statement modifying sys.path at the top of all of > the > > > test files. > > > > > > (I'm not sure if this is the best way to do this though.) > > > > Stuff will either be run with a proper PYTHONPATH, and if it's not you should > > look at add_build_paths() in > > > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/tools.... > > See it in action in > > > https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/slave... > > Hey, you're right -- runit.py adds both telemetry and mock to the path, it looks > like both of these things can be imported directly. > > In case telemetry and mock aren't necessarily set up locally, I added these > things to sys.path for unit tests in run_tests and for pylint in PRESUBMIT.py. > > Although, this means that in order to run the unit tests or pylint directly on > our workstations, we must set up PYTHONPATH to include telemetry and mock (e.g. > in ~/.bashrc). Or you can wrap the tests and pylint in runit.py :)
On 2014/09/26 10:24:54, agable wrote: > Or you can wrap the tests and pylint in runit.py :) Is this possible? auto_bisect isn't in tools/build. Note, I talked to hinoka@ briefly about ways of downloading things from google storage -- this may or may not be the same as other use-cases (since this is downloading stuff produced by other masters) but either way I'll plan to stay updated on that. Ryan also told me that try jobs can now be started specifying revision numbers of dependency repos as well as src/, which is awesome. Alright, Ojan, Prasad et al, does this look OK to submit now? (Remember, this is a preliminary CL -- this doesn't yet make the main bisect script use fetch_build.py to download builds.)
lgtm IMO, nearly all of the one-line docstring comments in this patch are redundant with the name of the function and I'd remove them. https://codereview.chromium.org/548233002/diff/140001/tools/auto_bisect/fetch... File tools/auto_bisect/fetch_build.py (right): https://codereview.chromium.org/548233002/diff/140001/tools/auto_bisect/fetch... tools/auto_bisect/fetch_build.py:266: sevenzip_path = r'C:\Program Files\7-Zip\7z.exe' Why is this a regexp? https://codereview.chromium.org/548233002/diff/140001/tools/auto_bisect/run_t... File tools/auto_bisect/run_tests (right): https://codereview.chromium.org/548233002/diff/140001/tools/auto_bisect/run_t... tools/auto_bisect/run_tests:18: print sys.path Did you mean to leave this in?
https://codereview.chromium.org/548233002/diff/140001/tools/auto_bisect/fetch... File tools/auto_bisect/fetch_build.py (right): https://codereview.chromium.org/548233002/diff/140001/tools/auto_bisect/fetch... tools/auto_bisect/fetch_build.py:266: sevenzip_path = r'C:\Program Files\7-Zip\7z.exe' On 2014/09/29 23:53:42, ojan-only-code-yellow-reviews wrote: > Why is this a regexp? The "r" in front of the string literal here means that it's a "raw string", not necessarily a regexp. It tells python to ignore the escape meaning of any backslashes. Note that: 'C:\Program Files\7-Zip\7z.exe' == 'C:\\Program Files\x07-Zip\x07z.exe' r'C:\Program Files\7-Zip\7z.exe' == 'C:\\Program Files\\7-Zip\\7z.exe' https://codereview.chromium.org/548233002/diff/140001/tools/auto_bisect/run_t... File tools/auto_bisect/run_tests (right): https://codereview.chromium.org/548233002/diff/140001/tools/auto_bisect/run_t... tools/auto_bisect/run_tests:18: print sys.path On 2014/09/29 23:53:42, ojan-only-code-yellow-reviews wrote: > Did you mean to leave this in? Nope! Thanks for catching this. I was just thinking of possibly adding to sys.path in main(), does this seem reasonable?
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/548233002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as 7927fb78eb719ae17cbd41b15bd2b3bf4750ec3b
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0d9779113ad21ce858abdaaefe240bab950e435a Cr-Commit-Position: refs/heads/master@{#297669} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
