PRESUBMIT.py 20.3 KB
Newer Older
1 2 3 4 5 6 7 8 9 10 11
# Copyright (c) 2013 The Chromium Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.


"""Top-level presubmit script for Skia.

See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
for more details about the presubmit API built into gcl.
"""

12
import collections
13
import csv
14
import fnmatch
15
import os
16
import re
17
import subprocess
18
import sys
19
import traceback
20

21

22 23
REVERT_CL_SUBJECT_PREFIX = 'Revert '

24 25
SKIA_TREE_STATUS_URL = 'http://skia-tree-status.appspot.com'

26
# Please add the complete email address here (and not just 'xyz@' or 'xyz').
27 28 29 30 31
PUBLIC_API_OWNERS = (
    'reed@chromium.org',
    'reed@google.com',
    'bsalomon@chromium.org',
    'bsalomon@google.com',
rmistry's avatar
rmistry committed
32 33
    'djsollen@chromium.org',
    'djsollen@google.com',
34 35
)

36 37
AUTHORS_FILE_NAME = 'AUTHORS'

38
DOCS_PREVIEW_URL = 'https://skia.org/?cl='
39 40
GOLD_TRYBOT_URL = ('https://gold.skia.org/search2?unt=true'
                   '&query=source_type%3Dgm&master=false&issue=')
41

42
# Path to CQ bots feature is described in https://bug.skia.org/4364
43 44
PATH_PREFIX_TO_EXTRA_TRYBOTS = {
    # pylint: disable=line-too-long
mtklein's avatar
mtklein committed
45
    'cmake/': 'client.skia.compile:Build-Mac-Clang-x86_64-Release-CMake-Trybot,Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot',
46 47 48
    # pylint: disable=line-too-long
    'src/opts/': 'client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot',

49 50 51 52 53
    'include/private/SkAtomics.h': ('client.skia:'
      'Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-TSAN-Trybot,'
      'Test-Ubuntu-GCC-Golo-GPU-GT610-x86_64-Release-TSAN-Trybot'
    ),

54 55 56 57 58 59
    # Below are examples to show what is possible with this feature.
    # 'src/svg/': 'master1:abc;master2:def',
    # 'src/svg/parser/': 'master3:ghi,jkl;master4:mno',
    # 'src/image/SkImage_Base.h': 'master5:pqr,stu;master1:abc1;master2:def',
}

60

61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76
def _CheckChangeHasEol(input_api, output_api, source_file_filter=None):
  """Checks that files end with atleast one \n (LF)."""
  eof_files = []
  for f in input_api.AffectedSourceFiles(source_file_filter):
    contents = input_api.ReadFile(f, 'rb')
    # Check that the file ends in atleast one newline character.
    if len(contents) > 1 and contents[-1:] != '\n':
      eof_files.append(f.LocalPath())

  if eof_files:
    return [output_api.PresubmitPromptWarning(
      'These files should end in a newline character:',
      items=eof_files)]
  return []


77 78 79 80 81 82 83 84 85 86 87
def _PythonChecks(input_api, output_api):
  """Run checks on any modified Python files."""
  pylint_disabled_warnings = (
      'F0401',  # Unable to import.
      'E0611',  # No name in module.
      'W0232',  # Class has no __init__ method.
      'E1002',  # Use of super on an old style class.
      'W0403',  # Relative import used.
      'R0201',  # Method could be a function.
      'E1003',  # Using class name in super.
      'W0613',  # Unused argument.
88
      'W0105',  # String statement has no effect.
89 90 91 92 93 94 95 96 97 98 99 100 101 102
  )
  # Run Pylint on only the modified python files. Unfortunately it still runs
  # Pylint on the whole file instead of just the modified lines.
  affected_python_files = []
  for affected_file in input_api.AffectedSourceFiles(None):
    affected_file_path = affected_file.LocalPath()
    if affected_file_path.endswith('.py'):
      affected_python_files.append(affected_file_path)
  return input_api.canned_checks.RunPylint(
      input_api, output_api,
      disabled_warnings=pylint_disabled_warnings,
      white_list=affected_python_files)


103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138
def _IfDefChecks(input_api, output_api):
  """Ensures if/ifdef are not before includes. See skbug/3362 for details."""
  comment_block_start_pattern = re.compile('^\s*\/\*.*$')
  comment_block_middle_pattern = re.compile('^\s+\*.*')
  comment_block_end_pattern = re.compile('^\s+\*\/.*$')
  single_line_comment_pattern = re.compile('^\s*//.*$')
  def is_comment(line):
    return (comment_block_start_pattern.match(line) or
            comment_block_middle_pattern.match(line) or
            comment_block_end_pattern.match(line) or
            single_line_comment_pattern.match(line))

  empty_line_pattern = re.compile('^\s*$')
  def is_empty_line(line):
    return empty_line_pattern.match(line)

  failing_files = []
  for affected_file in input_api.AffectedSourceFiles(None):
    affected_file_path = affected_file.LocalPath()
    if affected_file_path.endswith('.cpp') or affected_file_path.endswith('.h'):
      f = open(affected_file_path)
      for line in f.xreadlines():
        if is_comment(line) or is_empty_line(line):
          continue
        # The below will be the first real line after comments and newlines.
        if line.startswith('#if 0 '):
          pass
        elif line.startswith('#if ') or line.startswith('#ifdef '):
          failing_files.append(affected_file_path)
        break

  results = []
  if failing_files:
    results.append(
        output_api.PresubmitError(
            'The following files have #if or #ifdef before includes:\n%s\n\n'
139
            'See https://bug.skia.org/3362 for why this should be fixed.' %
140 141 142 143
                '\n'.join(failing_files)))
  return results


borenet's avatar
borenet committed
144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161
def _CopyrightChecks(input_api, output_api, source_file_filter=None):
  results = []
  year_pattern = r'\d{4}'
  year_range_pattern = r'%s(-%s)?' % (year_pattern, year_pattern)
  years_pattern = r'%s(,%s)*,?' % (year_range_pattern, year_range_pattern)
  copyright_pattern = (
      r'Copyright (\([cC]\) )?%s \w+' % years_pattern)

  for affected_file in input_api.AffectedSourceFiles(source_file_filter):
    if 'third_party' in affected_file.LocalPath():
      continue
    contents = input_api.ReadFile(affected_file, 'rb')
    if not re.search(copyright_pattern, contents):
      results.append(output_api.PresubmitError(
          '%s is missing a correct copyright header.' % affected_file))
  return results


162 163 164 165 166 167 168 169 170 171 172
def _ToolFlags(input_api, output_api):
  """Make sure `{dm,nanobench}_flags.py test` passes if modified."""
  results = []
  sources = lambda x: ('dm_flags.py'        in x.LocalPath() or
                       'nanobench_flags.py' in x.LocalPath())
  for f in input_api.AffectedSourceFiles(sources):
    if 0 != subprocess.call(['python', f.LocalPath(), 'test']):
      results.append(output_api.PresubmitError('`python %s test` failed' % f))
  return results


173 174 175 176 177 178 179 180
def _CommonChecks(input_api, output_api):
  """Presubmit checks common to upload and commit."""
  results = []
  sources = lambda x: (x.LocalPath().endswith('.h') or
                       x.LocalPath().endswith('.gypi') or
                       x.LocalPath().endswith('.gyp') or
                       x.LocalPath().endswith('.py') or
                       x.LocalPath().endswith('.sh') or
mtklein's avatar
mtklein committed
181 182 183 184 185
                       x.LocalPath().endswith('.m') or
                       x.LocalPath().endswith('.mm') or
                       x.LocalPath().endswith('.go') or
                       x.LocalPath().endswith('.c') or
                       x.LocalPath().endswith('.cc') or
186 187
                       x.LocalPath().endswith('.cpp'))
  results.extend(
188
      _CheckChangeHasEol(
189
          input_api, output_api, source_file_filter=sources))
190
  results.extend(_PythonChecks(input_api, output_api))
191
  results.extend(_IfDefChecks(input_api, output_api))
borenet's avatar
borenet committed
192 193
  results.extend(_CopyrightChecks(input_api, output_api,
                                  source_file_filter=sources))
194
  results.extend(_ToolFlags(input_api, output_api))
195 196
  return results

197 198

def CheckChangeOnUpload(input_api, output_api):
199 200 201 202 203 204 205 206
  """Presubmit checks for the change on upload.

  The following are the presubmit checks:
  * Check change has one and only one EOL.
  """
  results = []
  results.extend(_CommonChecks(input_api, output_api))
  return results
207 208


209 210 211 212 213 214 215 216 217 218 219 220 221 222 223
def _CheckTreeStatus(input_api, output_api, json_url):
  """Check whether to allow commit.

  Args:
    input_api: input related apis.
    output_api: output related apis.
    json_url: url to download json style status.
  """
  tree_status_results = input_api.canned_checks.CheckTreeIsOpen(
      input_api, output_api, json_url=json_url)
  if not tree_status_results:
    # Check for caution state only if tree is not closed.
    connection = input_api.urllib2.urlopen(json_url)
    status = input_api.json.loads(connection.read())
    connection.close()
224 225 226 227 228
    if ('caution' in status['message'].lower() and
        os.isatty(sys.stdout.fileno())):
      # Display a prompt only if we are in an interactive shell. Without this
      # check the commit queue behaves incorrectly because it considers
      # prompts to be failures.
229 230 231 232 233
      short_text = 'Tree state is: ' + status['general_state']
      long_text = status['message'] + '\n' + json_url
      tree_status_results.append(
          output_api.PresubmitPromptWarning(
              message=short_text, long_text=long_text))
234 235 236 237 238 239 240 241 242 243
  else:
    # Tree status is closed. Put in message about contacting sheriff.
    connection = input_api.urllib2.urlopen(
        SKIA_TREE_STATUS_URL + '/current-sheriff')
    sheriff_details = input_api.json.loads(connection.read())
    if sheriff_details:
      tree_status_results[0]._message += (
          '\n\nPlease contact the current Skia sheriff (%s) if you are trying '
          'to submit a build fix\nand do not know how to submit because the '
          'tree is closed') % sheriff_details['username']
244 245 246
  return tree_status_results


247 248 249 250
def _CheckOwnerIsInAuthorsFile(input_api, output_api):
  results = []
  issue = input_api.change.issue
  if issue and input_api.rietveld:
rmistry's avatar
rmistry committed
251
    issue_properties = input_api.rietveld.get_issue_properties(
252 253 254 255 256 257 258 259 260 261 262 263 264 265 266 267 268 269
        issue=int(issue), messages=False)
    owner_email = issue_properties['owner_email']

    try:
      authors_content = ''
      for line in open(AUTHORS_FILE_NAME):
        if not line.startswith('#'):
          authors_content += line
      email_fnmatches = re.findall('<(.*)>', authors_content)
      for email_fnmatch in email_fnmatches:
        if fnmatch.fnmatch(owner_email, email_fnmatch):
          # Found a match, the user is in the AUTHORS file break out of the loop
          break
      else:
        results.append(
          output_api.PresubmitError(
            'The email %s is not in Skia\'s AUTHORS file.\n'
            'Issue owner, this CL must include an addition to the Skia AUTHORS '
rmistry's avatar
rmistry committed
270
            'file.'
rmistry's avatar
rmistry committed
271
            % owner_email))
272 273 274 275 276 277 278 279
    except IOError:
      # Do not fail if authors file cannot be found.
      traceback.print_exc()
      input_api.logging.error('AUTHORS file not found!')

  return results


280 281 282 283 284 285 286 287
def _CheckLGTMsForPublicAPI(input_api, output_api):
  """Check LGTMs for public API changes.

  For public API files make sure there is an LGTM from the list of owners in
  PUBLIC_API_OWNERS.
  """
  results = []
  requires_owner_check = False
288 289
  for affected_file in input_api.AffectedFiles():
    affected_file_path = affected_file.LocalPath()
290
    file_path, file_ext = os.path.splitext(affected_file_path)
291
    # We only care about files that end in .h and are under the top-level
292 293 294 295
    # include dir, but not include/private.
    if (file_ext == '.h' and
        'include' == file_path.split(os.path.sep)[0] and
        'private' not in file_path):
296 297 298 299 300 301 302 303 304 305
      requires_owner_check = True

  if not requires_owner_check:
    return results

  lgtm_from_owner = False
  issue = input_api.change.issue
  if issue and input_api.rietveld:
    issue_properties = input_api.rietveld.get_issue_properties(
        issue=int(issue), messages=True)
306 307 308
    if re.match(REVERT_CL_SUBJECT_PREFIX, issue_properties['subject'], re.I):
      # It is a revert CL, ignore the public api owners check.
      return results
309

rmistry's avatar
rmistry committed
310
    if issue_properties['cq_dry_run']:
rmistry's avatar
rmistry committed
311
      # Ignore public api owners check for dry run CLs since they are not
312 313 314
      # going to be committed.
      return results

315 316 317 318 319 320 321 322 323
    match = re.search(r'^TBR=(.*)$', issue_properties['description'], re.M)
    if match:
      tbr_entries = match.group(1).strip().split(',')
      for owner in PUBLIC_API_OWNERS:
        if owner in tbr_entries or owner.split('@')[0] in tbr_entries:
          # If an owner is specified in the TBR= line then ignore the public
          # api owners check.
          return results

324 325 326 327 328 329 330 331 332 333 334
    if issue_properties['owner_email'] in PUBLIC_API_OWNERS:
      # An owner created the CL that is an automatic LGTM.
      lgtm_from_owner = True

    messages = issue_properties.get('messages')
    if messages:
      for message in messages:
        if (message['sender'] in PUBLIC_API_OWNERS and
            'lgtm' in message['text'].lower()):
          # Found an lgtm in a message from an owner.
          lgtm_from_owner = True
335
          break
336

337 338 339
  if not lgtm_from_owner:
    results.append(
        output_api.PresubmitError(
340 341 342 343 344 345
            "If this CL adds to or changes Skia's public API, you need an LGTM "
            "from any of %s.  If this CL only removes from or doesn't change "
            "Skia's public API, please add a short note to the CL saying so "
            "and add one of those reviewers on a TBR= line.  If you don't know "
            "if this CL affects Skia's public API, treat it like it does."
            % str(PUBLIC_API_OWNERS)))
346 347 348
  return results


349 350 351 352
def PostUploadHook(cl, change, output_api):
  """git cl upload will call this hook after the issue is created/modified.

  This hook does the following:
353
  * Adds a link to the CL's Gold trybot results.
354 355
  * Adds a link to preview docs changes if there are any docs changes in the CL.
  * Adds 'NOTRY=true' if the CL contains only docs changes.
356 357 358 359
  * Adds 'NOTREECHECKS=true' for non master branch changes since they do not
    need to be gated on the master branch's tree.
  * Adds 'NOTRY=true' for non master branch changes since trybots do not yet
    work on them.
360 361
  * Adds 'NOPRESUBMIT=true' for non master branch changes since those don't
    run the presubmit checks.
362
  * Adds extra trybots for the paths defined in PATH_TO_EXTRA_TRYBOTS.
363 364 365 366 367 368 369 370 371 372 373 374 375 376 377 378 379 380 381 382 383
  """

  results = []
  atleast_one_docs_change = False
  all_docs_changes = True
  for affected_file in change.AffectedFiles():
    affected_file_path = affected_file.LocalPath()
    file_path, _ = os.path.splitext(affected_file_path)
    if 'site' == file_path.split(os.path.sep)[0]:
      atleast_one_docs_change = True
    else:
      all_docs_changes = False
    if atleast_one_docs_change and not all_docs_changes:
      break

  issue = cl.issue
  rietveld_obj = cl.RpcServer()
  if issue and rietveld_obj:
    original_description = rietveld_obj.get_description(issue)
    new_description = original_description

384 385 386 387 388 389 390 391 392
    # Add GOLD_TRYBOT_URL if it does not exist yet.
    if not re.search(r'^GOLD_TRYBOT_URL=', new_description, re.M | re.I):
      new_description += '\nGOLD_TRYBOT_URL= %s%s' % (GOLD_TRYBOT_URL, issue)
      results.append(
          output_api.PresubmitNotifyResult(
              'Added link to Gold trybot runs to the CL\'s description.\n'
              'Note: Results may take sometime to be populated after trybots '
              'complete.'))

393 394 395
    # If the change includes only doc changes then add NOTRY=true in the
    # CL's description if it does not exist yet.
    if all_docs_changes and not re.search(
396
        r'^NOTRY=true$', new_description, re.M | re.I):
397 398 399 400 401 402 403 404 405
      new_description += '\nNOTRY=true'
      results.append(
          output_api.PresubmitNotifyResult(
              'This change has only doc changes. Automatically added '
              '\'NOTRY=true\' to the CL\'s description'))

    # If there is atleast one docs change then add preview link in the CL's
    # description if it does not already exist there.
    if atleast_one_docs_change and not re.search(
406
        r'^DOCS_PREVIEW=.*', new_description, re.M | re.I):
407 408 409 410 411 412 413
      # Automatically add a link to where the docs can be previewed.
      new_description += '\nDOCS_PREVIEW= %s%s' % (DOCS_PREVIEW_URL, issue)
      results.append(
          output_api.PresubmitNotifyResult(
              'Automatically added a link to preview the docs changes to the '
              'CL\'s description'))

414 415 416 417 418 419 420 421 422 423 424 425 426 427 428 429 430 431 432 433
    # If the target ref is not master then add NOTREECHECKS=true and NOTRY=true
    # to the CL's description if it does not already exist there.
    target_ref = rietveld_obj.get_issue_properties(issue, False).get(
        'target_ref', '')
    if target_ref != 'refs/heads/master':
      if not re.search(
          r'^NOTREECHECKS=true$', new_description, re.M | re.I):
        new_description += "\nNOTREECHECKS=true"
        results.append(
            output_api.PresubmitNotifyResult(
                'Branch changes do not need to rely on the master branch\'s '
                'tree status. Automatically added \'NOTREECHECKS=true\' to the '
                'CL\'s description'))
      if not re.search(
          r'^NOTRY=true$', new_description, re.M | re.I):
        new_description += "\nNOTRY=true"
        results.append(
            output_api.PresubmitNotifyResult(
                'Trybots do not yet work for non-master branches. '
                'Automatically added \'NOTRY=true\' to the CL\'s description'))
434 435 436 437 438 439
      if not re.search(
          r'^NOPRESUBMIT=true$', new_description, re.M | re.I):
        new_description += "\nNOPRESUBMIT=true"
        results.append(
            output_api.PresubmitNotifyResult(
                'Branch changes do not run the presubmit checks.'))
440

441 442 443 444 445 446 447 448 449 450 451 452 453 454 455 456
    # Automatically set CQ_EXTRA_TRYBOTS if any of the changed files here begin
    # with the paths of interest.
    cq_master_to_trybots = collections.defaultdict(set)
    for affected_file in change.AffectedFiles():
      affected_file_path = affected_file.LocalPath()
      for path_prefix, extra_bots in PATH_PREFIX_TO_EXTRA_TRYBOTS.iteritems():
        if affected_file_path.startswith(path_prefix):
          results.append(
              output_api.PresubmitNotifyResult(
                  'Your CL modifies the path %s.\nAutomatically adding %s to '
                  'the CL description.' % (affected_file_path, extra_bots)))
          _MergeCQExtraTrybotsMaps(
              cq_master_to_trybots, _GetCQExtraTrybotsMap(extra_bots))
    if cq_master_to_trybots:
      new_description = _AddCQExtraTrybotsToDesc(
          cq_master_to_trybots, new_description)
457

458 459 460 461 462 463 464
    # If the description has changed update it.
    if new_description != original_description:
      rietveld_obj.update_description(issue, new_description)

    return results


465 466 467 468 469 470 471 472 473 474 475 476 477 478 479 480 481 482 483 484 485 486 487 488 489 490 491 492 493 494 495 496 497 498 499 500 501 502 503 504 505 506 507
def _AddCQExtraTrybotsToDesc(cq_master_to_trybots, description):
  """Adds the specified master and trybots to the CQ_EXTRA_TRYBOTS keyword.

  If the keyword already exists in the description then it appends to it only
  if the specified values do not already exist.
  If the keyword does not exist then it creates a new section in the
  description.
  """
  match = re.search(r'^CQ_EXTRA_TRYBOTS=(.*)$', description, re.M | re.I)
  if match:
    original_trybots_map = _GetCQExtraTrybotsMap(match.group(1))
    _MergeCQExtraTrybotsMaps(cq_master_to_trybots, original_trybots_map)
    new_description = description.replace(
        match.group(0), _GetCQExtraTrybotsStr(cq_master_to_trybots))
  else:
    new_description = description + "\n%s" % (
        _GetCQExtraTrybotsStr(cq_master_to_trybots))
  return new_description


def _MergeCQExtraTrybotsMaps(dest_map, map_to_be_consumed):
  """Merges two maps of masters to trybots into one."""
  for master, trybots in map_to_be_consumed.iteritems():
    dest_map[master].update(trybots)
  return dest_map


def _GetCQExtraTrybotsMap(cq_extra_trybots_str):
  """Parses the CQ_EXTRA_TRYBOTS str and returns a map of masters to trybots."""
  cq_master_to_trybots = collections.defaultdict(set)
  for section in cq_extra_trybots_str.split(';'):
    if section:
      master, bots = section.split(':')
      cq_master_to_trybots[master].update(bots.split(','))
  return cq_master_to_trybots


def _GetCQExtraTrybotsStr(cq_master_to_trybots):
  """Constructs the CQ_EXTRA_TRYBOTS str from a map of masters to trybots."""
  sections = []
  for master, trybots in cq_master_to_trybots.iteritems():
    sections.append('%s:%s' % (master, ','.join(trybots)))
  return 'CQ_EXTRA_TRYBOTS=%s' % ';'.join(sections)
508 509


510 511 512 513
def CheckChangeOnCommit(input_api, output_api):
  """Presubmit checks for the change on commit.

  The following are the presubmit checks:
514
  * Check change has one and only one EOL.
515 516 517
  * Ensures that the Skia tree is open in
    http://skia-tree-status.appspot.com/. Shows a warning if it is in 'Caution'
    state and an error if it is in 'Closed' state.
518 519
  """
  results = []
520
  results.extend(_CommonChecks(input_api, output_api))
521
  results.extend(
522
      _CheckTreeStatus(input_api, output_api, json_url=(
523
          SKIA_TREE_STATUS_URL + '/banner-status?format=json')))
524
  results.extend(_CheckLGTMsForPublicAPI(input_api, output_api))
525
  results.extend(_CheckOwnerIsInAuthorsFile(input_api, output_api))
526
  return results