| OLD | NEW |
| 1 # Copyright 2014 The Chromium Authors. All rights reserved. | 1 # Copyright 2014 The Chromium Authors. All rights reserved. |
| 2 # Use of this source code is governed by a BSD-style license that can be | 2 # Use of this source code is governed by a BSD-style license that can be |
| 3 # found in the LICENSE file. | 3 # found in the LICENSE file. |
| 4 | 4 |
| 5 """Chromium presubmit script for src/extensions/browser. | 5 """Chromium presubmit script for src/extensions/browser. |
| 6 | 6 |
| 7 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts | 7 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts |
| 8 for more details on the presubmit API built into gcl. | 8 for more details on the presubmit API built into gcl. |
| 9 """ | 9 """ |
| 10 | 10 |
| 11 import sys |
| 12 |
| 11 def GetPreferredTryMasters(project, change): | 13 def GetPreferredTryMasters(project, change): |
| 12 return { | 14 return { |
| 13 'tryserver.chromium': { | 15 'tryserver.chromium': { |
| 14 'linux_chromium_chromeos_rel': set(['defaulttests']), | 16 'linux_chromium_chromeos_rel': set(['defaulttests']), |
| 15 } | 17 } |
| 16 } | 18 } |
| 17 | 19 |
| 18 class HistogramValueChecker(object): | 20 def _CreateHistogramValueChecker(input_api, output_api): |
| 19 """Verify that changes to "extension_function_histogram_value.h" are valid. | 21 original_sys_path = sys.path |
| 20 | 22 |
| 21 See comments at the top of the "extension_function_histogram_value.h" file | 23 try: |
| 22 for what are considered valid changes. There are situations where this script | 24 sys.path.append(input_api.os_path.join( |
| 23 gives false positive warnings, i.e. it warns even though the edit is | 25 input_api.PresubmitLocalPath(), '..', '..', 'tools', |
| 24 legitimate. Since the script warns using prompt warnings, the user can always | 26 'strict_enum_value_checker')) |
| 25 choose to continue. The main point is to attract the attention to all | 27 from strict_enum_value_checker import StrictEnumValueChecker |
| 26 (potentially or not) invalid edits. | 28 finally: |
| 29 sys.path = original_sys_path |
| 27 | 30 |
| 28 """ | 31 return StrictEnumValueChecker(input_api, output_api, |
| 29 | 32 start_marker='enum HistogramValue {', end_marker=' // Last entry:', |
| 30 # The name of the file we want to check against | 33 path='extensions/browser/extension_function_histogram_value.h') |
| 31 LOCAL_PATH = "extensions/browser/extension_function_histogram_value.h" | |
| 32 | |
| 33 # The markers we look for in the above source file as delimiters of the enum | |
| 34 # definition. | |
| 35 ENUM_START_MARKER = "enum HistogramValue {" | |
| 36 ENUM_END_MARKER = " // Last entry:" | |
| 37 | |
| 38 def __init__(self, input_api, output_api): | |
| 39 self.input_api = input_api | |
| 40 self.output_api = output_api | |
| 41 self.results = [] | |
| 42 | |
| 43 class EnumRange(object): | |
| 44 """Represents a range of line numbers (1-based)""" | |
| 45 def __init__(self, first_line, last_line): | |
| 46 self.first_line = first_line | |
| 47 self.last_line = last_line | |
| 48 | |
| 49 def Count(self): | |
| 50 return self.last_line - self.first_line + 1 | |
| 51 | |
| 52 def Contains(self, line_num): | |
| 53 return self.first_line <= line_num and line_num <= self.last_line | |
| 54 | |
| 55 def LogInfo(self, message): | |
| 56 self.input_api.logging.info(message) | |
| 57 return | |
| 58 | |
| 59 def LogDebug(self, message): | |
| 60 self.input_api.logging.debug(message) | |
| 61 return | |
| 62 | |
| 63 def ComputeEnumRangeInContents(self, contents): | |
| 64 """Returns an |EnumRange| object representing the line extent of the | |
| 65 HistogramValue enum members in |contents|. The line numbers are 1-based, | |
| 66 compatible with line numbers returned by AffectedFile.ChangeContents(). | |
| 67 |contents| is a list of strings reprenting the lines of a text file. | |
| 68 | |
| 69 If either ENUM_START_MARKER or ENUM_END_MARKER cannot be found in | |
| 70 |contents|, returns None and emits detailed warnings about the problem. | |
| 71 | |
| 72 """ | |
| 73 first_enum_line = 0 | |
| 74 last_enum_line = 0 | |
| 75 line_num = 1 # Line numbers are 1-based | |
| 76 for line in contents: | |
| 77 if line.startswith(self.ENUM_START_MARKER): | |
| 78 first_enum_line = line_num + 1 | |
| 79 elif line.startswith(self.ENUM_END_MARKER): | |
| 80 last_enum_line = line_num | |
| 81 line_num += 1 | |
| 82 | |
| 83 if first_enum_line == 0: | |
| 84 self.EmitWarning("The presubmit script could not find the start of the " | |
| 85 "enum definition (\"%s\"). Did the enum definition " | |
| 86 "change?" % self.ENUM_START_MARKER) | |
| 87 return None | |
| 88 | |
| 89 if last_enum_line == 0: | |
| 90 self.EmitWarning("The presubmit script could not find the end of the " | |
| 91 "enum definition (\"%s\"). Did the enum definition " | |
| 92 "change?" % self.ENUM_END_MARKER) | |
| 93 return None | |
| 94 | |
| 95 if first_enum_line >= last_enum_line: | |
| 96 self.EmitWarning("The presubmit script located the start of the enum " | |
| 97 "definition (\"%s\" at line %d) *after* its end " | |
| 98 "(\"%s\" at line %d). Something is not quite right." | |
| 99 % (self.ENUM_START_MARKER, first_enum_line, | |
| 100 self.ENUM_END_MARKER, last_enum_line)) | |
| 101 return None | |
| 102 | |
| 103 self.LogInfo("Line extent of |HistogramValue| enum definition: " | |
| 104 "first_line=%d, last_line=%d." | |
| 105 % (first_enum_line, last_enum_line)) | |
| 106 return self.EnumRange(first_enum_line, last_enum_line) | |
| 107 | |
| 108 def ComputeEnumRangeInNewFile(self, affected_file): | |
| 109 return self.ComputeEnumRangeInContents(affected_file.NewContents()) | |
| 110 | |
| 111 def GetLongMessage(self): | |
| 112 return str("The file \"%s\" contains the definition of the " | |
| 113 "|HistogramValue| enum which should be edited in specific ways " | |
| 114 "only - *** read the comments at the top of the header file ***" | |
| 115 ". There are changes to the file that may be incorrect and " | |
| 116 "warrant manual confirmation after review. Note that this " | |
| 117 "presubmit script can not reliably report the nature of all " | |
| 118 "types of invalid changes, especially when the diffs are " | |
| 119 "complex. For example, an invalid deletion may be reported " | |
| 120 "whereas the change contains a valid rename." | |
| 121 % self.LOCAL_PATH) | |
| 122 | |
| 123 def EmitWarning(self, message, line_number=None, line_text=None): | |
| 124 """Emits a presubmit prompt warning containing the short message | |
| 125 |message|. |item| is |LOCAL_PATH| with optional |line_number| and | |
| 126 |line_text|. | |
| 127 | |
| 128 """ | |
| 129 if line_number is not None and line_text is not None: | |
| 130 item = "%s(%d): %s" % (self.LOCAL_PATH, line_number, line_text) | |
| 131 elif line_number is not None: | |
| 132 item = "%s(%d)" % (self.LOCAL_PATH, line_number) | |
| 133 else: | |
| 134 item = self.LOCAL_PATH | |
| 135 long_message = self.GetLongMessage() | |
| 136 self.LogInfo(message) | |
| 137 self.results.append( | |
| 138 self.output_api.PresubmitPromptWarning(message, [item], long_message)) | |
| 139 | |
| 140 def CollectRangesInsideEnumDefinition(self, affected_file, | |
| 141 first_line, last_line): | |
| 142 """Returns a list of triplet (line_start, line_end, line_text) of ranges of | |
| 143 edits changes. The |line_text| part is the text at line |line_start|. | |
| 144 Since it used only for reporting purposes, we do not need all the text | |
| 145 lines in the range. | |
| 146 | |
| 147 """ | |
| 148 results = [] | |
| 149 previous_line_number = 0 | |
| 150 previous_range_start_line_number = 0 | |
| 151 previous_range_start_text = "" | |
| 152 | |
| 153 def addRange(): | |
| 154 tuple = (previous_range_start_line_number, | |
| 155 previous_line_number, | |
| 156 previous_range_start_text) | |
| 157 results.append(tuple) | |
| 158 | |
| 159 for line_number, line_text in affected_file.ChangedContents(): | |
| 160 if first_line <= line_number and line_number <= last_line: | |
| 161 self.LogDebug("Line change at line number " + str(line_number) + ": " + | |
| 162 line_text) | |
| 163 # Start a new interval if none started | |
| 164 if previous_range_start_line_number == 0: | |
| 165 previous_range_start_line_number = line_number | |
| 166 previous_range_start_text = line_text | |
| 167 # Add new interval if we reached past the previous one | |
| 168 elif line_number != previous_line_number + 1: | |
| 169 addRange() | |
| 170 previous_range_start_line_number = line_number | |
| 171 previous_range_start_text = line_text | |
| 172 previous_line_number = line_number | |
| 173 | |
| 174 # Add a last interval if needed | |
| 175 if previous_range_start_line_number != 0: | |
| 176 addRange() | |
| 177 return results | |
| 178 | |
| 179 def CheckForFileDeletion(self, affected_file): | |
| 180 """Emits a warning notification if file has been deleted """ | |
| 181 if not affected_file.NewContents(): | |
| 182 self.EmitWarning("The file seems to be deleted in the changelist. If " | |
| 183 "your intent is to really delete the file, the code in " | |
| 184 "PRESUBMIT.py should be updated to remove the " | |
| 185 "|HistogramValueChecker| class."); | |
| 186 return False | |
| 187 return True | |
| 188 | |
| 189 def GetDeletedLinesFromScmDiff(self, affected_file): | |
| 190 """Return a list of of line numbers (1-based) corresponding to lines | |
| 191 deleted from the new source file (if they had been present in it). Note | |
| 192 that if multiple contiguous lines have been deleted, the returned list will | |
| 193 contain contiguous line number entries. To prevent false positives, we | |
| 194 return deleted line numbers *only* from diff chunks which decrease the size | |
| 195 of the new file. | |
| 196 | |
| 197 Note: We need this method because we have access to neither the old file | |
| 198 content nor the list of "delete" changes from the current presubmit script | |
| 199 API. | |
| 200 | |
| 201 """ | |
| 202 results = [] | |
| 203 line_num = 0 | |
| 204 deleting_lines = False | |
| 205 for line in affected_file.GenerateScmDiff().splitlines(): | |
| 206 # Parse the unified diff chunk optional section heading, which looks like | |
| 207 # @@ -l,s +l,s @@ optional section heading | |
| 208 m = self.input_api.re.match( | |
| 209 r'^@@ \-([0-9]+)\,([0-9]+) \+([0-9]+)\,([0-9]+) @@', line) | |
| 210 if m: | |
| 211 old_line_num = int(m.group(1)) | |
| 212 old_size = int(m.group(2)) | |
| 213 new_line_num = int(m.group(3)) | |
| 214 new_size = int(m.group(4)) | |
| 215 line_num = new_line_num | |
| 216 # Return line numbers only from diff chunks decreasing the size of the | |
| 217 # new file | |
| 218 deleting_lines = old_size > new_size | |
| 219 continue | |
| 220 if not line.startswith('-'): | |
| 221 line_num += 1 | |
| 222 if deleting_lines and line.startswith('-') and not line.startswith('--'): | |
| 223 results.append(line_num) | |
| 224 return results | |
| 225 | |
| 226 def CheckForEnumEntryDeletions(self, affected_file): | |
| 227 """Look for deletions inside the enum definition. We currently use a | |
| 228 simple heuristics (not 100% accurate): if there are deleted lines inside | |
| 229 the enum definition, this might be a deletion. | |
| 230 | |
| 231 """ | |
| 232 range_new = self.ComputeEnumRangeInNewFile(affected_file) | |
| 233 if not range_new: | |
| 234 return False | |
| 235 | |
| 236 is_ok = True | |
| 237 for line_num in self.GetDeletedLinesFromScmDiff(affected_file): | |
| 238 if range_new.Contains(line_num): | |
| 239 self.EmitWarning("It looks like you are deleting line(s) from the " | |
| 240 "enum definition. This should never happen.", | |
| 241 line_num) | |
| 242 is_ok = False | |
| 243 return is_ok | |
| 244 | |
| 245 def CheckForEnumEntryInsertions(self, affected_file): | |
| 246 range = self.ComputeEnumRangeInNewFile(affected_file) | |
| 247 if not range: | |
| 248 return False | |
| 249 | |
| 250 first_line = range.first_line | |
| 251 last_line = range.last_line | |
| 252 | |
| 253 # Collect the range of changes inside the enum definition range. | |
| 254 is_ok = True | |
| 255 for line_start, line_end, line_text in \ | |
| 256 self.CollectRangesInsideEnumDefinition(affected_file, | |
| 257 first_line, | |
| 258 last_line): | |
| 259 # The only edit we consider valid is adding 1 or more entries *exactly* | |
| 260 # at the end of the enum definition. Every other edit inside the enum | |
| 261 # definition will result in a "warning confirmation" message. | |
| 262 # | |
| 263 # TODO(rpaquay): We currently cannot detect "renames" of existing entries | |
| 264 # vs invalid insertions, so we sometimes will warn for valid edits. | |
| 265 is_valid_edit = (line_end == last_line - 1) | |
| 266 | |
| 267 self.LogDebug("Edit range in new file at starting at line number %d and " | |
| 268 "ending at line number %d: valid=%s" | |
| 269 % (line_start, line_end, is_valid_edit)) | |
| 270 | |
| 271 if not is_valid_edit: | |
| 272 self.EmitWarning("The change starting at line %d and ending at line " | |
| 273 "%d is *not* located *exactly* at the end of the " | |
| 274 "enum definition. Unless you are renaming an " | |
| 275 "existing entry, this is not a valid changes, as new " | |
| 276 "entries should *always* be added at the end of the " | |
| 277 "enum definition, right before the 'ENUM_BOUNDARY' " | |
| 278 "entry." % (line_start, line_end), | |
| 279 line_start, | |
| 280 line_text) | |
| 281 is_ok = False | |
| 282 return is_ok | |
| 283 | |
| 284 def PerformChecks(self, affected_file): | |
| 285 if not self.CheckForFileDeletion(affected_file): | |
| 286 return | |
| 287 if not self.CheckForEnumEntryDeletions(affected_file): | |
| 288 return | |
| 289 if not self.CheckForEnumEntryInsertions(affected_file): | |
| 290 return | |
| 291 | |
| 292 def ProcessHistogramValueFile(self, affected_file): | |
| 293 self.LogInfo("Start processing file \"%s\"" % affected_file.LocalPath()) | |
| 294 self.PerformChecks(affected_file) | |
| 295 self.LogInfo("Done processing file \"%s\"" % affected_file.LocalPath()) | |
| 296 | |
| 297 def Run(self): | |
| 298 for file in self.input_api.AffectedFiles(include_deletes=True): | |
| 299 if file.LocalPath() == self.LOCAL_PATH: | |
| 300 self.ProcessHistogramValueFile(file) | |
| 301 return self.results | |
| 302 | 34 |
| 303 def CheckChangeOnUpload(input_api, output_api): | 35 def CheckChangeOnUpload(input_api, output_api): |
| 304 results = [] | 36 results = [] |
| 305 results += HistogramValueChecker(input_api, output_api).Run() | 37 results += _CreateHistogramValueChecker(input_api, output_api).Run() |
| 306 results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api) | 38 results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api) |
| 307 return results | 39 return results |
| 308 | 40 |
| 41 def CheckChangeOnCommit(input_api, output_api): |
| 42 return _CreateHistogramValueChecker(input_api, output_api).Run() |
| OLD | NEW |