diff --git a/.github/workflows/README.md b/.github/workflows/README.md index acae72f827..41602d3e76 100644 --- a/.github/workflows/README.md +++ b/.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. diff --git a/.github/workflows/build_and_test.yaml b/.github/workflows/build_and_test.yaml index afa384aa63..70edf57e6a 100644 --- a/.github/workflows/build_and_test.yaml +++ b/.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"] diff --git a/.github/workflows/custom-actions/lint-packager/action.yaml b/.github/workflows/custom-actions/lint-packager/action.yaml new file mode 100644 index 0000000000..e506b427d6 --- /dev/null +++ b/.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::" diff --git a/.github/workflows/github_release.yaml b/.github/workflows/github_release.yaml index 65a2c9979b..e28fb9a6ba 100644 --- a/.github/workflows/github_release.yaml +++ b/.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"] diff --git a/gyp_packager.py b/gyp_packager.py index 9145ad40de..801be26824 100755 --- a/gyp_packager.py +++ b/gyp_packager.py @@ -39,7 +39,7 @@ checkout_dir = os.path.dirname(os.path.realpath(__file__)) 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 diff --git a/kokoro/windows/build.bat b/kokoro/windows/build.bat index 7d3bcb8fba..65314ba548 100644 --- a/kokoro/windows/build.bat +++ b/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" ( diff --git a/packager/app/test/packager_test.py b/packager/app/test/packager_test.py index aa4fb40a6c..1334b93609 100755 --- a/packager/app/test/packager_test.py +++ b/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 @@ class DiffFilesPolicy(object): 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 @@ class DiffFilesPolicy(object): 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 @@ class PackagerAppTest(unittest.TestCase): 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, diff --git a/packager/tools/git/check_pylint.py b/packager/tools/git/check_pylint.py new file mode 100755 index 0000000000..812a6c8d37 --- /dev/null +++ b/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() diff --git a/packager/tools/git/pylintrc b/packager/tools/git/pylintrc new file mode 100644 index 0000000000..9cdfb9cb2b --- /dev/null +++ b/packager/tools/git/pylintrc @@ -0,0 +1,441 @@ +# This Pylint rcfile contains a best-effort configuration to uphold the +# best-practices and style described in the Google Python style guide: +# https://google.github.io/styleguide/pyguide.html +# +# Its canonical open-source location is: +# https://google.github.io/styleguide/pylintrc + +[MASTER] + +# Files or directories to be skipped. They should be base names, not paths. +ignore=third_party + +# Files or directories matching the regex patterns are skipped. The regex +# matches against base names, not paths. +ignore-patterns= + +# Pickle collected data for later comparisons. +persistent=no + +# List of plugins (as comma separated values of python modules names) to load, +# usually to register additional checkers. +load-plugins= + +# Use multiple processes to speed up Pylint. +jobs=4 + +# Allow loading of arbitrary C extensions. Extensions are imported into the +# active Python interpreter and may run arbitrary code. +unsafe-load-any-extension=no + + +[MESSAGES CONTROL] + +# Only show warnings with the listed confidence levels. Leave empty to show +# all. Valid levels: HIGH, INFERENCE, INFERENCE_FAILURE, UNDEFINED +confidence= + +# Enable the message, report, category or checker with the given id(s). You can +# either give multiple identifier separated by comma (,) or put this option +# multiple time (only on the command line, not in the configuration file where +# it should appear only once). See also the "--disable" option for examples. +#enable= + +# Disable the message, report, category or checker with the given id(s). You +# can either give multiple identifiers separated by comma (,) or put this +# option multiple times (only on the command line, not in the configuration +# file where it should appear only once).You can also use "--disable=all" to +# disable everything first and then reenable specific checks. For example, if +# you want to run only the similarities checker, you can use "--disable=all +# --enable=similarities". If you want to run only the classes checker, but have +# no Warning level messages displayed, use"--disable=all --enable=classes +# --disable=W" +disable=abstract-method, + apply-builtin, + arguments-differ, + attribute-defined-outside-init, + backtick, + bad-option-value, + basestring-builtin, + buffer-builtin, + c-extension-no-member, + consider-using-enumerate, + cmp-builtin, + cmp-method, + coerce-builtin, + coerce-method, + delslice-method, + div-method, + duplicate-code, + eq-without-hash, + execfile-builtin, + file-builtin, + filter-builtin-not-iterating, + fixme, + getslice-method, + global-statement, + hex-method, + idiv-method, + implicit-str-concat-in-sequence, + import-error, + import-self, + import-star-module-level, + inconsistent-return-statements, + input-builtin, + intern-builtin, + invalid-str-codec, + locally-disabled, + long-builtin, + long-suffix, + map-builtin-not-iterating, + misplaced-comparison-constant, + missing-function-docstring, + metaclass-assignment, + next-method-called, + next-method-defined, + no-absolute-import, + no-else-break, + no-else-continue, + no-else-raise, + no-else-return, + no-init, # added + no-member, + no-name-in-module, + no-self-use, + nonzero-method, + oct-method, + old-division, + old-ne-operator, + old-octal-literal, + old-raise-syntax, + parameter-unpacking, + print-statement, + raising-string, + range-builtin-not-iterating, + raw_input-builtin, + rdiv-method, + reduce-builtin, + relative-import, + reload-builtin, + round-builtin, + setslice-method, + signature-differs, + standarderror-builtin, + suppressed-message, + sys-max-int, + too-few-public-methods, + too-many-ancestors, + too-many-arguments, + too-many-boolean-expressions, + too-many-branches, + too-many-instance-attributes, + too-many-locals, + too-many-nested-blocks, + too-many-public-methods, + too-many-return-statements, + too-many-statements, + trailing-newlines, + unichr-builtin, + unicode-builtin, + unnecessary-pass, + unpacking-in-except, + useless-else-on-loop, + useless-object-inheritance, + useless-suppression, + using-cmp-argument, + wrong-import-order, + xrange-builtin, + zip-builtin-not-iterating, + + +[REPORTS] + +# Set the output format. Available formats are text, parseable, colorized, msvs +# (visual studio) and html. You can also give a reporter class, eg +# mypackage.mymodule.MyReporterClass. +output-format=text + +# Put messages in a separate file for each module / package specified on the +# command line instead of printing them on stdout. Reports (if any) will be +# written in a file name "pylint_global.[txt|html]". This option is deprecated +# and it will be removed in Pylint 2.0. +files-output=no + +# Tells whether to display a full report or only the messages +reports=no + +# Python expression which should return a note less than 10 (10 is the highest +# note). You have access to the variables errors warning, statement which +# respectively contain the number of errors / warnings messages and the total +# number of statements analyzed. This is used by the global evaluation report +# (RP0004). +evaluation=10.0 - ((float(5 * error + warning + refactor + convention) / statement) * 10) + +# Template used to display messages. This is a python new-style format string +# used to format the message information. See doc for all details +#msg-template= + + +[BASIC] + +# Good variable names which should always be accepted, separated by a comma +good-names=main,_ + +# Bad variable names which should always be refused, separated by a comma +bad-names= + +# Colon-delimited sets of names that determine each other's naming style when +# the name regexes allow several styles. +name-group= + +# Include a hint for the correct naming format with invalid-name +include-naming-hint=no + +# List of decorators that produce properties, such as abc.abstractproperty. Add +# to this list to register other decorators that produce valid properties. +property-classes=abc.abstractproperty,cached_property.cached_property,cached_property.threaded_cached_property,cached_property.cached_property_with_ttl,cached_property.threaded_cached_property_with_ttl + +# Regular expression matching correct function names +function-rgx=^(?:(?PsetUp|tearDown|setUpModule|tearDownModule)|(?P_?[A-Z][a-zA-Z0-9]*)|(?P_?[a-z][a-z0-9_]*))$ + +# Regular expression matching correct variable names +variable-rgx=^[a-z][a-z0-9_]*$ + +# Regular expression matching correct constant names +const-rgx=^(_?[A-Z][A-Z0-9_]*|__[a-z0-9_]+__|_?[a-z][a-z0-9_]*)$ + +# Regular expression matching correct attribute names +attr-rgx=^_{0,2}[a-z][a-z0-9_]*$ + +# Regular expression matching correct argument names +argument-rgx=^[a-z][a-z0-9_]*$ + +# Regular expression matching correct class attribute names +class-attribute-rgx=^(_?[A-Z][A-Z0-9_]*|__[a-z0-9_]+__|_?[a-z][a-z0-9_]*)$ + +# Regular expression matching correct inline iteration names +inlinevar-rgx=^[a-z][a-z0-9_]*$ + +# Regular expression matching correct class names +class-rgx=^_?[A-Z][a-zA-Z0-9]*$ + +# Regular expression matching correct module names +module-rgx=^(_?[a-z][a-z0-9_]*|__init__)$ + +# Regular expression matching correct method names +method-rgx=(?x)^(?:(?P_[a-z0-9_]+__|runTest|setUp|tearDown|setUpTestCase|tearDownTestCase|setupSelf|tearDownClass|setUpClass|(test|assert)_*[A-Z0-9][a-zA-Z0-9_]*|next)|(?P_{0,2}[A-Z][a-zA-Z0-9_]*)|(?P_{0,2}[a-z][a-z0-9_]*))$ + +# Regular expression which should only match function or class names that do +# not require a docstring. +no-docstring-rgx=(__.*__|main|test.*|.*test|.*Test)$ + +# Minimum line length for functions/classes that require docstrings, shorter +# ones are exempt. +docstring-min-length=10 + + +[TYPECHECK] + +# List of decorators that produce context managers, such as +# contextlib.contextmanager. Add to this list to register other decorators that +# produce valid context managers. +contextmanager-decorators=contextlib.contextmanager,contextlib2.contextmanager + +# Tells whether missing members accessed in mixin class should be ignored. A +# mixin class is detected if its name ends with "mixin" (case insensitive). +ignore-mixin-members=yes + +# List of module names for which member attributes should not be checked +# (useful for modules/projects where namespaces are manipulated during runtime +# and thus existing member attributes cannot be deduced by static analysis. It +# supports qualified module names, as well as Unix pattern matching. +ignored-modules= + +# List of class names for which member attributes should not be checked (useful +# for classes with dynamically set attributes). This supports the use of +# qualified names. +ignored-classes=optparse.Values,thread._local,_thread._local + +# List of members which are set dynamically and missed by pylint inference +# system, and so shouldn't trigger E1101 when accessed. Python regular +# expressions are accepted. +generated-members= + + +[FORMAT] + +# Maximum number of characters on a single line. +max-line-length=80 + +# TODO(https://github.com/PyCQA/pylint/issues/3352): Direct pylint to exempt +# lines made too long by directives to pytype. + +# Regexp for a line that is allowed to be longer than the limit. +ignore-long-lines=(?x)( + ^\s*(\#\ )??$| + ^\s*(from\s+\S+\s+)?import\s+.+$) + +# Allow the body of an if to be on the same line as the test if there is no +# else. +single-line-if-stmt=yes + +# List of optional constructs for which whitespace checking is disabled. `dict- +# separator` is used to allow tabulation in dicts, etc.: {1 : 1,\n222: 2}. +# `trailing-comma` allows a space between comma and closing bracket: (a, ). +# `empty-line` allows space-only lines. +no-space-check= + +# Maximum number of lines in a module +max-module-lines=99999 + +# String used as indentation unit. The internal Google style guide mandates 2 +# spaces. Google's externaly-published style guide says 4, consistent with +# PEP 8. Here, we use 2 spaces, for conformity with many open-sourced Google +# projects (like TensorFlow). +indent-string=' ' + +# Number of spaces of indent required inside a hanging or continued line. +indent-after-paren=4 + +# Expected format of line ending, e.g. empty (any line ending), LF or CRLF. +expected-line-ending-format= + + +[MISCELLANEOUS] + +# List of note tags to take in consideration, separated by a comma. +notes=TODO + + +[STRING] + +# This flag controls whether inconsistent-quotes generates a warning when the +# character used as a quote delimiter is used inconsistently within a module. +check-quote-consistency=yes + + +[VARIABLES] + +# Tells whether we should check for unused import in __init__ files. +init-import=no + +# A regular expression matching the name of dummy variables (i.e. expectedly +# not used). +dummy-variables-rgx=^\*{0,2}(_$|unused_|dummy_) + +# List of additional names supposed to be defined in builtins. Remember that +# you should avoid to define new builtins when possible. +additional-builtins= + +# List of strings which can identify a callback function by name. A callback +# name must start or end with one of those strings. +callbacks=cb_,_cb + +# List of qualified module names which can have objects that can redefine +# builtins. +redefining-builtins-modules=six,six.moves,past.builtins,future.builtins,functools + + +[LOGGING] + +# Logging modules to check that the string format arguments are in logging +# function parameter format +logging-modules=logging,absl.logging,tensorflow.io.logging + + +[SIMILARITIES] + +# Minimum lines number of a similarity. +min-similarity-lines=4 + +# Ignore comments when computing similarities. +ignore-comments=yes + +# Ignore docstrings when computing similarities. +ignore-docstrings=yes + +# Ignore imports when computing similarities. +ignore-imports=no + + +[SPELLING] + +# Spelling dictionary name. Available dictionaries: none. To make it working +# install python-enchant package. +spelling-dict= + +# List of comma separated words that should not be checked. +spelling-ignore-words= + +# A path to a file that contains private dictionary; one word per line. +spelling-private-dict-file= + +# Tells whether to store unknown words to indicated private dictionary in +# --spelling-private-dict-file option instead of raising a message. +spelling-store-unknown-words=no + + +[IMPORTS] + +# Deprecated modules which should not be used, separated by a comma +deprecated-modules=regsub, + TERMIOS, + Bastion, + rexec, + sets + +# Create a graph of every (i.e. internal and external) dependencies in the +# given file (report RP0402 must not be disabled) +import-graph= + +# Create a graph of external dependencies in the given file (report RP0402 must +# not be disabled) +ext-import-graph= + +# Create a graph of internal dependencies in the given file (report RP0402 must +# not be disabled) +int-import-graph= + +# Force import order to recognize a module as part of the standard +# compatibility libraries. +known-standard-library= + +# Force import order to recognize a module as part of a third party library. +known-third-party=enchant, absl + +# Analyse import fallback blocks. This can be used to support both Python 2 and +# 3 compatible code, which means that the block might have code that exists +# only in one or another interpreter, leading to false positives when analysed. +analyse-fallback-blocks=no + + +[CLASSES] + +# List of method names used to declare (i.e. assign) instance attributes. +defining-attr-methods=__init__, + __new__, + setUp + +# List of member names, which should be excluded from the protected access +# warning. +exclude-protected=_asdict, + _fields, + _replace, + _source, + _make + +# List of valid names for the first argument in a class method. +valid-classmethod-first-arg=cls, + class_ + +# List of valid names for the first argument in a metaclass class method. +valid-metaclass-classmethod-first-arg=mcs + + +[EXCEPTIONS] + +# Exceptions that will emit a warning when being caught. Defaults to +# "Exception" +overgeneral-exceptions=StandardError, + Exception, + BaseException diff --git a/packager/tools/pssh/pssh-box.py b/packager/tools/pssh/pssh-box.py index f13fc7798b..9216be28a1 100755 --- a/packager/tools/pssh/pssh-box.py +++ b/packager/tools/pssh/pssh-box.py @@ -7,6 +7,10 @@ """A utility to parse and generate PSSH boxes.""" +# This file itself is considered an invalid module name because of the dash in +# the filename: pssh-box.py +# pylint: disable=invalid-name + from __future__ import print_function import argparse @@ -26,7 +30,8 @@ assert os.path.exists(_proto_path), ( sys.path.insert(0, _proto_path) sys.path.insert(0, _widevine_proto_path) -import widevine_pssh_data_pb2 # pylint: disable=g-import-not-at-top +# pylint: disable=wrong-import-position +import widevine_pssh_data_pb2 COMMON_SYSTEM_ID = base64.b16decode('1077EFECC0B24D02ACE33C1E52E2FB4B') WIDEVINE_SYSTEM_ID = base64.b16decode('EDEF8BA979D64ACEA3C827DCD51D21ED') diff --git a/packager/version/generate_version_string.py b/packager/version/generate_version_string.py index 17c241724a..c5ddcf9a2b 100755 --- a/packager/version/generate_version_string.py +++ b/packager/version/generate_version_string.py @@ -11,30 +11,9 @@ from __future__ import print_function import subprocess -# To support python version before 2.7, which does not have -# subprocess.check_output. -if 'check_output' not in dir(subprocess): - - def check_output_implementation(*popenargs, **kwargs): - """Implement check_output if it is not available.""" - if 'stdout' in kwargs: - raise ValueError('stdout argument not allowed, it will be overridden.') - process = subprocess.Popen(stdout=subprocess.PIPE, *popenargs, **kwargs) - output, unused_err = process.communicate() - retcode = process.poll() - if retcode: - cmd = kwargs.get('args') - if cmd is None: - cmd = popenargs[0] - raise subprocess.CalledProcessError(retcode, cmd) - return output - - subprocess.check_output = check_output_implementation - if __name__ == '__main__': try: - version_tag = subprocess.check_output( - 'git tag --points-at HEAD', + version_tag = subprocess.check_output('git tag --points-at HEAD', stderr=subprocess.STDOUT, shell=True).rstrip() except subprocess.CalledProcessError as e: # git tag --points-at is not supported in old versions of git. Just ignore @@ -42,8 +21,7 @@ if __name__ == '__main__': version_tag = None try: - version_hash = subprocess.check_output( - 'git rev-parse --short HEAD', + version_hash = subprocess.check_output('git rev-parse --short HEAD', stderr=subprocess.STDOUT, shell=True).rstrip() except subprocess.CalledProcessError as e: version_hash = 'unknown-version'