Merge pull request #3953 from gilles-peskine-arm/python-mypy-mkdir

Python upscale: pass mypy and create library directory

.mypy.ini

@@ -0,0 +1,4 @@
+[mypy]
+mypy_path = scripts
+namespace_packages = True
+warn_unused_configs = True

.pylintrc

@@ -1,3 +1,6 @@
+[MASTER]
+init-hook='import sys; sys.path.append("scripts")'
+
 [BASIC]
 # We're ok with short funtion argument names.
 # [invalid-name]

.travis.yml

@@ -17,7 +17,7 @@ jobs:
       language: python # Needed to get pip for Python 3
       python: 3.5 # version from Ubuntu 16.04
       install:
-        - pip install pylint==2.4.4
+        - pip install mypy==0.780 pylint==2.4.4
       script:
         - tests/scripts/all.sh -k 'check_*'
         - tests/scripts/all.sh -k test_default_out_of_box

scripts/mbedtls_dev/c_build_helper.py

@@ -0,0 +1,138 @@
+"""Generate and run C code.
+"""
+
+# Copyright The Mbed TLS Contributors
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import platform
+import subprocess
+import sys
+import tempfile
+
+def remove_file_if_exists(filename):
+    """Remove the specified file, ignoring errors."""
+    if not filename:
+        return
+    try:
+        os.remove(filename)
+    except OSError:
+        pass
+
+def create_c_file(file_label):
+    """Create a temporary C file.
+
+    * ``file_label``: a string that will be included in the file name.
+
+    Return ```(c_file, c_name, exe_name)``` where ``c_file`` is a Python
+    stream open for writing to the file, ``c_name`` is the name of the file
+    and ``exe_name`` is the name of the executable that will be produced
+    by compiling the file.
+    """
+    c_fd, c_name = tempfile.mkstemp(prefix='tmp-{}-'.format(file_label),
+                                    suffix='.c')
+    exe_suffix = '.exe' if platform.system() == 'Windows' else ''
+    exe_name = c_name[:-2] + exe_suffix
+    remove_file_if_exists(exe_name)
+    c_file = os.fdopen(c_fd, 'w', encoding='ascii')
+    return c_file, c_name, exe_name
+
+def generate_c_printf_expressions(c_file, cast_to, printf_format, expressions):
+    """Generate C instructions to print the value of ``expressions``.
+
+    Write the code with ``c_file``'s ``write`` method.
+
+    Each expression is cast to the type ``cast_to`` and printed with the
+    printf format ``printf_format``.
+    """
+    for expr in expressions:
+        c_file.write('    printf("{}\\n", ({}) {});\n'
+                     .format(printf_format, cast_to, expr))
+
+def generate_c_file(c_file,
+                    caller, header,
+                    main_generator):
+    """Generate a temporary C source file.
+
+    * ``c_file`` is an open stream on the C source file.
+    * ``caller``: an informational string written in a comment at the top
+      of the file.
+    * ``header``: extra code to insert before any function in the generated
+      C file.
+    * ``main_generator``: a function called with ``c_file`` as its sole argument
+      to generate the body of the ``main()`` function.
+    """
+    c_file.write('/* Generated by {} */'
+                 .format(caller))
+    c_file.write('''
+#include <stdio.h>
+''')
+    c_file.write(header)
+    c_file.write('''
+int main(void)
+{
+''')
+    main_generator(c_file)
+    c_file.write('''    return 0;
+}
+''')
+
+def get_c_expression_values(
+        cast_to, printf_format,
+        expressions,
+        caller=__name__, file_label='',
+        header='', include_path=None,
+        keep_c=False,
+): # pylint: disable=too-many-arguments
+    """Generate and run a program to print out numerical values for expressions.
+
+    * ``cast_to``: a C type.
+    * ``printf_format``: a printf format suitable for the type ``cast_to``.
+    * ``header``: extra code to insert before any function in the generated
+      C file.
+    * ``expressions``: a list of C language expressions that have the type
+      ``cast_to``.
+    * ``include_path``: a list of directories containing header files.
+    * ``keep_c``: if true, keep the temporary C file (presumably for debugging
+      purposes).
+
+    Return the list of values of the ``expressions``.
+    """
+    if include_path is None:
+        include_path = []
+    c_name = None
+    exe_name = None
+    try:
+        c_file, c_name, exe_name = create_c_file(file_label)
+        generate_c_file(
+            c_file, caller, header,
+            lambda c_file: generate_c_printf_expressions(c_file,
+                                                         cast_to, printf_format,
+                                                         expressions)
+        )
+        c_file.close()
+        cc = os.getenv('CC', 'cc')
+        subprocess.check_call([cc] +
+                              ['-I' + dir for dir in include_path] +
+                              ['-o', exe_name, c_name])
+        if keep_c:
+            sys.stderr.write('List of {} tests kept at {}\n'
+                             .format(caller, c_name))
+        else:
+            os.remove(c_name)
+        output = subprocess.check_output([exe_name])
+        return output.decode('ascii').strip().split('\n')
+    finally:
+        remove_file_if_exists(exe_name)

tests/scripts/check-python-files.sh

@@ -14,11 +14,13 @@
 # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-#
-# Purpose:
-#
-# Run 'pylint' on Python files for programming errors and helps enforcing
-# PEP8 coding standards.
+
+# Purpose: check Python files for potential programming errors or maintenance
+# hurdles. Run pylint to detect some potential mistakes and enforce PEP8
+# coding standards. If available, run mypy to perform static type checking.
+
+# We'll keep going on errors and report the status at the end.
+ret=0
 
 if type python3 >/dev/null 2>/dev/null; then
     PYTHON=python3
@@ -26,4 +28,56 @@ else
     PYTHON=python
 fi
 
-$PYTHON -m pylint -j 2 scripts/*.py tests/scripts/*.py
+check_version () {
+    $PYTHON - "$2" <<EOF
+import packaging.version
+import sys
+import $1 as package
+actual = package.__version__
+wanted = sys.argv[1]
+if packaging.version.parse(actual) < packaging.version.parse(wanted):
+    sys.stderr.write("$1: version %s is too old (want %s)\n" % (actual, wanted))
+    exit(1)
+EOF
+}
+
+can_pylint () {
+    # Pylint 1.5.2 from Ubuntu 16.04 is too old:
+    #     E: 34, 0: Unable to import 'mbedtls_dev' (import-error)
+    # Pylint 1.8.3 from Ubuntu 18.04 passed on the first commit containing this line.
+    check_version pylint 1.8.3
+}
+
+can_mypy () {
+    # mypy 0.770 is too old:
+    #     tests/scripts/test_psa_constant_names.py:34: error: Cannot find implementation or library stub for module named 'mbedtls_dev'
+    # mypy 0.780 from pip passed on the first commit containing this line.
+    check_version mypy.version 0.780
+}
+
+# With just a --can-xxx option, check whether the tool for xxx is available
+# with an acceptable version, and exit without running any checks. The exit
+# status is true if the tool is available and acceptable and false otherwise.
+if [ "$1" = "--can-pylint" ]; then
+    can_pylint
+    exit
+elif [ "$1" = "--can-mypy" ]; then
+    can_mypy
+    exit
+fi
+
+echo 'Running pylint ...'
+$PYTHON -m pylint -j 2 scripts/mbedtls_dev/*.py scripts/*.py tests/scripts/*.py || {
+    echo >&2 "pylint reported errors"
+    ret=1
+}
+
+# Check types if mypy is available
+if can_mypy; then
+    echo
+    echo 'Running mypy ...'
+    $PYTHON -m mypy scripts/*.py tests/scripts/*.py ||
+      ret=1
+fi
+
+exit $ret

tests/scripts/check_files.py

@@ -29,6 +29,10 @@ import codecs
 import re
 import subprocess
 import sys
+try:
+    from typing import FrozenSet, Optional, Pattern # pylint: disable=unused-import
+except ImportError:
+    pass
 
 
 class FileIssueTracker:
@@ -48,8 +52,8 @@ class FileIssueTracker:
     ``heading``: human-readable description of the issue
     """
 
-    suffix_exemptions = frozenset()
-    path_exemptions = None
+    suffix_exemptions = frozenset() #type: FrozenSet[str]
+    path_exemptions = None #type: Optional[Pattern[str]]
     # heading must be defined in derived classes.
     # pylint: disable=no-member
 
@@ -161,13 +165,64 @@ class PermissionIssueTracker(FileIssueTracker):
 
     heading = "Incorrect permissions:"
 
+    # .py files can be either full scripts or modules, so they may or may
+    # not be executable.
+    suffix_exemptions = frozenset({".py"})
+
     def check_file_for_issue(self, filepath):
         is_executable = os.access(filepath, os.X_OK)
-        should_be_executable = filepath.endswith((".sh", ".pl", ".py"))
+        should_be_executable = filepath.endswith((".sh", ".pl"))
         if is_executable != should_be_executable:
             self.files_with_issues[filepath] = None
 
 
+class ShebangIssueTracker(FileIssueTracker):
+    """Track files with a bad, missing or extraneous shebang line.
+
+    Executable scripts must start with a valid shebang (#!) line.
+    """
+
+    heading = "Invalid shebang line:"
+
+    # Allow either /bin/sh, /bin/bash, or /usr/bin/env.
+    # Allow at most one argument (this is a Linux limitation).
+    # For sh and bash, the argument if present must be options.
+    # For env, the argument must be the base name of the interpeter.
+    _shebang_re = re.compile(rb'^#! ?(?:/bin/(bash|sh)(?: -[^\n ]*)?'
+                             rb'|/usr/bin/env ([^\n /]+))$')
+    _extensions = {
+        b'bash': 'sh',
+        b'perl': 'pl',
+        b'python3': 'py',
+        b'sh': 'sh',
+    }
+
+    def is_valid_shebang(self, first_line, filepath):
+        m = re.match(self._shebang_re, first_line)
+        if not m:
+            return False
+        interpreter = m.group(1) or m.group(2)
+        if interpreter not in self._extensions:
+            return False
+        if not filepath.endswith('.' + self._extensions[interpreter]):
+            return False
+        return True
+
+    def check_file_for_issue(self, filepath):
+        is_executable = os.access(filepath, os.X_OK)
+        with open(filepath, "rb") as f:
+            first_line = f.readline()
+        if first_line.startswith(b'#!'):
+            if not is_executable:
+                # Shebang on a non-executable file
+                self.files_with_issues[filepath] = None
+            elif not self.is_valid_shebang(first_line, filepath):
+                self.files_with_issues[filepath] = [1]
+        elif is_executable:
+            # Executable without a shebang
+            self.files_with_issues[filepath] = None
+
+
 class EndOfFileNewlineIssueTracker(FileIssueTracker):
     """Track files that end with an incomplete line
     (no newline character at the end of the last line)."""
@@ -288,6 +343,7 @@ class IntegrityChecker:
         self.setup_logger(log_file)
         self.issues_to_check = [
             PermissionIssueTracker(),
+            ShebangIssueTracker(),
             EndOfFileNewlineIssueTracker(),
             Utf8BomIssueTracker(),
             UnixLineEndingIssueTracker(),

tests/scripts/mbedtls_test.py

@@ -38,7 +38,7 @@ import re
 import os
 import binascii
 
-from mbed_host_tests import BaseHostTest, event_callback # pylint: disable=import-error
+from mbed_host_tests import BaseHostTest, event_callback # type: ignore # pylint: disable=import-error
 
 
 class TestDataParserError(Exception):

tests/scripts/scripts_path.py

@@ -0,0 +1,28 @@
+"""Add our Python library directory to the module search path.
+
+Usage:
+
+    import scripts_path # pylint: disable=unused-import
+"""
+
+# Copyright The Mbed TLS Contributors
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import sys
+
+sys.path.append(os.path.join(os.path.dirname(__file__),
+                             os.path.pardir, os.path.pardir,
+                             'scripts'))

tests/scripts/test_generate_test_code.py

@@ -20,21 +20,10 @@
 Unit tests for generate_test_code.py
 """
 
-# pylint: disable=wrong-import-order
-try:
-    # Python 2
-    from StringIO import StringIO
-except ImportError:
-    # Python 3
-    from io import StringIO
+from io import StringIO
 from unittest import TestCase, main as unittest_main
-try:
-    # Python 2
-    from mock import patch
-except ImportError:
-    # Python 3
-    from unittest.mock import patch
-# pylint: enable=wrong-import-order
+from unittest.mock import patch
+
 from generate_test_code import gen_dependencies, gen_dependencies_one_line
 from generate_test_code import gen_function_wrapper, gen_dispatch
 from generate_test_code import parse_until_pattern, GeneratorInputError
@@ -317,25 +306,16 @@ class StringIOWrapper(StringIO):
         :return: Line read from file.
         """
         parent = super(StringIOWrapper, self)
-        if getattr(parent, 'next', None):
-            # Python 2
-            line = parent.next()
-        else:
-            # Python 3
-            line = parent.__next__()
+        line = parent.__next__()
         return line
 
-    # Python 3
-    __next__ = next
-
-    def readline(self, length=0):
+    def readline(self, _length=0):
         """
         Wrap the base class readline.
 
         :param length:
         :return:
         """
-        # pylint: disable=unused-argument
         line = super(StringIOWrapper, self).readline()
         if line is not None:
             self.line_no += 1
@@ -549,38 +529,6 @@ class ParseFunctionCode(TestCase):
     Test suite for testing parse_function_code()
     """
 
-    def assert_raises_regex(self, exp, regex, func, *args):
-        """
-        Python 2 & 3 portable wrapper of assertRaisesRegex(p)? function.
-
-        :param exp: Exception type expected to be raised by cb.
-        :param regex: Expected exception message
-        :param func: callable object under test
-        :param args: variable positional arguments
-        """
-        parent = super(ParseFunctionCode, self)
-
-        # Pylint does not appreciate that the super method called
-        # conditionally can be available in other Python version
-        # then that of Pylint.
-        # Workaround is to call the method via getattr.
-        # Pylint ignores that the method got via getattr is
-        # conditionally executed. Method has to be a callable.
-        # Hence, using a dummy callable for getattr default.
-        dummy = lambda *x: None
-        # First Python 3 assertRaisesRegex is checked, since Python 2
-        # assertRaisesRegexp is also available in Python 3 but is
-        # marked deprecated.
-        for name in ('assertRaisesRegex', 'assertRaisesRegexp'):
-            method = getattr(parent, name, dummy)
-            if method is not dummy:
-                method(exp, regex, func, *args)
-                break
-        else:
-            raise AttributeError(" 'ParseFunctionCode' object has no attribute"
-                                 " 'assertRaisesRegex' or 'assertRaisesRegexp'"
-                                )
-
     def test_no_function(self):
         """
         Test no test function found.
@@ -593,8 +541,8 @@ function
 '''
         stream = StringIOWrapper('test_suite_ut.function', data)
         err_msg = 'file: test_suite_ut.function - Test functions not found!'
-        self.assert_raises_regex(GeneratorInputError, err_msg,
-                                 parse_function_code, stream, [], [])
+        self.assertRaisesRegex(GeneratorInputError, err_msg,
+                               parse_function_code, stream, [], [])
 
     def test_no_end_case_comment(self):
         """
@@ -609,8 +557,8 @@ void test_func()
         stream = StringIOWrapper('test_suite_ut.function', data)
         err_msg = r'file: test_suite_ut.function - '\
                   'end case pattern .*? not found!'
-        self.assert_raises_regex(GeneratorInputError, err_msg,
-                                 parse_function_code, stream, [], [])
+        self.assertRaisesRegex(GeneratorInputError, err_msg,
+                               parse_function_code, stream, [], [])
 
     @patch("generate_test_code.parse_function_arguments")
     def test_function_called(self,
@@ -727,8 +675,8 @@ exit:
         data = 'int entropy_threshold( char * a, data_t * h, int result )'
         err_msg = 'file: test_suite_ut.function - Test functions not found!'
         stream = StringIOWrapper('test_suite_ut.function', data)
-        self.assert_raises_regex(GeneratorInputError, err_msg,
-                                 parse_function_code, stream, [], [])
+        self.assertRaisesRegex(GeneratorInputError, err_msg,
+                               parse_function_code, stream, [], [])
 
     @patch("generate_test_code.gen_dispatch")
     @patch("generate_test_code.gen_dependencies")

tests/scripts/test_psa_constant_names.py

@@ -26,11 +26,12 @@ import argparse
 from collections import namedtuple
 import itertools
 import os
-import platform
 import re
 import subprocess
 import sys
-import tempfile
+
+import scripts_path # pylint: disable=unused-import
+from mbedtls_dev import c_build_helper
 
 class ReadFileLineException(Exception):
     def __init__(self, filename, line_number):
@@ -308,63 +309,23 @@ def gather_inputs(headers, test_suites, inputs_class=Inputs):
     inputs.gather_arguments()
     return inputs
 
-def remove_file_if_exists(filename):
-    """Remove the specified file, ignoring errors."""
-    if not filename:
-        return
-    try:
-        os.remove(filename)
-    except OSError:
-        pass
-
 def run_c(type_word, expressions, include_path=None, keep_c=False):
-    """Generate and run a program to print out numerical values for expressions."""
-    if include_path is None:
-        include_path = []
+    """Generate and run a program to print out numerical values of C expressions."""
     if type_word == 'status':
         cast_to = 'long'
         printf_format = '%ld'
     else:
         cast_to = 'unsigned long'
         printf_format = '0x%08lx'
-    c_name = None
-    exe_name = None
-    try:
-        c_fd, c_name = tempfile.mkstemp(prefix='tmp-{}-'.format(type_word),
-                                        suffix='.c',
-                                        dir='programs/psa')
-        exe_suffix = '.exe' if platform.system() == 'Windows' else ''
-        exe_name = c_name[:-2] + exe_suffix
-        remove_file_if_exists(exe_name)
-        c_file = os.fdopen(c_fd, 'w', encoding='ascii')
-        c_file.write('/* Generated by test_psa_constant_names.py for {} values */'
-                     .format(type_word))
-        c_file.write('''
-#include <stdio.h>
-#include <psa/crypto.h>
-int main(void)
-{
-''')
-        for expr in expressions:
-            c_file.write('    printf("{}\\n", ({}) {});\n'
-                         .format(printf_format, cast_to, expr))
-        c_file.write('''    return 0;
-}
-''')
-        c_file.close()
-        cc = os.getenv('CC', 'cc')
-        subprocess.check_call([cc] +
-                              ['-I' + dir for dir in include_path] +
-                              ['-o', exe_name, c_name])
-        if keep_c:
-            sys.stderr.write('List of {} tests kept at {}\n'
-                             .format(type_word, c_name))
-        else:
-            os.remove(c_name)
-        output = subprocess.check_output([exe_name])
-        return output.decode('ascii').strip().split('\n')
-    finally:
-        remove_file_if_exists(exe_name)
+    return c_build_helper.get_c_expression_values(
+        cast_to, printf_format,
+        expressions,
+        caller='test_psa_constant_names.py for {} values'.format(type_word),
+        file_label=type_word,
+        header='#include <psa/crypto.h>',
+        include_path=include_path,
+        keep_c=keep_c
+    )
 
 NORMALIZE_STRIP_RE = re.compile(r'\s+')
 def normalize(expr):