From c37300030717b2fb818b72ee5ee023f8de176869 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 15:07:57 +0100 Subject: [PATCH 01/10] Make sure to use a Python 3 pylint On some systems, such as Ubuntu up to 19.04, `pylint` is for Python 2 and `pylint3` is for Python 3, so we should not use `pylint` even if it's available. Use the Python module instead of the trivial shell wrapper. This way we can make sure to use the correct Python version. Fix #3111 Signed-off-by: Gilles Peskine --- tests/scripts/check-python-files.sh | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/scripts/check-python-files.sh b/tests/scripts/check-python-files.sh index 6b864d264..cd18518ca 100755 --- a/tests/scripts/check-python-files.sh +++ b/tests/scripts/check-python-files.sh @@ -9,15 +9,10 @@ # Run 'pylint' on Python files for programming errors and helps enforcing # PEP8 coding standards. -# Find the installed version of Pylint. Installed as a distro package this can -# be pylint3 and as a PEP egg, pylint. We prefer pylint over pylint3 -if type pylint >/dev/null 2>/dev/null; then - PYLINT=pylint -elif type pylint3 >/dev/null 2>/dev/null; then - PYLINT=pylint3 +if type python3 >/dev/null 2>/dev/null; then + PYTHON=python3 else - echo 'Pylint was not found.' - exit 1 + PYTHON=python fi -$PYLINT -j 2 scripts/*.py tests/scripts/*.py +$PYTHON -m pylint -j 2 scripts/*.py tests/scripts/*.py From 8c3ad4bfcb2985210805142fa837c12339bfde5d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 15:09:13 +0100 Subject: [PATCH 02/10] Make check_python_files non-optional in all.sh check_python_files was optional in all.sh because we used to have CI machines where pylint wasn't available. But this had the downside that check_python_files kept breaking because it wasn't checked in the CI. Now our CI has pylint and check_python_files should not be optional. Signed-off-by: Gilles Peskine --- tests/scripts/all.sh | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index dd8f510dd..4e4951552 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1409,15 +1409,6 @@ component_test_zeroize () { unset gdb_disable_aslr } -support_check_python_files () { - # Find the installed version of Pylint. Installed as a distro package this can - # be pylint3 and as a PEP egg, pylint. - if type pylint >/dev/null 2>/dev/null || type pylint3 >/dev/null 2>/dev/null; then - true; - else - false; - fi -} component_check_python_files () { msg "Lint: Python scripts" record_status tests/scripts/check-python-files.sh From 926f696a7366a2d2b649adf5c543079622da3483 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 16:07:40 +0100 Subject: [PATCH 03/10] Pylint: allow if-return-else-return Allow the perfectly reasonable idiom if condition1: return value1 else: return value2 Signed-off-by: Gilles Peskine --- .pylintrc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index 168e0b759..635985870 100644 --- a/.pylintrc +++ b/.pylintrc @@ -40,7 +40,12 @@ max-attributes=15 max-module-lines=2000 [MESSAGES CONTROL] -disable= +# * no-else-return: Allow the perfectly reasonable idiom +# if condition1: +# return value1 +# else: +# return value2 +disable=no-else-return [REPORTS] # Don't diplay statistics. Just the facts. From ea16e3dd7f328cc986ab4a0b034d12a66a7500aa Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 16:39:30 +0100 Subject: [PATCH 04/10] Pylint: disable logging-format-interpolation warning Pylint warns about things like ``log.info('...'.format(...))``. It insists on ``log.info('...', ...)``. This is of minor utility (mainly a performance gain when there are many messages that use formatting and are below the log level). Some versions of Pylint (including 1.8, which is the version on Ubuntu 18.04) only recognize old-style format strings using '%', and complain about something like ``log.info('{}', foo)`` with logging-too-many-args (Pylint supports new-style formatting if declared globally with logging_format_style under [LOGGING] but this requires Pylint >=2.2). Disable this warning to remain compatible with Pylint 1.8 and not have to change abi_check.py to use %-formats instead of {}-formats when logging. Signed-off-by: Gilles Peskine --- .pylintrc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index 635985870..f82d4cfc0 100644 --- a/.pylintrc +++ b/.pylintrc @@ -40,12 +40,22 @@ max-attributes=15 max-module-lines=2000 [MESSAGES CONTROL] +# * logging-format-interpolation: Pylint warns about things like +# ``log.info('...'.format(...))``. It insists on ``log.info('...', ...)``. +# This is of minor utility (mainly a performance gain when there are +# many messages that use formatting and are below the log level). +# Some versions of Pylint (including 1.8, which is the version on +# Ubuntu 18.04) only recognize old-style format strings using '%', +# and complain about something like ``log.info('{}', foo)`` with +# logging-too-many-args (Pylint supports new-style formatting if +# declared globally with logging_format_style under [LOGGING] but +# this requires Pylint >=2.2). # * no-else-return: Allow the perfectly reasonable idiom # if condition1: # return value1 # else: # return value2 -disable=no-else-return +disable=logging-format-interpolation,no-else-return [REPORTS] # Don't diplay statistics. Just the facts. From af67f8db77b06f739127ebc615827be83eb14241 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 16:49:21 +0100 Subject: [PATCH 05/10] Document more methods in Python scripts Signed-off-by: Gilles Peskine --- tests/scripts/check-files.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 255bed8b9..2515ec9c1 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -37,20 +37,31 @@ class FileIssueTracker(object): self.files_with_issues = {} def should_check_file(self, filepath): + """Whether the given file name should be checked. + + Files whose name ends with a string listed in ``self.files_exemptions`` + will not be checked. + """ for files_exemption in self.files_exemptions: if filepath.endswith(files_exemption): return False return True def check_file_for_issue(self, filepath): + """Check the specified file for the issue that this class is for. + + Subclasses must implement this method. + """ raise NotImplementedError def record_issue(self, filepath, line_number): + """Record that an issue was found at the specified location.""" if filepath not in self.files_with_issues.keys(): self.files_with_issues[filepath] = [] self.files_with_issues[filepath].append(line_number) def output_file_issues(self, logger): + """Log all the locations where the issue was found.""" if self.files_with_issues.values(): logger.info(self.heading) for filename, lines in sorted(self.files_with_issues.items()): @@ -70,6 +81,10 @@ class LineIssueTracker(FileIssueTracker): """ def issue_with_line(self, line, filepath): + """Check the specified line for the issue that this class is for. + + Subclasses must implement this method. + """ raise NotImplementedError def check_file_line(self, filepath, line, line_number): @@ -77,6 +92,10 @@ class LineIssueTracker(FileIssueTracker): self.record_issue(filepath, line_number) def check_file_for_issue(self, filepath): + """Check the lines of the specified file. + + Subclasses must implement the ``issue_with_line`` method. + """ with open(filepath, "rb") as f: for i, line in enumerate(iter(f.readline, b"")): self.check_file_line(filepath, line, i + 1) From a1bb3f86e967ce63e7ae2902704077019aae8788 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:20:59 +0100 Subject: [PATCH 06/10] mbedtls_test.py: drop compatibility with Python 2 Python 2 is no longer supported upstream. Actively drop compatibility with Python 2. Removing the inheritance of a class on object pacifies recent versions of Pylint (useless-object-inheritance). Signed-off-by: Gilles Peskine --- tests/scripts/mbedtls_test.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/scripts/mbedtls_test.py b/tests/scripts/mbedtls_test.py index 8f24435bf..9a58a369a 100755 --- a/tests/scripts/mbedtls_test.py +++ b/tests/scripts/mbedtls_test.py @@ -1,3 +1,5 @@ +#!/usr/bin/env python3 + # Greentea host test script for Mbed TLS on-target test suite testing. # # Copyright (C) 2018, Arm Limited, All Rights Reserved @@ -46,7 +48,7 @@ class TestDataParserError(Exception): pass -class TestDataParser(object): +class TestDataParser: """ Parses test name, dependencies, test function name and test parameters from the data file. From 5d1dfd4108941f18cb3aee61a3ae850894974883 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:25:17 +0100 Subject: [PATCH 07/10] Pylint: abide by useless-object-inheritance warnings Inheriting from object is a remainder of Python 2 habits and is just clutter in Python 3. Signed-off-by: Gilles Peskine --- scripts/abi_check.py | 2 +- tests/scripts/check-files.py | 4 ++-- tests/scripts/generate_test_code.py | 2 +- tests/scripts/test_generate_test_code.py | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index e19f2c0c6..c2aca501d 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -29,7 +29,7 @@ from types import SimpleNamespace import xml.etree.ElementTree as ET -class AbiChecker(object): +class AbiChecker: """API and ABI checker.""" def __init__(self, old_version, new_version, configuration): diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 2515ec9c1..f8d9261b4 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -17,7 +17,7 @@ import codecs import sys -class FileIssueTracker(object): +class FileIssueTracker: """Base class for file-wide issue tracking. To implement a checker that processes a file as a whole, inherit from @@ -188,7 +188,7 @@ class MergeArtifactIssueTracker(LineIssueTracker): return False -class IntegrityChecker(object): +class IntegrityChecker: """Sanity-check files under the current directory.""" def __init__(self, log_file): diff --git a/tests/scripts/generate_test_code.py b/tests/scripts/generate_test_code.py index 1fff09992..c0f99f74f 100755 --- a/tests/scripts/generate_test_code.py +++ b/tests/scripts/generate_test_code.py @@ -208,7 +208,7 @@ class GeneratorInputError(Exception): pass -class FileWrapper(io.FileIO, object): +class FileWrapper(io.FileIO): """ This class extends built-in io.FileIO class with attribute line_no, that indicates line number for the line that is read. diff --git a/tests/scripts/test_generate_test_code.py b/tests/scripts/test_generate_test_code.py index 6d7113e18..e39b29b83 100755 --- a/tests/scripts/test_generate_test_code.py +++ b/tests/scripts/test_generate_test_code.py @@ -294,7 +294,7 @@ class GenDispatch(TestCase): self.assertEqual(code, expected) -class StringIOWrapper(StringIO, object): +class StringIOWrapper(StringIO): """ file like class to mock file object in tests. """ From 399b82f98665427d9491195b4d53ce30c46adafb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:36:56 +0100 Subject: [PATCH 08/10] Pylint: minor code simplifications Simplify the code in minor ways. Each of this changes fixes a warning from Pylint 2.4 that doesn't appear with Pylint 1.7. Signed-off-by: Gilles Peskine --- tests/scripts/generate_test_code.py | 3 +-- tests/scripts/mbedtls_test.py | 2 +- tests/scripts/test_generate_test_code.py | 10 ++++------ 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/scripts/generate_test_code.py b/tests/scripts/generate_test_code.py index c0f99f74f..21f816ea9 100755 --- a/tests/scripts/generate_test_code.py +++ b/tests/scripts/generate_test_code.py @@ -402,8 +402,7 @@ def parse_dependencies(inp_str): :param inp_str: Input string with macros delimited by ':'. :return: list of dependencies """ - dependencies = [dep for dep in map(validate_dependency, - inp_str.split(':'))] + dependencies = list(map(validate_dependency, inp_str.split(':'))) return dependencies diff --git a/tests/scripts/mbedtls_test.py b/tests/scripts/mbedtls_test.py index 9a58a369a..709bb1a3e 100755 --- a/tests/scripts/mbedtls_test.py +++ b/tests/scripts/mbedtls_test.py @@ -262,7 +262,7 @@ class MbedTlsTest(BaseHostTest): data_bytes += bytearray(dependencies) data_bytes += bytearray([function_id, len(parameters)]) for typ, param in parameters: - if typ == 'int' or typ == 'exp': + if typ in ('int', 'exp'): i = int(param, 0) data_bytes += b'I' if typ == 'int' else b'E' self.align_32bit(data_bytes) diff --git a/tests/scripts/test_generate_test_code.py b/tests/scripts/test_generate_test_code.py index e39b29b83..c8e8c5ce1 100755 --- a/tests/scripts/test_generate_test_code.py +++ b/tests/scripts/test_generate_test_code.py @@ -1127,9 +1127,8 @@ Diffie-Hellman selftest dhm_selftest: """ stream = StringIOWrapper('test_suite_ut.function', data) - tests = [(name, test_function, dependencies, args) - for name, test_function, dependencies, args in - parse_test_data(stream)] + # List of (name, function_name, dependencies, args) + tests = list(parse_test_data(stream)) test1, test2, test3, test4 = tests self.assertEqual(test1[0], 'Diffie-Hellman full exchange #1') self.assertEqual(test1[1], 'dhm_do_dhm') @@ -1170,9 +1169,8 @@ dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622" """ stream = StringIOWrapper('test_suite_ut.function', data) - tests = [(name, function_name, dependencies, args) - for name, function_name, dependencies, args in - parse_test_data(stream)] + # List of (name, function_name, dependencies, args) + tests = list(parse_test_data(stream)) test1, test2 = tests self.assertEqual(test1[0], 'Diffie-Hellman full exchange #1') self.assertEqual(test1[1], 'dhm_do_dhm') From 867ab917db2abce812157ea6635eb4fde4282850 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:39:50 +0100 Subject: [PATCH 09/10] Pylint: allow using pass even when not strictly necessary If we take the trouble of using pass, it's because we think the code is clearer that way. For example, Pylint 2.4 rejects pass in def foo(): """Do nothing.""" pass But relying on a docstring as the sole code is weird, hence the use of pass. Signed-off-by: Gilles Peskine --- .pylintrc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index f82d4cfc0..262b5cf04 100644 --- a/.pylintrc +++ b/.pylintrc @@ -55,7 +55,9 @@ max-module-lines=2000 # return value1 # else: # return value2 -disable=logging-format-interpolation,no-else-return +# * unnecessary-pass: If we take the trouble of adding a line with "pass", +# it's because we think the code is clearer that way. +disable=logging-format-interpolation,no-else-return,unnecessary-pass [REPORTS] # Don't diplay statistics. Just the facts. From 11b026969612b2aec6b91aaed131c762951ff428 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:47:06 +0100 Subject: [PATCH 10/10] Pylint: silence locally-disabled/enabled messages If we disable or enable a message locally, it's by design. There's no need to clutter the Pylint output with this information. Signed-off-by: Gilles Peskine --- .pylintrc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index 262b5cf04..08c114ad9 100644 --- a/.pylintrc +++ b/.pylintrc @@ -40,6 +40,9 @@ max-attributes=15 max-module-lines=2000 [MESSAGES CONTROL] +# * locally-disabled, locally-enabled: If we disable or enable a message +# locally, it's by design. There's no need to clutter the Pylint output +# with this information. # * logging-format-interpolation: Pylint warns about things like # ``log.info('...'.format(...))``. It insists on ``log.info('...', ...)``. # This is of minor utility (mainly a performance gain when there are @@ -57,7 +60,7 @@ max-module-lines=2000 # return value2 # * unnecessary-pass: If we take the trouble of adding a line with "pass", # it's because we think the code is clearer that way. -disable=logging-format-interpolation,no-else-return,unnecessary-pass +disable=locally-disabled,locally-enabled,logging-format-interpolation,no-else-return,unnecessary-pass [REPORTS] # Don't diplay statistics. Just the facts.