Add fixes & test for isConfigTexturable and isConfigRenderable
[platform/upstream/libSkiaSharp.git] / PRESUBMIT.py
index 92b686e..768892a 100644 (file)
@@ -31,6 +31,8 @@ PUBLIC_API_OWNERS = (
     'bsalomon@google.com',
     'djsollen@chromium.org',
     'djsollen@google.com',
+    'hcm@chromium.org',
+    'hcm@google.com',
 )
 
 AUTHORS_FILE_NAME = 'AUTHORS'
@@ -40,14 +42,11 @@ GOLD_TRYBOT_URL = 'https://gold.skia.org/search?issue='
 
 # Path to CQ bots feature is described in https://bug.skia.org/4364
 PATH_PREFIX_TO_EXTRA_TRYBOTS = {
-    # pylint: disable=line-too-long
-    'cmake/': 'master.client.skia.compile:Build-Mac-Clang-x86_64-Release-CMake-Trybot,Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot',
-    # pylint: disable=line-too-long
-    'src/opts/': 'master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot',
-
-    'include/private/SkAtomics.h': ('master.client.skia:'
-      'Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-TSAN-Trybot,'
-      'Test-Ubuntu-GCC-Golo-GPU-GT610-x86_64-Release-TSAN-Trybot'
+    'src/opts/':
+        'skia.primary:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD',
+    'include/private/SkAtomics.h': ('skia.primary:'
+      'Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Release-TSAN,'
+      'Test-Ubuntu-Clang-Golo-GPU-GT610-x86_64-Release-TSAN-Trybot'
     ),
 
     # Below are examples to show what is possible with this feature.
@@ -169,15 +168,14 @@ def _ToolFlags(input_api, output_api):
   return results
 
 
-def _RecipeSimulationTest(input_api, output_api):
-  """Run the recipe simulation test."""
+def _InfraTests(input_api, output_api):
+  """Run the infra tests."""
   results = []
   if not any(f.LocalPath().startswith('infra')
              for f in input_api.AffectedFiles()):
     return results
 
-  recipes_py = os.path.join('infra', 'bots', 'recipes.py')
-  cmd = ['python', recipes_py, 'simulation_test']
+  cmd = ['python', os.path.join('infra', 'bots', 'infra_tests.py')]
   try:
     subprocess.check_output(cmd)
   except subprocess.CalledProcessError as e:
@@ -185,14 +183,17 @@ def _RecipeSimulationTest(input_api, output_api):
         '`%s` failed:\n%s' % (' '.join(cmd), e.output)))
   return results
 
+
 def _CheckGNFormatted(input_api, output_api):
   """Make sure any .gn files we're changing have been formatted."""
   results = []
   for f in input_api.AffectedFiles():
-    if not f.LocalPath().endswith('.gn'):
+    if (not f.LocalPath().endswith('.gn') and
+        not f.LocalPath().endswith('.gni')):
       continue
 
-    cmd = ['gn', 'format', '--dry-run', f.LocalPath()]
+    gn = 'gn.bat' if 'win32' in sys.platform else 'gn'
+    cmd = [gn, 'format', '--dry-run', f.LocalPath()]
     try:
       subprocess.check_output(cmd)
     except subprocess.CalledProcessError:
@@ -206,8 +207,6 @@ 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
                        x.LocalPath().endswith('.m') or
@@ -236,8 +235,9 @@ def CheckChangeOnUpload(input_api, output_api):
   results = []
   results.extend(_CommonChecks(input_api, output_api))
   # Run on upload, not commit, since the presubmit bot apparently doesn't have
-  # coverage installed.
-  results.extend(_RecipeSimulationTest(input_api, output_api))
+  # coverage or Go installed.
+  results.extend(_InfraTests(input_api, output_api))
+
   results.extend(_CheckGNFormatted(input_api, output_api))
   return results
 
@@ -316,11 +316,20 @@ class CodeReview(object):
     else:
       return self._rietveld_properties['cq_dry_run']
 
+  def GetReviewers(self):
+    if self._gerrit:
+      code_review_label = (
+          self._gerrit.GetChangeInfo(self._issue)['labels']['Code-Review'])
+      return [r['email'] for r in code_review_label.get('all', [])]
+    else:
+      return self._rietveld_properties['reviewers']
+
   def GetApprovers(self):
     approvers = []
     if self._gerrit:
-      for m in self._gerrit.GetChangeInfo(
-                   self._issue)['labels']['Code-Review']['all']:
+      code_review_label = (
+          self._gerrit.GetChangeInfo(self._issue)['labels']['Code-Review'])
+      for m in code_review_label.get('all', []):
         if m.get("value") == 1:
           approvers.append(m["email"])
     else:
@@ -395,14 +404,22 @@ def _CheckLGTMsForPublicAPI(input_api, output_api):
       # going to be committed.
       return results
 
-    match = re.search(r'^TBR=(.*)$', cr.GetDescription(), 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.
+    if input_api.gerrit:
+      for reviewer in cr.GetReviewers():
+        if reviewer in PUBLIC_API_OWNERS:
+          # If an owner is specified as an reviewer in Gerrit then ignore the
+          # public api owners check.
           return results
+    else:
+      match = re.search(r'^TBR=(.*)$', cr.GetDescription(), re.M)
+      if match:
+        tbr_section = match.group(1).strip().split(' ')[0]
+        tbr_entries = tbr_section.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
 
     if cr.GetOwnerEmail() in PUBLIC_API_OWNERS:
       # An owner created the CL that is an automatic LGTM.
@@ -419,10 +436,11 @@ def _CheckLGTMsForPublicAPI(input_api, output_api):
         output_api.PresubmitError(
             "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)))
+            "Skia's public API, please add a short note to the CL saying so. "
+            "Add one of the owners as a reviewer to your CL. For Rietveld CLs "
+            "you also need to add one of those owners 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)))
   return results
 
 
@@ -430,7 +448,6 @@ def PostUploadHook(cl, change, output_api):
   """git cl upload will call this hook after the issue is created/modified.
 
   This hook does the following:
-  * Adds a link to the CL's Gold trybot results.
   * 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.
   * Adds 'NOTREECHECKS=true' for non master branch changes since they do not
@@ -443,12 +460,6 @@ def PostUploadHook(cl, change, output_api):
   """
 
   results = []
-  if cl.IsGerrit():
-    results.append(
-        output_api.PresubmitNotifyResult(
-            'Post upload hooks are not yet supported for Gerrit CLs'))
-    return results
-
   atleast_one_docs_change = False
   all_docs_changes = True
   for affected_file in change.AffectedFiles():
@@ -462,25 +473,14 @@ def PostUploadHook(cl, change, output_api):
       break
 
   issue = cl.issue
-  rietveld_obj = cl.RpcServer()
-  if issue and rietveld_obj:
-    original_description = rietveld_obj.get_description(issue)
-    new_description = original_description
-
-    # 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.'))
+  if issue:
+    original_description_lines, footers = cl.GetDescriptionFooters()
+    new_description_lines = list(original_description_lines)
 
     # 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(
-        r'^NOTRY=true$', new_description, re.M | re.I):
-      new_description += '\nNOTRY=true'
+    if all_docs_changes and 'NOTRY=true' not in new_description_lines:
+      new_description_lines.append('NOTRY=true')
       results.append(
           output_api.PresubmitNotifyResult(
               'This change has only doc changes. Automatically added '
@@ -488,10 +488,11 @@ def PostUploadHook(cl, change, output_api):
 
     # 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(
-        r'^DOCS_PREVIEW=.*', new_description, re.M | re.I):
+    docs_preview_line = 'DOCS_PREVIEW= %s%s' % (DOCS_PREVIEW_URL, issue)
+    if (atleast_one_docs_change and
+        docs_preview_line not in new_description_lines):
       # Automatically add a link to where the docs can be previewed.
-      new_description += '\nDOCS_PREVIEW= %s%s' % (DOCS_PREVIEW_URL, issue)
+      new_description_lines.append(docs_preview_line)
       results.append(
           output_api.PresubmitNotifyResult(
               'Automatically added a link to preview the docs changes to the '
@@ -499,27 +500,23 @@ def PostUploadHook(cl, change, output_api):
 
     # 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"
+    target_ref = cl.GetRemoteBranch()[1]
+    if target_ref != 'refs/remotes/origin/master':
+      if 'NOTREECHECKS=true' not in new_description_lines:
+        new_description_lines.append('NOTREECHECKS=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"
+      if 'NOTRY=true' not in new_description_lines:
+        new_description_lines.append('NOTRY=true')
         results.append(
             output_api.PresubmitNotifyResult(
                 'Trybots do not yet work for non-master branches. '
                 'Automatically added \'NOTRY=true\' to the CL\'s description'))
-      if not re.search(
-          r'^NOPRESUBMIT=true$', new_description, re.M | re.I):
-        new_description += "\nNOPRESUBMIT=true"
+      if 'NOPRESUBMIT=true' not in new_description_lines:
+        new_description_lines.append('NOPRESUBMIT=true')
         results.append(
             output_api.PresubmitNotifyResult(
                 'Branch changes do not run the presubmit checks.'))
@@ -538,17 +535,18 @@ def PostUploadHook(cl, change, output_api):
           _MergeCQExtraTrybotsMaps(
               cq_master_to_trybots, _GetCQExtraTrybotsMap(extra_bots))
     if cq_master_to_trybots:
-      new_description = _AddCQExtraTrybotsToDesc(
-          cq_master_to_trybots, new_description)
+      _AddCQExtraTrybotsToDesc(cq_master_to_trybots, new_description_lines)
 
     # If the description has changed update it.
-    if new_description != original_description:
-      rietveld_obj.update_description(issue, new_description)
+    if new_description_lines != original_description_lines:
+      # Add a new line separating the new contents from the old contents.
+      new_description_lines.insert(len(original_description_lines), '')
+      cl.UpdateDescriptionFooters(new_description_lines, footers)
 
     return results
 
 
-def _AddCQExtraTrybotsToDesc(cq_master_to_trybots, description):
+def _AddCQExtraTrybotsToDesc(cq_master_to_trybots, description_lines):
   """Adds the specified master and trybots to the CQ_INCLUDE_TRYBOTS keyword.
 
   If the keyword already exists in the description then it appends to it only
@@ -556,16 +554,21 @@ def _AddCQExtraTrybotsToDesc(cq_master_to_trybots, description):
   If the keyword does not exist then it creates a new section in the
   description.
   """
-  match = re.search(r'^CQ_INCLUDE_TRYBOTS=(.*)$', description, re.M | re.I)
-  if match:
-    original_trybots_map = _GetCQExtraTrybotsMap(match.group(1))
+  found = None
+  foundIdx = -1
+  for idx, line in enumerate(description_lines):
+    if line.startswith('CQ_INCLUDE_TRYBOTS'):
+      found = line
+      foundIdx = idx
+
+  if found:
+    original_trybots_map = _GetCQExtraTrybotsMap(found)
     _MergeCQExtraTrybotsMaps(cq_master_to_trybots, original_trybots_map)
-    new_description = description.replace(
-        match.group(0), _GetCQExtraTrybotsStr(cq_master_to_trybots))
+    new_line = _GetCQExtraTrybotsStr(cq_master_to_trybots)
+    if new_line != found:
+      description_lines[foundIdx] = new_line
   else:
-    new_description = description + "\n%s" % (
-        _GetCQExtraTrybotsStr(cq_master_to_trybots))
-  return new_description
+    description_lines.append(_GetCQExtraTrybotsStr(cq_master_to_trybots))
 
 
 def _MergeCQExtraTrybotsMaps(dest_map, map_to_be_consumed):