Fix python linter errors and add linter checks to CI

Internal CI systems and the new GitHub CI system were out of sync,
with the external system not doing any linting.  Further, the internal
system was using an internal-only linter for Python.

This creates a script for Python linting based on the open-source
pylint tool, checks in the Google Style Guide's pylintrc file, creates
a custom action for linting and adds it to the existing workflows,
fixes pre-existing linter errors in Python scripts, and updates pylint
overrides.

b/190743862

Change-Id: Iff1f5d4690b32479af777ded0834c31c2161bd10

.github/workflows/README.md

@@ -1,6 +1,10 @@
 # GitHub Actions CI
 
 ## Actions
+ - `custom-actions/lint-packager`:
+   Lints Shaka Packager.  You must pass `fetch-depth: 2` to `actions/checkout`
+   in order to provide enough history for the linter to tell which files have
+   changed.
  - `custom-actions/build-packager`:
    Builds Shaka Packager.  Leaves build artifacts in the "artifacts" folder.
    Requires OS-dependent and build-dependent inputs.

.github/workflows/build_and_test.yaml

@@ -16,7 +16,26 @@ on:
         required: False
 
 jobs:
+  lint:
+    name: Lint
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v2
+        with:
+          path: src
+          ref: ${{ github.event.inputs.ref || github.ref }}
+          # This makes the merge base available for the C++ linter, so that it
+          # can tell which files have changed.
+          fetch-depth: 2
+
+      - name: Lint
+        uses: ./src/.github/workflows/custom-actions/lint-packager
+
   build_and_test:
+    # Doesn't really "need" it, but let's not waste time on an expensive matrix
+    # build step just to cancel it because of a linter error.
+    needs: lint
     strategy:
       matrix:
         os: ["ubuntu-latest", "macos-latest", "windows-latest"]

.github/workflows/custom-actions/lint-packager/action.yaml

@@ -0,0 +1,33 @@
+name: Lint Shaka Packager
+
+description: |
+  A reusable action to lint Shaka Packager source.
+  When checking out source, you must use 'fetch-depth: 2' in actions/checkout,
+  or else the linter won't have another revision to compare to.
+
+runs:
+  using: composite
+  steps:
+    - name: Lint
+      shell: bash
+      run: |
+        cd src/
+        echo "::group::Installing git-clang-format"
+        wget https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/git-clang-format
+        sudo install -m 755 git-clang-format /usr/local/bin/git-clang-format
+        rm git-clang-format
+        echo "::endgroup::"
+        echo "::group::Check clang-format for C++ sources"
+        # NOTE: --binary forces use of global clang-format (which works) instead
+        # of depot_tools clang-format (which doesn't).
+        # NOTE: Must use base.sha instead of base.ref, since we don't have
+        # access to the branch name that base.ref would give us.
+        # NOTE: Must also use fetch-depth: 2 in actions/checkout to have access
+        # to the base ref for comparison.
+        packager/tools/git/check_formatting.py \
+            --binary /usr/bin/clang-format \
+            ${{ github.event.pull_request.base.sha || 'HEAD^' }}
+        echo "::endgroup::"
+        echo "::group::Check pylint for Python sources"
+        packager/tools/git/check_pylint.py
+        echo "::endgroup::"

.github/workflows/github_release.yaml

@@ -91,8 +91,25 @@ jobs:
           body_path: RELEASE_NOTES.md
           draft: true
 
+  lint:
+    needs: setup
+    name: Lint
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v2
+        with:
+          path: src
+          ref: ${{ needs.setup.outputs.tag }}
+          # This makes the merge base available for the C++ linter, so that it
+          # can tell which files have changed.
+          fetch-depth: 2
+
+      - name: Lint
+        uses: ./src/.github/workflows/custom-actions/lint-packager
+
   build_and_test:
-    needs: [setup, draft_release]
+    needs: [setup, lint, draft_release]
     strategy:
       matrix:
         os: ["ubuntu-latest", "macos-latest", "windows-latest"]

gyp_packager.py

@@ -39,7 +39,7 @@
 src_dir = os.path.join(checkout_dir, 'packager')
 
 # Workaround the dynamic path.
-# pylint: disable=g-import-not-at-top,g-bad-import-order
+# pylint: disable=wrong-import-position
 
 sys.path.insert(0, os.path.join(src_dir, 'build'))
 import gyp_helper

kokoro/windows/build.bat

@@ -1,6 +1,7 @@
 :: Copyright 2017 Google Inc. All Rights Reserved.
 set ROOTDIR=%cd%
 set PACKAGERDIR=%ROOTDIR%\git\src
+set PATH=c:\Python35;%PATH%
 
 set GYP_DEFINES="target_arch=%PLATFORM%"
 if "%PLATFORM%"=="x64" (

packager/app/test/packager_test.py

@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/python3
 #
 # Copyright 2014 Google Inc. All Rights Reserved.
 #
@@ -131,23 +131,19 @@ def _DiffDir(self, out_dir, gold_dir):
       actual_file = os.path.join(out_dir, diff_file)
       expected_file = os.path.join(gold_dir, diff_file)
 
-      output, error = self._GitDiff(expected_file, actual_file)
+      output = self._GitDiff(expected_file, actual_file)
 
       # If this is an MP4 file, get a better looking diff.
-      if ((output or error) and
+      if (output and
           os.path.splitext(actual_file)[1] in {'.mp4', '.m4s'}):
-        new_output, new_error = self._Mp4Diff(
+        new_output = self._Mp4Diff(
             out_dir, expected_file, actual_file)
 
         output = new_output or output
-        error = new_error or error
 
       if output:
         failure_messages += [output.decode('utf8')]
 
-      if error:
-        failure_messages += [error.decode('utf8')]
-
     if self._exact:
       for diff_file in self._allowed_diff_files:
         if diff_file not in diff.diff_files:
@@ -166,8 +162,7 @@ def _GitDiff(self, file_a, file_b):
         file_a,
         file_b
     ]
-    p = subprocess.Popen(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
-    return p.communicate()
+    return subprocess.check_output(cmd)
 
   def _Mp4Diff(self, out_dir, file_a, file_b):
     dump_a = os.path.join(out_dir, os.path.basename(file_a) + '.dump.expected')
@@ -262,7 +257,7 @@ def GetSegmentedExtension(base_extension):
 class PackagerAppTest(unittest.TestCase):
 
   def setUp(self):
-    super(PackagerAppTest, self).setUp()
+    super().setUp()
     self.packager = packager_app.PackagerApp()
     self.tmp_dir = tempfile.mkdtemp()
     self.test_data_dir = os.path.join(test_env.SRC_DIR, 'packager', 'media',
@@ -294,7 +289,7 @@ def setUp(self):
   def tearDown(self):
     if test_env.options.remove_temp_files_after_test:
       shutil.rmtree(self.tmp_dir)
-    super(PackagerAppTest, self).tearDown()
+    super().tearDown()
 
   def _GetStream(self,
                  descriptor,

packager/tools/git/check_pylint.py

@@ -0,0 +1,43 @@
+#!/usr/bin/python3
+"""Lints all Python sources in the repo, excluding third-party code."""
+
+import os
+import subprocess
+
+import pylint.lint
+
+def ShouldLintFile(path):
+  """Returns True if this path should be linted."""
+  excluded_folders = [
+      'third_party',
+      'protoc_wrapper',
+      'ycm_extra_conf',
+  ]
+
+  if (not path.endswith('.py') or
+      any(f in path for f in excluded_folders)):
+    return False
+
+  return True
+
+def GetPyFileList():
+  """Yield the paths of Python source files that should be linted."""
+  output = subprocess.check_output(['git', 'ls-files'], text=True)
+
+  for path in output.split('\n'):
+    if ShouldLintFile(path):
+      yield path
+
+def main():
+  """Lint Python source files.
+
+  Pylint will exit with a non-zero status if there are any failures."""
+  dir_name = os.path.dirname(__file__)
+  rc_path = os.path.join(dir_name, 'pylintrc')
+
+  py_files = list(GetPyFileList())
+  # Run will call sys.exit, so no explicit call to sys.exit is used here.
+  pylint.lint.Run(['--rcfile={}'.format(rc_path)] + py_files)
+
+if __name__ == '__main__':
+  main()