Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
468823003:
[Findit] findit algorithms and a cluster-fuzz implementation. (Closed)
Description[Findit] findit algorithms and a cluster-fuzz implementation.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290959
Patch Set 1 #
Total comments: 29
Patch Set 2 : Addressed code review. #
Total comments: 3
Patch Set 3 : addressed codereview #
Total comments: 84
Patch Set 4 : addressed codereview and changed the algorithm to look for file path rather than file name #
Total comments: 18
Patch Set 5 : Addressed codereview. #
Total comments: 6
Patch Set 6 : addressed codereview #
Messages
Total messages: 22 (0 generated)
Hi Shuotao, This is a CL containing core algorithms for findit. Can you take a look? Thanks!
https://codereview.chromium.org/468823003/diff/1/tools/findit/chromium_deps.py File tools/findit/chromium_deps.py (right): https://codereview.chromium.org/468823003/diff/1/tools/findit/chromium_deps.p... tools/findit/chromium_deps.py:1: # Copyright (c) 2014 The Chromium Authors. All rights reserved. This File could be reverted, as it is already in another CL you sent out. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... File tools/findit/findit_for_clusterfuzz.py (right): https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... tools/findit/findit_for_clusterfuzz.py:58: def GetResultsFromString(stacktrace_string, Rename to "FindCulpritCL"? https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... tools/findit/findit_for_clusterfuzz.py:65: """Returns string representation of the result, a list of suspected CLs. After the result.py is committed, maybe refer to result.Result instead of saying suspected CLs. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... tools/findit/findit_for_clusterfuzz.py:79: A list of result objects. We agreed the returned result would be as below, right? (message, result_list) message: Message as summary of the result. result_list: a list of result.Result, could be an empty list. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... tools/findit/findit_for_clusterfuzz.py:125: release_stacktrace_exists = parsed_release_build_stacktrace.stack_list *_exists is not necessary, could be merged into the if/elif clause. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:20: logging.basicConfig(filename='errors.log', level=logging.WARNING, Let's move this out. logging setup should be done by the end user, not by findit. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:46: # Check if this CL should be avoided. avoided -> ignored? https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:166: This empty line could be removed. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:204: results_lock: A lock that guards result_map. result_map is not clear. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:236: results_lock: A lock that guards all_results. all_result is not clear. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:356: This empty line could be removed. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:374: # be ignored. Could this comment turned into bullet point? https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:400: reason = [] reasons? https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:414: (priority, crashed_line_number, file_name, file_url) = \ file_name or file_path? Same the one for above in line 409. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:567: This empty line could be removed.
Hi Shuotao, I have addressed the codereview. Thanks! https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... File tools/findit/findit_for_clusterfuzz.py (right): https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... tools/findit/findit_for_clusterfuzz.py:58: def GetResultsFromString(stacktrace_string, On 2014/08/14 18:25:42, Shuotao wrote: > Rename to "FindCulpritCL"? Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... tools/findit/findit_for_clusterfuzz.py:65: """Returns string representation of the result, a list of suspected CLs. On 2014/08/14 18:25:42, Shuotao wrote: > After the result.py is committed, maybe refer to result.Result instead of saying > suspected CLs. Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... tools/findit/findit_for_clusterfuzz.py:79: A list of result objects. On 2014/08/14 18:25:42, Shuotao wrote: > We agreed the returned result would be as below, right? > > (message, result_list) > message: Message as summary of the result. > result_list: a list of result.Result, could be an empty list. Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_clus... tools/findit/findit_for_clusterfuzz.py:125: release_stacktrace_exists = parsed_release_build_stacktrace.stack_list On 2014/08/14 18:25:42, Shuotao wrote: > *_exists is not necessary, could be merged into the if/elif clause. Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:20: logging.basicConfig(filename='errors.log', level=logging.WARNING, On 2014/08/14 18:25:42, Shuotao wrote: > Let's move this out. > logging setup should be done by the end user, not by findit. Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:46: # Check if this CL should be avoided. On 2014/08/14 18:25:42, Shuotao wrote: > avoided -> ignored? Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:166: On 2014/08/14 18:25:42, Shuotao wrote: > This empty line could be removed. Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:204: results_lock: A lock that guards result_map. On 2014/08/14 18:25:42, Shuotao wrote: > result_map is not clear. Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:236: results_lock: A lock that guards all_results. On 2014/08/14 18:25:42, Shuotao wrote: > all_result is not clear. Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:356: On 2014/08/14 18:25:42, Shuotao wrote: > This empty line could be removed. Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:374: # be ignored. On 2014/08/14 18:25:42, Shuotao wrote: > Could this comment turned into bullet point? Done. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:400: reason = [] On 2014/08/14 18:25:42, Shuotao wrote: > reasons? I would call it a reason because it is a list of strings that together compose a reason why this match is a match. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:414: (priority, crashed_line_number, file_name, file_url) = \ On 2014/08/14 18:25:42, Shuotao wrote: > file_name or file_path? Same the one for above in line 409. It is file_name. https://codereview.chromium.org/468823003/diff/1/tools/findit/findit_for_cras... tools/findit/findit_for_crash.py:567: On 2014/08/14 18:25:42, Shuotao wrote: > This empty line could be removed. Done.
hi Shuotao, i have uploaded the new patch with updated file names.
Hi, This is a CL for findit algorithm and clusterfuzz, along with config file. Would you mind taking a look at these? Thanks in advance!
Hi Jason, I did a detail review for your CL. Please address my comments and reupload a new patch. If you have any question on my comments, please feel free to chat with me. Thanks, Shuotao Gao https://codereview.chromium.org/468823003/diff/40001/tools/findit/findit_for_... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/40001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:20: URL_MAP = crash_utils.ParseURLsFromConfig('urls.config') The logic to handle the absolute path of the config file should be in the code that use crash_utils.ParseURLsFromConfig, and not in crash_utils.ParseURLsFromConfig. Please make the change in crash_utils.py too. https://codereview.chromium.org/468823003/diff/40001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:33: cl: The SVN revision number or git hash. What's the difference between cls in |revisions| and |cl|? As revisions is used to retrieve revisions[cl], why not just pass in revisions[cl] instead of both |revisions| and |cl|? https://codereview.chromium.org/468823003/diff/40001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:41: file_action: Whether file is modified, added or deleted. file_change_type? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... File tools/findit/findit_for_clusterfuzz.py (right): https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:67: """Returns json format of the result, a list of result.Result objects. Seems json format is wrong now? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:88: logging.basicConfig(filename='errors.log', level=logging.WARNING, Findit should not write any log. It is the responsibility of the caller to setup the logging if they want. Please also include this in the function documentation above. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:97: return ('This feature is not supported.', []) "Component builds are not supported yet." https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:112: crash_revision_dict = chromium_deps.GetChromiumComponents( Do we need to verify |chrome_crash_revision| is not None? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:135: main_stack = parsed_release_build_stacktrace.GetCrashStack() What's main stack? What it is for? Why only handle it? Add an explanation for this. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:141: # Run the algorithm on the parsed stacktrace, and return the JSON. We are not returning JSON any more, right? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:25: matches, revision, cl, file_path, file_name, function, component_path, |revision| -> |revision_info|? |cl| -> |cl_revision|? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:40: crashed_lines: The list of the lines in the file that caused the crash. Line number or line content? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:63: matches.cls_to_ignore.add(match.revert_of) What if |match.revert_of| is None? It is not true that we could tell whether it is a revert or not by just the "message". https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:86: (line_intersection, stack_frame_index_intersection) = ( line_number_intersection? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:117: if priority < match.rank: If priority and rank are the same, could we just use one of them instead of both? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:128: revisions: A dictionary mapping revision number to the CL information. revision_info_map? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:129: file_map: A dictionary mapping file to the revision that modifies it. file_to_revision_info? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:130: file_dict: A dictionary mapping file to its occurrence in stacktrace. file_to_frames? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:198: file_dict: A dictionary mapping file to its occurrence in stackframe. file_to_frames? Same for other occurrences of this name. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:230: Args: One empty line before this. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:245: file_dict = component_dict.GetFileDict(component_path) Not clear for var name |file_dict|. Please rename it to a better one. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:248: continue Why continue? Please add a comment. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:263: def FindMatchForStacktrace(stacktrace, components, regression_dict): stack_trace? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:277: # A list to aggregate results from the whole stacktrace. multiple call stacks in the whole stack trace? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:292: continue Why continue? Should we just return a message "Regression from 0 to * is too big and not supported." ? Please add a comment. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:309: continue Why continue? Please add a comment. For other code like this, please add comments. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:344: in_crash_state = False What's crash state? Do you need this concept? If so, please add some explanation what it is. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:347: # Sort the matches in specific order. Please add more details on what order it is and why in that order. This will help to understand the algorithm behind it. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:361: if current_stack < highest_priority_stack: Will two stacks have the same priority? If so, what should we do? Please also add comments to algorithm-related code. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:365: flattened_stack_frame_indices = [i for sl in match.stack_frame_indices Use full name for |sl|. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:402: # (how the file is modified) and sort it in specific order. What order it is and why? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:419: ' Line %s of file %s which caused the crash according ' Why blank space at the beginning? Formatting like indent here should be the responsibility of the end user. Here we just provide the raw data without any format. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:430: file_string = ' File %s is changed in this cl.\n' Same as above. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:448: def CombineMatches(matches): CombineReasonsForMatches? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:466: found_match = match_combined add a break? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:497: def ParseCrashComponents(main_stack, top_n_frames=3): Why default is 3? Would you mind a comment and explanation here? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:522: crash_revision_dict: A dictionary mapping component to its crash revision. component_to_revision_info? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:556: parsed_regression: A parsed regression information as a result of parsing component_to_regression_info? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:558: parsed_crash_revision: A parsed crash revision information. Prefix "parsed" is not needed. Same for other occurrences in all code. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:580: components = ParseCrashComponents(main_stack) Why do we handle the first 3 frames only? Add an explanation for the reason behind that. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:591: 'The result is a list of CLs that change the crashed files.') Also add "Wrong result? Please email findit-for-me-eng@." https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:595: # If not match is found, return the blame information for the input "not" -> "no"?
Hi, I have addressed the code review. Would you mind taking another look? Thanks, Jason https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... File tools/findit/findit_for_clusterfuzz.py (right): https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:67: """Returns json format of the result, a list of result.Result objects. On 2014/08/18 22:37:07, Shuotao wrote: > Seems json format is wrong now? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:88: logging.basicConfig(filename='errors.log', level=logging.WARNING, On 2014/08/18 22:37:07, Shuotao wrote: > Findit should not write any log. > > It is the responsibility of the caller to setup the logging if they want. > Please also include this in the function documentation above. Does this mean anything other than this file should not use logging module in it? If so, how would I be able to log error messages? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:97: return ('This feature is not supported.', []) On 2014/08/18 22:37:07, Shuotao wrote: > "Component builds are not supported yet." Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:112: crash_revision_dict = chromium_deps.GetChromiumComponents( On 2014/08/18 22:37:06, Shuotao wrote: > Do we need to verify |chrome_crash_revision| is not None? Yes, in case we only have component crash revision https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:135: main_stack = parsed_release_build_stacktrace.GetCrashStack() On 2014/08/18 22:37:06, Shuotao wrote: > What's main stack? What it is for? Why only handle it? > Add an explanation for this. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:141: # Run the algorithm on the parsed stacktrace, and return the JSON. On 2014/08/18 22:37:07, Shuotao wrote: > We are not returning JSON any more, right? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:25: matches, revision, cl, file_path, file_name, function, component_path, On 2014/08/18 22:37:09, Shuotao wrote: > |revision| -> |revision_info|? > |cl| -> |cl_revision|? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:40: crashed_lines: The list of the lines in the file that caused the crash. On 2014/08/18 22:37:08, Shuotao wrote: > Line number or line content? Changed to crashed_line_numbers https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:63: matches.cls_to_ignore.add(match.revert_of) On 2014/08/18 22:37:07, Shuotao wrote: > What if |match.revert_of| is None? I check in line 62 if match.revert_of is None > It is not true that we could tell whether it is a revert or not by just the > "message". In match_set.py, I'm trying to be very conservative by marking it as a revert only if it contains some sort of "reverting/revert of/reverts xxxxx". Also, revert_of checks whether this CL reverts some other CL. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:86: (line_intersection, stack_frame_index_intersection) = ( On 2014/08/18 22:37:07, Shuotao wrote: > line_number_intersection? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:117: if priority < match.rank: On 2014/08/18 22:37:08, Shuotao wrote: > If priority and rank are the same, could we just use one of them instead of > both? match.rank is same as the highest priority. I will remove it. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:128: revisions: A dictionary mapping revision number to the CL information. On 2014/08/18 22:37:09, Shuotao wrote: > revision_info_map? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:129: file_map: A dictionary mapping file to the revision that modifies it. On 2014/08/18 22:37:08, Shuotao wrote: > file_to_revision_info? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:130: file_dict: A dictionary mapping file to its occurrence in stacktrace. On 2014/08/18 22:37:08, Shuotao wrote: > file_to_frames? Renamed to file_to_crash_map https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:198: file_dict: A dictionary mapping file to its occurrence in stackframe. On 2014/08/18 22:37:08, Shuotao wrote: > file_to_frames? > > Same for other occurrences of this name. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:230: Args: On 2014/08/18 22:37:09, Shuotao wrote: > One empty line before this. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:245: file_dict = component_dict.GetFileDict(component_path) On 2014/08/18 22:37:08, Shuotao wrote: > Not clear for var name |file_dict|. > Please rename it to a better one. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:248: continue On 2014/08/18 22:37:08, Shuotao wrote: > Why continue? > Please add a comment. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:263: def FindMatchForStacktrace(stacktrace, components, regression_dict): On 2014/08/18 22:37:07, Shuotao wrote: > stack_trace? I will go consistent with the variable name in other files. They are named stacktrace. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:277: # A list to aggregate results from the whole stacktrace. On 2014/08/18 22:37:07, Shuotao wrote: > multiple call stacks in the whole stack trace? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:292: continue On 2014/08/18 22:37:09, Shuotao wrote: > Why continue? Should we just return a message "Regression from 0 to * is too big > and not supported." ? > > Please add a comment. We are continuing because this loop is going over all the components, and if we just return here, we are ignoring all other components which might not have 0 as starting range. Should I just log an error message here? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:309: continue On 2014/08/18 22:37:07, Shuotao wrote: > Why continue? > > Please add a comment. > > For other code like this, please add comments. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:344: in_crash_state = False On 2014/08/18 22:37:09, Shuotao wrote: > What's crash state? > Do you need this concept? > If so, please add some explanation what it is. Added additional parameter to the function and explained the concept. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:347: # Sort the matches in specific order. On 2014/08/18 22:37:09, Shuotao wrote: > Please add more details on what order it is and why in that order. > > This will help to understand the algorithm behind it. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:361: if current_stack < highest_priority_stack: On 2014/08/18 22:37:07, Shuotao wrote: > Will two stacks have the same priority? If so, what should we do? > Please also add comments to algorithm-related code. Added more detailed comments. Two stacks having same priority is not a special case. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:365: flattened_stack_frame_indices = [i for sl in match.stack_frame_indices On 2014/08/18 22:37:09, Shuotao wrote: > Use full name for |sl|. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:402: # (how the file is modified) and sort it in specific order. On 2014/08/18 22:37:08, Shuotao wrote: > What order it is and why? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:419: ' Line %s of file %s which caused the crash according ' On 2014/08/18 22:37:09, Shuotao wrote: > Why blank space at the beginning? > Formatting like indent here should be the responsibility of the end user. > Here we just provide the raw data without any format. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:430: file_string = ' File %s is changed in this cl.\n' On 2014/08/18 22:37:07, Shuotao wrote: > Same as above. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:448: def CombineMatches(matches): On 2014/08/18 22:37:08, Shuotao wrote: > CombineReasonsForMatches? This is not only combining reasons, but also combine matches overall, because different callstacks can have same CL as the match, so the matches list can contain two same CLs. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:466: found_match = match_combined On 2014/08/18 22:37:09, Shuotao wrote: > add a break? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:497: def ParseCrashComponents(main_stack, top_n_frames=3): On 2014/08/18 22:37:09, Shuotao wrote: > Why default is 3? > Would you mind a comment and explanation here? It is an arbitrary number (it is because clusterfuzz seems to take top 3 frames as the crash state), I'm not sure if I have to explain how I chose the number. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:522: crash_revision_dict: A dictionary mapping component to its crash revision. On 2014/08/18 22:37:08, Shuotao wrote: > component_to_revision_info? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:556: parsed_regression: A parsed regression information as a result of parsing On 2014/08/18 22:37:08, Shuotao wrote: > component_to_regression_info? Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:558: parsed_crash_revision: A parsed crash revision information. On 2014/08/18 22:37:09, Shuotao wrote: > Prefix "parsed" is not needed. > > Same for other occurrences in all code. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:580: components = ParseCrashComponents(main_stack) On 2014/08/18 22:37:08, Shuotao wrote: > Why do we handle the first 3 frames only? > Add an explanation for the reason behind that. Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:591: 'The result is a list of CLs that change the crashed files.') On 2014/08/18 22:37:08, Shuotao wrote: > Also add "Wrong result? Please email findit-for-me-eng@." I will add it after discussing with Abhishek/Marty. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_crash.py:595: # If not match is found, return the blame information for the input On 2014/08/18 22:37:08, Shuotao wrote: > "not" -> "no"? Done.
I started preparing these comments earlier today but didn't publish them. Sorry that they're on the older version of the CL. I'll just publish these now and continue on the newer version. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... File tools/findit/findit_for_clusterfuzz.py (right): https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:85: if 'android' in build_type or 'syzyasan' in build_type: Why are these job types being skipped explicitly? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:94: # TODO(jeun): Come up with a good way to connect crashing component name to What work is still needed to be done here? I thought the limitation was just that we didn't have the chromium revision for component builds yet. If called with all of the required information, it's important that this works. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:105: # If the regression is not reliable, do not parse regression information. A regression range starting with 0 has nothing to do with its reliability. In fact, they are probably the most reliable regression ranges we have since it means the crash reproduced in the earliest build for a job type. This should just say something like "Do not parse regression information for crashes introduced before the first archived build for a job type" (or some shorter variation of that). https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:122: return ('No regression and crash revision information available.', []) If someone runs into an error, they probably don't really care what went wrong but rather what needs to be done about it. If we run into this case, it's probably the caller's fault. Try to make how they can fix this clear. Maybe something like "Identifying culprit CLs requires a crash revision or regression range."
https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:58: if match.is_reverted: s/is_reverted/is_revert/ https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:80: # Ignore this match if the component is not supported for svn. Is this correct? If so, is there more work that needs to be done here? https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:104: file_name = os.path.basename(file_path) Could you confirm that this works on Windows? The stacks will still contain forward slashes. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:175: if file_change_type == 'D': Should this be startswith('D'), or are other parts of the status ignored before we get to this point? I'm not sure how often this would come up either way. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:304: # If range start is 0, it means regression is not reliable, so ignore. Again, 0 isn't about reliability. It just means that the crash was introduced before the archived builds. It makes sense to continue in this case since the range is so large, but please update the comment. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:370: # 2) The callstack this match is computed (crash stack, freed, allocation) Was this updated so that free/allocation stacks have the same priority? If so, using the name stack_index below might not make sense. Maybe stack_priority? https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:375: matches.sort(key=lambda (stack_index, cl, match): Using a lambda here is pretty ugly. Please just define a function for this and move the comment explaining the sort priority to there. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:450: 'Line %s of file %s which caused the crash according ' I'm not sure about saying "caused the crash" here. It sounds certain. Maybe "potentially caused the crash". https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:500: # If current match is not already in, add it to the list of matches. In general, I think this file had a lot of unnecessary comments. This is one example.
Hi, I have addressed the codereview. Would you mind taking another look? Thanks, Jason https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... File tools/findit/findit_for_clusterfuzz.py (right): https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:85: if 'android' in build_type or 'syzyasan' in build_type: On 2014/08/19 02:51:35, mbarbella wrote: > Why are these job types being skipped explicitly? The stacktrace parsing for them is not supported. I will add them. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:94: # TODO(jeun): Come up with a good way to connect crashing component name to On 2014/08/19 02:51:35, mbarbella wrote: > What work is still needed to be done here? I thought the limitation was just > that we didn't have the chromium revision for component builds yet. If called > with all of the required information, it's important that this works. Since we started parsing DEPS file for regression range/crash revisions (and not clusterfuzz website like https://cluster-fuzz.appspot.com/revisions?job=windows_syzyasan_content_shell...), we do not have an access to the "name" of the component, like v8, blink, chromium etc. Instead, we use the "path" of the component (for example, 'src/third_party/Source' for blink). For DEPS file parsing, path is unique while parsed component name is not. We were thinking about what would be a good way of passing in the name of the crashing component. If you are fine with it, you can just pass in the path as defined in DEPS file, or we can create a config file mapping a component's path to its component name. We prefer first one, what is your opinion on this? https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:105: # If the regression is not reliable, do not parse regression information. On 2014/08/19 02:51:35, mbarbella wrote: > A regression range starting with 0 has nothing to do with its reliability. In > fact, they are probably the most reliable regression ranges we have since it > means the crash reproduced in the earliest build for a job type. > > This should just say something like "Do not parse regression information for > crashes introduced before the first archived build for a job type" (or some > shorter variation of that). Done. https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... tools/findit/findit_for_clusterfuzz.py:122: return ('No regression and crash revision information available.', []) On 2014/08/19 02:51:35, mbarbella wrote: > If someone runs into an error, they probably don't really care what went wrong > but rather what needs to be done about it. If we run into this case, it's > probably the caller's fault. Try to make how they can fix this clear. > > Maybe something like "Identifying culprit CLs requires a crash revision or > regression range." Done. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:58: if match.is_reverted: On 2014/08/19 16:11:33, mbarbella wrote: > s/is_reverted/is_revert/ Done. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:80: # Ignore this match if the component is not supported for svn. On 2014/08/19 16:11:33, mbarbella wrote: > Is this correct? If so, is there more work that needs to be done here? Yes this is correct, for SVN we need to write a parser for googlecode (v8 and others). But assuming that they will show git hash eventually, I don't think it is too much of a problem? https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:104: file_name = os.path.basename(file_path) On 2014/08/19 16:11:33, mbarbella wrote: > Could you confirm that this works on Windows? The stacks will still contain > forward slashes. Changed all occurrences to file_path.split('/')[-1]. All path should be in linux path when this is called elsewhere, so this should work everywhere. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:175: if file_change_type == 'D': On 2014/08/19 16:11:32, mbarbella wrote: > Should this be startswith('D'), or are other parts of the status ignored before > we get to this point? I'm not sure how often this would come up either way. Yes the other parts are ignored when the check happens. In blame, it returns 'delete', so I make it into 'D'. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:304: # If range start is 0, it means regression is not reliable, so ignore. On 2014/08/19 16:11:32, mbarbella wrote: > Again, 0 isn't about reliability. It just means that the crash was introduced > before the archived builds. It makes sense to continue in this case since the > range is so large, but please update the comment. Done. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:370: # 2) The callstack this match is computed (crash stack, freed, allocation) On 2014/08/19 16:11:33, mbarbella wrote: > Was this updated so that free/allocation stacks have the same priority? If so, > using the name stack_index below might not make sense. Maybe stack_priority? Yes it has been updated. I changed the name to stack_priority. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:375: matches.sort(key=lambda (stack_index, cl, match): On 2014/08/19 16:11:33, mbarbella wrote: > Using a lambda here is pretty ugly. Please just define a function for this and > move the comment explaining the sort priority to there. Done. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:450: 'Line %s of file %s which caused the crash according ' On 2014/08/19 16:11:33, mbarbella wrote: > I'm not sure about saying "caused the crash" here. It sounds certain. Maybe > "potentially caused the crash". Done. https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... tools/findit/findit_for_crash.py:500: # If current match is not already in, add it to the list of matches. On 2014/08/19 16:11:33, mbarbella wrote: > In general, I think this file had a lot of unnecessary comments. This is one > example. I removed a lot of self-explanatory comments.
On 2014/08/19 18:30:06, jeun wrote: > Hi, > > I have addressed the codereview. Would you mind taking another look? > > Thanks, > Jason > > https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... > File tools/findit/findit_for_clusterfuzz.py (right): > > https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... > tools/findit/findit_for_clusterfuzz.py:85: if 'android' in build_type or > 'syzyasan' in build_type: > On 2014/08/19 02:51:35, mbarbella wrote: > > Why are these job types being skipped explicitly? > The stacktrace parsing for them is not supported. I will add them. Try to prioritize android over syzyasan for this. > > https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... > tools/findit/findit_for_clusterfuzz.py:94: # TODO(jeun): Come up with a good way > to connect crashing component name to > On 2014/08/19 02:51:35, mbarbella wrote: > > What work is still needed to be done here? I thought the limitation was just > > that we didn't have the chromium revision for component builds yet. If called > > with all of the required information, it's important that this works. > > Since we started parsing DEPS file for regression range/crash revisions (and not > clusterfuzz website like > https://cluster-fuzz.appspot.com/revisions?job=windows_syzyasan_content_shell...), > we do not have an access to the "name" of the component, like v8, blink, > chromium etc. Instead, we use the "path" of the component (for example, > 'src/third_party/Source' for blink). For DEPS file parsing, path is unique while > parsed component name is not. We were thinking about what would be a good way of > passing in the name of the crashing component. If you are fine with it, you can > just pass in the path as defined in DEPS file, or we can create a config file > mapping a component's path to its component name. We prefer first one, what is > your opinion on this? I don't think having the name is too important. Using the path is fine. > > https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... > tools/findit/findit_for_clusterfuzz.py:105: # If the regression is not reliable, > do not parse regression information. > On 2014/08/19 02:51:35, mbarbella wrote: > > A regression range starting with 0 has nothing to do with its reliability. In > > fact, they are probably the most reliable regression ranges we have since it > > means the crash reproduced in the earliest build for a job type. > > > > This should just say something like "Do not parse regression information for > > crashes introduced before the first archived build for a job type" (or some > > shorter variation of that). > > Done. > > https://codereview.chromium.org/468823003/diff/80001/tools/findit/findit_for_... > tools/findit/findit_for_clusterfuzz.py:122: return ('No regression and crash > revision information available.', []) > On 2014/08/19 02:51:35, mbarbella wrote: > > If someone runs into an error, they probably don't really care what went wrong > > but rather what needs to be done about it. If we run into this case, it's > > probably the caller's fault. Try to make how they can fix this clear. > > > > Maybe something like "Identifying culprit CLs requires a crash revision or > > regression range." > > Done. > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > File tools/findit/findit_for_crash.py (right): > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:58: if match.is_reverted: > On 2014/08/19 16:11:33, mbarbella wrote: > > s/is_reverted/is_revert/ > > Done. Did this require any changes in other files, or was it only used here? > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:80: # Ignore this match if the component is not > supported for svn. > On 2014/08/19 16:11:33, mbarbella wrote: > > Is this correct? If so, is there more work that needs to be done here? > Yes this is correct, for SVN we need to write a parser for googlecode (v8 and > others). But assuming that they will show git hash eventually, I don't think it > is too much of a problem? What happens for these components now? I don't care about the SVN support, but I do want to make sure that we try to use the git repositories for these components if it's possible. Either way, this CL doesn't need to be blocked on this. > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:104: file_name = os.path.basename(file_path) > On 2014/08/19 16:11:33, mbarbella wrote: > > Could you confirm that this works on Windows? The stacks will still contain > > forward slashes. > Changed all occurrences to file_path.split('/')[-1]. All path should be in linux > path when this is called elsewhere, so this should work everywhere. > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:175: if file_change_type == 'D': > On 2014/08/19 16:11:32, mbarbella wrote: > > Should this be startswith('D'), or are other parts of the status ignored > before > > we get to this point? I'm not sure how often this would come up either way. > > Yes the other parts are ignored when the check happens. In blame, it returns > 'delete', so I make it into 'D'. Cool, that's fine then. I was just thinking it might have been coming from raw status information. > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:304: # If range start is 0, it means regression > is not reliable, so ignore. > On 2014/08/19 16:11:32, mbarbella wrote: > > Again, 0 isn't about reliability. It just means that the crash was introduced > > before the archived builds. It makes sense to continue in this case since the > > range is so large, but please update the comment. > > Done. > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:370: # 2) The callstack this match is computed > (crash stack, freed, allocation) > On 2014/08/19 16:11:33, mbarbella wrote: > > Was this updated so that free/allocation stacks have the same priority? If so, > > using the name stack_index below might not make sense. Maybe stack_priority? > Yes it has been updated. I changed the name to stack_priority. > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:375: matches.sort(key=lambda (stack_index, cl, > match): > On 2014/08/19 16:11:33, mbarbella wrote: > > Using a lambda here is pretty ugly. Please just define a function for this and > > move the comment explaining the sort priority to there. > > Done. > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:450: 'Line %s of file %s which caused the > crash according ' > On 2014/08/19 16:11:33, mbarbella wrote: > > I'm not sure about saying "caused the crash" here. It sounds certain. Maybe > > "potentially caused the crash". > > Done. > > https://codereview.chromium.org/468823003/diff/100001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:500: # If current match is not already in, add > it to the list of matches. > On 2014/08/19 16:11:33, mbarbella wrote: > > In general, I think this file had a lot of unnecessary comments. This is one > > example. > I removed a lot of self-explanatory comments. Questions addressed inline. I'm going to take a last pass over this now.
LGTM, but please address the minor comments that are left before committing. https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... File tools/findit/findit_for_clusterfuzz.py (right): https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... tools/findit/findit_for_clusterfuzz.py:85: if 'android' in build_type or 'syzyasan' in build_type: Is there any reason that android needs to be skipped at the moment? I discussed this with Shuotao briefly and he thinks it looks like it should work since there aren't any major differences in the stacks. If there isn't a specific issue that was blocking this, please just enable it. https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... tools/findit/findit_for_clusterfuzz.py:89: filemode='w+') Why is this w+? https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... tools/findit/findit_for_crash.py:531: def ParseCrashComponents(main_stack, top_n_frames=3): I discussed this with Shuotao a bit and we think it might not be necessary to limit this. You could check for all of the components and ignore ones that did not roll in the regression range provided. I think for most cases, this should be fine as-is. You could clean this up in a separate CL (probably prioritized under getting Android and Syzyasan working).
The CQ bit was checked by jeun@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeun@chromium.org/468823003/140001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Hi, I have addressed the review. I will wait until someone can LGTM and commit. I will also create another CL with syzyasan support as soons as this is committed. https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... File tools/findit/findit_for_clusterfuzz.py (right): https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... tools/findit/findit_for_clusterfuzz.py:85: if 'android' in build_type or 'syzyasan' in build_type: On 2014/08/19 19:52:12, mbarbella wrote: > Is there any reason that android needs to be skipped at the moment? I discussed > this with Shuotao briefly and he thinks it looks like it should work since there > aren't any major differences in the stacks. > > If there isn't a specific issue that was blocking this, please just enable it. Done. https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... tools/findit/findit_for_clusterfuzz.py:89: filemode='w+') On 2014/08/19 19:52:12, mbarbella wrote: > Why is this w+? It creates errors.log file if it is not already there. Should I assume that it is already there? I changed it to 'w'. https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... File tools/findit/findit_for_crash.py (right): https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... tools/findit/findit_for_crash.py:531: def ParseCrashComponents(main_stack, top_n_frames=3): On 2014/08/19 19:52:12, mbarbella wrote: > I discussed this with Shuotao a bit and we think it might not be necessary to > limit this. You could check for all of the components and ignore ones that did > not roll in the regression range provided. > > I think for most cases, this should be fine as-is. You could clean this up in a > separate CL (probably prioritized under getting Android and Syzyasan working). Done.
On 2014/08/19 21:37:43, jeun wrote: > Hi, > > I have addressed the review. I will wait until someone can LGTM and commit. I > will also create another CL with syzyasan support as soons as this is committed. > > https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... > File tools/findit/findit_for_clusterfuzz.py (right): > > https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... > tools/findit/findit_for_clusterfuzz.py:85: if 'android' in build_type or > 'syzyasan' in build_type: > On 2014/08/19 19:52:12, mbarbella wrote: > > Is there any reason that android needs to be skipped at the moment? I > discussed > > this with Shuotao briefly and he thinks it looks like it should work since > there > > aren't any major differences in the stacks. > > > > If there isn't a specific issue that was blocking this, please just enable it. > > Done. > > https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... > tools/findit/findit_for_clusterfuzz.py:89: filemode='w+') > On 2014/08/19 19:52:12, mbarbella wrote: > > Why is this w+? > > It creates errors.log file if it is not already there. Should I assume that it > is already there? I changed it to 'w'. 'w' will create a new file, 'w+' will do the same but for reading/writing. > > https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... > File tools/findit/findit_for_crash.py (right): > > https://codereview.chromium.org/468823003/diff/120001/tools/findit/findit_for... > tools/findit/findit_for_crash.py:531: def ParseCrashComponents(main_stack, > top_n_frames=3): > On 2014/08/19 19:52:12, mbarbella wrote: > > I discussed this with Shuotao a bit and we think it might not be necessary to > > limit this. You could check for all of the components and ignore ones that did > > not roll in the regression range provided. > > > > I think for most cases, this should be fine as-is. You could clean this up in > a > > separate CL (probably prioritized under getting Android and Syzyasan working). > > Done.
The CQ bit was checked by stgao@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeun@chromium.org/468823003/140001
Message was sent while issue was closed.
Committed patchset #6 (140001) as 290959 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
