Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236703003:
tool for repackaging chrome.perf builds for manual bisect (Closed)
Descriptiontool for repackaging chrome.perf builds for manual bisect
BUG=604452
R=dimu,prasadv@chromium.org
Committed: https://crrev.com/1374854daaad57f13df1251f261c47bc94317c37
Cr-Commit-Position: refs/heads/master@{#417962}
Patch Set 1 #Patch Set 2 : tool for repackaging chrome.perf builds for manual bisect #
Total comments: 21
Patch Set 3 : tool for repackaging chrome.perf builds for manual bisect #
Total comments: 1
Patch Set 4 : linux repackaging tool for manual bisect #Patch Set 5 : Repackaging tool for linux, mac, and win64 #
Total comments: 10
Patch Set 6 : fixed style issues #
Total comments: 12
Patch Set 7 : fixed error logging/exception #
Messages
Total messages: 18 (6 generated)
Description was changed from ========== tool for repackaging chrome.perf builds for manual bisect BUG=604452 R=dimu,prasadv@chromium.org ========== to ========== tool for repackaging chrome.perf builds for manual bisect BUG=604452 R=dimu,prasadv@chromium.org ==========
miimnk@google.com changed reviewers: + prasadv@google.com
https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... File tools/bisect_repackage/bisect_repackage_linux.py (right): https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:66: STAGING_DIR= os.path.join('.', 'a', 'tmp_forward') is there any cleanup for the staging dir? https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:92: print 'ValueError for JSON URL: %s' % json_url It would be better to use logging instead of print. (https://docs.python.org/2/howto/logging.html) https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:93: return None throw exception for failure, and use try/catch in the caller. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:114: def isProperHash(regex_match): isGitCommitHash https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:114: def isProperHash(regex_match): move it to utils https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:116: if len(regex_match) == 40: return True use re.match(r"^[0-9,A-F]{40}$", regex_match.upper()) https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:119: def isProperRevision(regex_match): move this to utils https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:121: if len(regex_match) == 6: return True also use regex https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:166: with open(context.revision_map, 'r') as f: nit: better naming: revision_file https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:196: def make_filtered_archive(file_archive, archive_name): better naming https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... File tools/bisect_repackage/bisect_repackage_utils.py (right): https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_utils.py:86: def MaybeMakeDirectory(*path): better naming
Hi Di. Thank you for your code review. I fixed the issues in your comments. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... File tools/bisect_repackage/bisect_repackage_linux.py (right): https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:66: STAGING_DIR= os.path.join('.', 'a', 'tmp_forward') On 2016/08/19 04:14:05, dimu wrote: > is there any cleanup for the staging dir? Erased the two global variables and used 'tempfile.mkdtemp' instead. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:92: print 'ValueError for JSON URL: %s' % json_url On 2016/08/19 04:14:05, dimu wrote: > It would be better to use logging instead of print. > (https://docs.python.org/2/howto/logging.html) Done. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:93: return None On 2016/08/19 04:14:05, dimu wrote: > throw exception for failure, and use try/catch in the caller. Done. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:114: def isProperHash(regex_match): On 2016/08/19 04:14:05, dimu wrote: > move it to utils Done. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:114: def isProperHash(regex_match): On 2016/08/19 04:14:05, dimu wrote: > move it to utils Done. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:116: if len(regex_match) == 40: return True On 2016/08/19 04:14:05, dimu wrote: > use re.match(r"^[0-9,A-F]{40}$", regex_match.upper()) Done. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:119: def isProperRevision(regex_match): On 2016/08/19 04:14:05, dimu wrote: > move this to utils Done. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:121: if len(regex_match) == 6: return True On 2016/08/19 04:14:05, dimu wrote: > also use regex Done. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:166: with open(context.revision_map, 'r') as f: On 2016/08/19 04:14:05, dimu wrote: > nit: better naming: revision_file Done. https://codereview.chromium.org/2236703003/diff/20001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_linux.py:196: def make_filtered_archive(file_archive, archive_name): On 2016/08/19 04:14:05, dimu wrote: > better naming Done. Changed to 'make_lightweight_archive'.
lgtm except nit https://codereview.chromium.org/2236703003/diff/40001/tools/bisect_repackage/... File tools/bisect_repackage/bisect_repackage_utils.py (right): https://codereview.chromium.org/2236703003/diff/40001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage_utils.py:5: """ Set of basic operations/utilities that are used by repacakging builds add a comment where most functions moved from.
Min, can you please run gpylint on this file, I think you need to fix these first. Meanwhile I'll continue the review for the logic part. https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... File tools/bisect_repackage/bisect_repackage.py (right): https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:10: #Declares required files to run manual bisect script on chrome Linux nit: Need 1 space after # https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:17: 'linux': [ 4 space on hanging indent. repeat same wherever applicable https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:103: ################################################################################ Remove this # line https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:104: import os Ordering of import is not correct. refer Python style guide or run gpylint on this file. https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:120: """A PathContext is used to carry the information used to construct URLs and Single line doc string.
Thank you so much for your review Prasad. I fixed many issues that appeared in gpylint. Best, Min https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... File tools/bisect_repackage/bisect_repackage.py (right): https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:10: #Declares required files to run manual bisect script on chrome Linux On 2016/08/30 22:12:20, prasadv wrote: > nit: Need 1 space after # Done. https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:17: 'linux': [ On 2016/08/30 22:12:20, prasadv wrote: > 4 space on hanging indent. > repeat same wherever applicable > > https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s... Done. https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:103: ################################################################################ On 2016/08/30 22:12:20, prasadv wrote: > Remove this # line Done. https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:104: import os On 2016/08/30 22:12:20, prasadv wrote: > Ordering of import is not correct. > refer Python style guide > or run gpylint on this file. Done. https://codereview.chromium.org/2236703003/diff/80001/tools/bisect_repackage/... tools/bisect_repackage/bisect_repackage.py:120: """A PathContext is used to carry the information used to construct URLs and On 2016/08/30 22:12:21, prasadv wrote: > Single line doc string. Done.
https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... File tools/bisect_repackage/bisect_repackage.py (right): https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:75: # ^$ meanse not to include any files from whitelist nit: s/meanse/means https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:134: """Converts a GitHash to Commit position number.""" s/GitHash/git hash s/Commit/commit https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:140: except ValueError: May be you should display the actual error in the exception https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:145: raise ValueError('Failed to extract commit position') Can we display the the actual response code and the URL in error. https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:164: print 'Downloaded %s / %s git hash' %(count, len(hash_list)) Is there any specific reason for printing this? https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:298: raise ValueError('Executing Chrome Failed: Need to check ') I think we should define a userdefine exception and raise that exception instead of ValueError everywhere. Something like class BisectPackingError(Exception): def __str__(self): return self.args[0]
Hi Prasad. Thank you for your code review. I fixed the issues you mentioned. Best, Min https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... File tools/bisect_repackage/bisect_repackage.py (right): https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:75: # ^$ meanse not to include any files from whitelist On 2016/09/09 21:34:01, prasadv wrote: > nit: s/meanse/means Done. https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:134: """Converts a GitHash to Commit position number.""" On 2016/09/09 21:34:02, prasadv wrote: > s/GitHash/git hash > s/Commit/commit Done. https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:140: except ValueError: On 2016/09/09 21:34:01, prasadv wrote: > May be you should display the actual error in the exception Changed to: except Exception,e: logging.warning('JSON URL: %s, Error Message: %s' % json_url, e) raise GitConversionError https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:145: raise ValueError('Failed to extract commit position') On 2016/09/09 21:34:02, prasadv wrote: > Can we display the the actual response code and the URL in error. Changed to: except Exception,e: logging.warning('JSON URL: %s, Error Message: %s' % json_url, e) raise GitConversionError https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:164: print 'Downloaded %s / %s git hash' %(count, len(hash_list)) On 2016/09/09 21:34:01, prasadv wrote: > Is there any specific reason for printing this? Removed https://codereview.chromium.org/2236703003/diff/100001/tools/bisect_repackage... tools/bisect_repackage/bisect_repackage.py:298: raise ValueError('Executing Chrome Failed: Need to check ') On 2016/09/09 21:34:02, prasadv wrote: > I think we should define a userdefine exception and raise that exception instead > of ValueError everywhere. > > Something like > class BisectPackingError(Exception): > def __str__(self): > return self.args[0] Changed to try: command = [os.path.join(zip_dir, 'chrome')] code = bisect_repackage_utils.RunCommand(command) if code != 0: raise ChromeExecutionError('An error occurred when executing Chrome') except ChromeExecutionError,e: print str(e)
lgtm
The CQ bit was checked by miimnk@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dimu@google.com Link to the patchset: https://codereview.chromium.org/2236703003/#ps120001 (title: "fixed error logging/exception")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== tool for repackaging chrome.perf builds for manual bisect BUG=604452 R=dimu,prasadv@chromium.org ========== to ========== tool for repackaging chrome.perf builds for manual bisect BUG=604452 R=dimu,prasadv@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== tool for repackaging chrome.perf builds for manual bisect BUG=604452 R=dimu,prasadv@chromium.org ========== to ========== tool for repackaging chrome.perf builds for manual bisect BUG=604452 R=dimu,prasadv@chromium.org Committed: https://crrev.com/1374854daaad57f13df1251f261c47bc94317c37 Cr-Commit-Position: refs/heads/master@{#417962} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1374854daaad57f13df1251f261c47bc94317c37 Cr-Commit-Position: refs/heads/master@{#417962} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
