From 64f2efdc40bb1d53605a5aba9c263a405d42eb22 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 16 Sep 2022 21:41:47 +0200
Subject: [PATCH 01/10] More precise name for test data generation
We have Python code both for test code generation
(tests/scripts/generate_test_code.py) and now for test data generation.
Avoid the ambiguous expression "test generation".
This commit renames the Python module and adjusts all references to it. A
subsequent commit will adjust the documentation.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
.../{test_generation.py => test_data_generation.py} | 0
tests/CMakeLists.txt | 4 ++--
tests/Makefile | 4 ++--
tests/scripts/generate_bignum_tests.py | 10 +++++-----
tests/scripts/generate_psa_tests.py | 6 +++---
5 files changed, 12 insertions(+), 12 deletions(-)
rename scripts/mbedtls_dev/{test_generation.py => test_data_generation.py} (100%)
diff --git a/scripts/mbedtls_dev/test_generation.py b/scripts/mbedtls_dev/test_data_generation.py
similarity index 100%
rename from scripts/mbedtls_dev/test_generation.py
rename to scripts/mbedtls_dev/test_data_generation.py
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 57cf9770ff..b518e5833c 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -63,7 +63,7 @@ if(GEN_FILES)
DEPENDS
${CMAKE_CURRENT_SOURCE_DIR}/../tests/scripts/generate_bignum_tests.py
${CMAKE_CURRENT_SOURCE_DIR}/../scripts/mbedtls_dev/test_case.py
- ${CMAKE_CURRENT_SOURCE_DIR}/../scripts/mbedtls_dev/test_generation.py
+ ${CMAKE_CURRENT_SOURCE_DIR}/../scripts/mbedtls_dev/test_data_generation.py
)
add_custom_command(
OUTPUT
@@ -80,7 +80,7 @@ if(GEN_FILES)
${CMAKE_CURRENT_SOURCE_DIR}/../scripts/mbedtls_dev/macro_collector.py
${CMAKE_CURRENT_SOURCE_DIR}/../scripts/mbedtls_dev/psa_storage.py
${CMAKE_CURRENT_SOURCE_DIR}/../scripts/mbedtls_dev/test_case.py
- ${CMAKE_CURRENT_SOURCE_DIR}/../scripts/mbedtls_dev/test_generation.py
+ ${CMAKE_CURRENT_SOURCE_DIR}/../scripts/mbedtls_dev/test_data_generation.py
${CMAKE_CURRENT_SOURCE_DIR}/../include/psa/crypto_config.h
${CMAKE_CURRENT_SOURCE_DIR}/../include/psa/crypto_values.h
${CMAKE_CURRENT_SOURCE_DIR}/../include/psa/crypto_extra.h
diff --git a/tests/Makefile b/tests/Makefile
index 8777ae92fe..57f8855441 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -93,7 +93,7 @@ generated_files: $(GENERATED_FILES)
$(GENERATED_BIGNUM_DATA_FILES): generated_bignum_test_data
generated_bignum_test_data: scripts/generate_bignum_tests.py
generated_bignum_test_data: ../scripts/mbedtls_dev/test_case.py
-generated_bignum_test_data: ../scripts/mbedtls_dev/test_generation.py
+generated_bignum_test_data: ../scripts/mbedtls_dev/test_data_generation.py
generated_bignum_test_data:
echo " Gen $(GENERATED_BIGNUM_DATA_FILES)"
$(PYTHON) scripts/generate_bignum_tests.py
@@ -104,7 +104,7 @@ generated_psa_test_data: ../scripts/mbedtls_dev/crypto_knowledge.py
generated_psa_test_data: ../scripts/mbedtls_dev/macro_collector.py
generated_psa_test_data: ../scripts/mbedtls_dev/psa_storage.py
generated_psa_test_data: ../scripts/mbedtls_dev/test_case.py
-generated_psa_test_data: ../scripts/mbedtls_dev/test_generation.py
+generated_psa_test_data: ../scripts/mbedtls_dev/test_data_generation.py
## The generated file only depends on the options that are present in
## crypto_config.h, not on which options are set. To avoid regenerating this
## file all the time when switching between configurations, don't declare
diff --git a/tests/scripts/generate_bignum_tests.py b/tests/scripts/generate_bignum_tests.py
index ceafa4a489..1cd859c4af 100755
--- a/tests/scripts/generate_bignum_tests.py
+++ b/tests/scripts/generate_bignum_tests.py
@@ -6,7 +6,7 @@ generate only the specified files.
Class structure:
-Child classes of test_generation.BaseTarget (file targets) represent an output
+Child classes of test_data_generation.BaseTarget (file targets) represent an output
file. These indicate where test cases will be written to, for all subclasses of
this target. Multiple file targets should not reuse a `target_basename`.
@@ -36,7 +36,7 @@ following:
call `.create_test_case()` to yield the TestCase.
Additional details and other attributes/methods are given in the documentation
-of BaseTarget in test_generation.py.
+of BaseTarget in test_data_generation.py.
"""
# Copyright The Mbed TLS Contributors
@@ -63,7 +63,7 @@ from typing import Iterator, List, Tuple, TypeVar
import scripts_path # pylint: disable=unused-import
from mbedtls_dev import test_case
-from mbedtls_dev import test_generation
+from mbedtls_dev import test_data_generation
T = TypeVar('T') #pylint: disable=invalid-name
@@ -85,7 +85,7 @@ def combination_pairs(values: List[T]) -> List[Tuple[T, T]]:
)
-class BignumTarget(test_generation.BaseTarget, metaclass=ABCMeta):
+class BignumTarget(test_data_generation.BaseTarget, metaclass=ABCMeta):
#pylint: disable=abstract-method
"""Target for bignum (mpi) test case generation."""
target_basename = 'test_suite_mpi.generated'
@@ -235,4 +235,4 @@ class BignumAdd(BignumOperation):
if __name__ == '__main__':
# Use the section of the docstring relevant to the CLI as description
- test_generation.main(sys.argv[1:], "\n".join(__doc__.splitlines()[:4]))
+ test_data_generation.main(sys.argv[1:], "\n".join(__doc__.splitlines()[:4]))
diff --git a/tests/scripts/generate_psa_tests.py b/tests/scripts/generate_psa_tests.py
index c788fd76b6..2f0900757b 100755
--- a/tests/scripts/generate_psa_tests.py
+++ b/tests/scripts/generate_psa_tests.py
@@ -30,7 +30,7 @@ from mbedtls_dev import crypto_knowledge
from mbedtls_dev import macro_collector
from mbedtls_dev import psa_storage
from mbedtls_dev import test_case
-from mbedtls_dev import test_generation
+from mbedtls_dev import test_data_generation
def psa_want_symbol(name: str) -> str:
@@ -892,7 +892,7 @@ class StorageFormatV0(StorageFormat):
yield from super().generate_all_keys()
yield from self.all_keys_for_implicit_usage()
-class PSATestGenerator(test_generation.TestGenerator):
+class PSATestGenerator(test_data_generation.TestGenerator):
"""Test generator subclass including PSA targets and info."""
# Note that targets whose names contain 'test_format' have their content
# validated by `abi_check.py`.
@@ -917,4 +917,4 @@ class PSATestGenerator(test_generation.TestGenerator):
super().generate_target(name, self.info)
if __name__ == '__main__':
- test_generation.main(sys.argv[1:], __doc__, PSATestGenerator)
+ test_data_generation.main(sys.argv[1:], __doc__, PSATestGenerator)
From 049042586e176abc215b0a57b8af1be5398981d6 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 16 Sep 2022 22:02:37 +0200
Subject: [PATCH 02/10] Clarify the descriptions of test-case-data-related
modules
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
scripts/mbedtls_dev/test_case.py | 2 +-
scripts/mbedtls_dev/test_data_generation.py | 5 ++++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/mbedtls_dev/test_case.py b/scripts/mbedtls_dev/test_case.py
index 6a46e4209b..d8f7b60040 100644
--- a/scripts/mbedtls_dev/test_case.py
+++ b/scripts/mbedtls_dev/test_case.py
@@ -1,4 +1,4 @@
-"""Library for generating Mbed TLS test data.
+"""Library for constructing an Mbed TLS test case.
"""
# Copyright The Mbed TLS Contributors
diff --git a/scripts/mbedtls_dev/test_data_generation.py b/scripts/mbedtls_dev/test_data_generation.py
index a88425f46a..f8e86ed8c6 100644
--- a/scripts/mbedtls_dev/test_data_generation.py
+++ b/scripts/mbedtls_dev/test_data_generation.py
@@ -1,4 +1,7 @@
-"""Common test generation classes and main function.
+"""Common code for test data generation.
+
+This module defines classes that are of general use to automatically
+generate .data files for unit tests, as well as a main function.
These are used both by generate_psa_tests.py and generate_bignum_tests.py.
"""
From 7b3fa657afaf856e76ad8f762cdbfe6ff90e22b9 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 16 Sep 2022 22:22:53 +0200
Subject: [PATCH 03/10] generate_*_tests.py --directory: fix handling of
relative path
The option to --directory was intended to be relative to the current
directory when the script is invoked, which is the intuitive behavior. But
this was not implemented correctly, and it was actually interpreted relative
to the mbedtls root (which the script chdir's into). Fix this.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
scripts/mbedtls_dev/test_data_generation.py | 24 +++++++++++----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/scripts/mbedtls_dev/test_data_generation.py b/scripts/mbedtls_dev/test_data_generation.py
index f8e86ed8c6..36c9475056 100644
--- a/scripts/mbedtls_dev/test_data_generation.py
+++ b/scripts/mbedtls_dev/test_data_generation.py
@@ -141,8 +141,7 @@ class BaseTarget(metaclass=ABCMeta):
class TestGenerator:
"""Generate test cases and write to data files."""
def __init__(self, options) -> None:
- self.test_suite_directory = self.get_option(options, 'directory',
- 'tests/suites')
+ self.test_suite_directory = options.directory
# Update `targets` with an entry for each child class of BaseTarget.
# Each entry represents a file generated by the BaseTarget framework,
# and enables generating the .data files using the CLI.
@@ -151,11 +150,6 @@ class TestGenerator:
for subclass in BaseTarget.__subclasses__()
})
- @staticmethod
- def get_option(options, name: str, default: T) -> T:
- value = getattr(options, name, None)
- return default if value is None else value
-
def filename_for(self, basename: str) -> str:
"""The location of the data file with the specified base name."""
return posixpath.join(self.test_suite_directory, basename + '.data')
@@ -189,16 +183,24 @@ def main(args, description: str, generator_class: Type[TestGenerator] = TestGene
help='List available targets and exit')
parser.add_argument('--list-for-cmake', action='store_true',
help='Print \';\'-separated list of available targets and exit')
+ # If specified explicitly, this option may be a path relative to the
+ # current directory when the script is invoked. The default value
+ # is relative to the mbedtls root, which we don't know yet. So we
+ # can't set a string as the default value here.
parser.add_argument('--directory', metavar='DIR',
help='Output directory (default: tests/suites)')
- # The `--directory` option is interpreted relative to the directory from
- # which the script is invoked, but the default is relative to the root of
- # the mbedtls tree. The default should not be set above, but instead after
- # `build_tree.chdir_to_root()` is called.
parser.add_argument('targets', nargs='*', metavar='TARGET',
help='Target file to generate (default: all; "-": none)')
options = parser.parse_args(args)
+
+ # Change to the mbedtls root, to keep things simple. But first, adjust
+ # command line options that might be relative paths.
+ if options.directory is None:
+ options.directory = 'tests/suites'
+ else:
+ options.directory = os.path.abspath(options.directory)
build_tree.chdir_to_root()
+
generator = generator_class(options)
if options.list:
for name in sorted(generator.targets):
From 4537d6d8385b68550199e67367e15e94f5ac1942 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 16 Sep 2022 22:26:38 +0200
Subject: [PATCH 04/10] Move implementation detail from docstring to comment
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
tests/scripts/generate_bignum_tests.py | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/tests/scripts/generate_bignum_tests.py b/tests/scripts/generate_bignum_tests.py
index 1cd859c4af..091630decc 100755
--- a/tests/scripts/generate_bignum_tests.py
+++ b/tests/scripts/generate_bignum_tests.py
@@ -74,11 +74,9 @@ def quote_str(val) -> str:
return "\"{}\"".format(val)
def combination_pairs(values: List[T]) -> List[Tuple[T, T]]:
- """Return all pair combinations from input values.
-
- The return value is cast, as older versions of mypy are unable to derive
- the specific type returned by itertools.combinations_with_replacement.
- """
+ """Return all pair combinations from input values."""
+ # The return value is cast, as older versions of mypy are unable to derive
+ # the specific type returned by itertools.combinations_with_replacement.
return typing.cast(
List[Tuple[T, T]],
list(itertools.combinations_with_replacement(values, 2))
From 15997bd389278b09afa69468bb454662e948a56e Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 16 Sep 2022 22:35:18 +0200
Subject: [PATCH 05/10] Use relative imports when importing other modules in
the same directory
We were using absolute imports under the assumption that the /scripts
directory is in the path. This worked in normal use because every one of our
Python scripts either were in the /scripts directory, or added the /scripts
directory to the module search path in order to reference mbedtls_dev.
However, this broke things like
```
python3 -m unittest scripts/mbedtls_dev/psa_storage.py
```
Fix this by using relative imports.
Relative imports are only supposed to be used inside a package (Python
doesn't complain, but Pylint does). So make /scripts/mbedtls_dev a proper
package by creating __init__.py.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
scripts/mbedtls_dev/__init__.py | 3 +++
scripts/mbedtls_dev/crypto_knowledge.py | 2 +-
scripts/mbedtls_dev/psa_storage.py | 2 +-
scripts/mbedtls_dev/test_case.py | 2 +-
scripts/mbedtls_dev/test_data_generation.py | 4 ++--
5 files changed, 8 insertions(+), 5 deletions(-)
create mode 100644 scripts/mbedtls_dev/__init__.py
diff --git a/scripts/mbedtls_dev/__init__.py b/scripts/mbedtls_dev/__init__.py
new file mode 100644
index 0000000000..c5bddaf74b
--- /dev/null
+++ b/scripts/mbedtls_dev/__init__.py
@@ -0,0 +1,3 @@
+# This file needs to exist to make mbedtls_dev a package.
+# Among other things, this allows modules in this directory to make
+# relative impotrs.
diff --git a/scripts/mbedtls_dev/crypto_knowledge.py b/scripts/mbedtls_dev/crypto_knowledge.py
index 592fc0afe2..1ce549903a 100644
--- a/scripts/mbedtls_dev/crypto_knowledge.py
+++ b/scripts/mbedtls_dev/crypto_knowledge.py
@@ -22,7 +22,7 @@ import enum
import re
from typing import FrozenSet, Iterable, List, Optional, Tuple
-from mbedtls_dev.asymmetric_key_data import ASYMMETRIC_KEY_DATA
+from .asymmetric_key_data import ASYMMETRIC_KEY_DATA
def short_expression(original: str, level: int = 0) -> str:
diff --git a/scripts/mbedtls_dev/psa_storage.py b/scripts/mbedtls_dev/psa_storage.py
index a06dce13ba..bae99383dc 100644
--- a/scripts/mbedtls_dev/psa_storage.py
+++ b/scripts/mbedtls_dev/psa_storage.py
@@ -26,7 +26,7 @@ import struct
from typing import Dict, List, Optional, Set, Union
import unittest
-from mbedtls_dev import c_build_helper
+from . import c_build_helper
class Expr:
diff --git a/scripts/mbedtls_dev/test_case.py b/scripts/mbedtls_dev/test_case.py
index d8f7b60040..43ddf203b6 100644
--- a/scripts/mbedtls_dev/test_case.py
+++ b/scripts/mbedtls_dev/test_case.py
@@ -21,7 +21,7 @@ import os
import sys
from typing import Iterable, List, Optional
-from mbedtls_dev import typing_util
+from . import typing_util
def hex_string(data: bytes) -> str:
return '"' + binascii.hexlify(data).decode('ascii') + '"'
diff --git a/scripts/mbedtls_dev/test_data_generation.py b/scripts/mbedtls_dev/test_data_generation.py
index 36c9475056..cdb1c03b89 100644
--- a/scripts/mbedtls_dev/test_data_generation.py
+++ b/scripts/mbedtls_dev/test_data_generation.py
@@ -29,8 +29,8 @@ import re
from abc import ABCMeta, abstractmethod
from typing import Callable, Dict, Iterable, Iterator, List, Type, TypeVar
-from mbedtls_dev import build_tree
-from mbedtls_dev import test_case
+from . import build_tree
+from . import test_case
T = TypeVar('T') #pylint: disable=invalid-name
From d9071e7d96d99a066ab9253b72e7cb98dc17770c Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Sun, 18 Sep 2022 21:17:09 +0200
Subject: [PATCH 06/10] Unify check_repo_path
We had 4 identical copies of the check_repo_path function. Replace them by a
single copy in the build_tree module where it naturally belongs.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
scripts/abi_check.py | 9 +++------
scripts/code_size_compare.py | 10 ++++------
scripts/mbedtls_dev/build_tree.py | 7 +++++++
tests/scripts/check_files.py | 10 ++++------
tests/scripts/check_names.py | 15 +++++----------
5 files changed, 23 insertions(+), 28 deletions(-)
diff --git a/scripts/abi_check.py b/scripts/abi_check.py
index c2288432ce..ac1d60ffd0 100755
--- a/scripts/abi_check.py
+++ b/scripts/abi_check.py
@@ -113,6 +113,8 @@ from types import SimpleNamespace
import xml.etree.ElementTree as ET
+from mbedtls_dev import build_tree
+
class AbiChecker:
"""API and ABI checker."""
@@ -150,11 +152,6 @@ class AbiChecker:
self.git_command = "git"
self.make_command = "make"
- @staticmethod
- def check_repo_path():
- if not all(os.path.isdir(d) for d in ["include", "library", "tests"]):
- raise Exception("Must be run from Mbed TLS root")
-
def _setup_logger(self):
self.log = logging.getLogger()
if self.verbose:
@@ -540,7 +537,7 @@ class AbiChecker:
def check_for_abi_changes(self):
"""Generate a report of ABI differences
between self.old_rev and self.new_rev."""
- self.check_repo_path()
+ build_tree.check_repo_path()
if self.check_api or self.check_abi:
self.check_abi_tools_are_installed()
self._get_abi_dump_for_ref(self.old_version)
diff --git a/scripts/code_size_compare.py b/scripts/code_size_compare.py
index 0ef438db7c..af6ddd4fcb 100755
--- a/scripts/code_size_compare.py
+++ b/scripts/code_size_compare.py
@@ -30,6 +30,9 @@ import os
import subprocess
import sys
+from mbedtls_dev import build_tree
+
+
class CodeSizeComparison:
"""Compare code size between two Git revisions."""
@@ -51,11 +54,6 @@ class CodeSizeComparison:
self.git_command = "git"
self.make_command = "make"
- @staticmethod
- def check_repo_path():
- if not all(os.path.isdir(d) for d in ["include", "library", "tests"]):
- raise Exception("Must be run from Mbed TLS root")
-
@staticmethod
def validate_revision(revision):
result = subprocess.check_output(["git", "rev-parse", "--verify",
@@ -172,7 +170,7 @@ class CodeSizeComparison:
def get_comparision_results(self):
"""Compare size of library/*.o between self.old_rev and self.new_rev,
and generate the result file."""
- self.check_repo_path()
+ build_tree.check_repo_path()
self._get_code_size_for_rev(self.old_rev)
self._get_code_size_for_rev(self.new_rev)
return self.compare_code_size()
diff --git a/scripts/mbedtls_dev/build_tree.py b/scripts/mbedtls_dev/build_tree.py
index 3920d0ed6c..f52b785d95 100644
--- a/scripts/mbedtls_dev/build_tree.py
+++ b/scripts/mbedtls_dev/build_tree.py
@@ -25,6 +25,13 @@ def looks_like_mbedtls_root(path: str) -> bool:
return all(os.path.isdir(os.path.join(path, subdir))
for subdir in ['include', 'library', 'programs', 'tests'])
+def check_repo_path():
+ """
+ Check that the current working directory is the project root, and throw
+ an exception if not.
+ """
+ if not all(os.path.isdir(d) for d in ["include", "library", "tests"]):
+ raise Exception("This script must be run from Mbed TLS root")
def chdir_to_root() -> None:
"""Detect the root of the Mbed TLS source tree and change to it.
diff --git a/tests/scripts/check_files.py b/tests/scripts/check_files.py
index a0f5e1f538..5c18702def 100755
--- a/tests/scripts/check_files.py
+++ b/tests/scripts/check_files.py
@@ -34,6 +34,9 @@ try:
except ImportError:
pass
+import scripts_path # pylint: disable=unused-import
+from mbedtls_dev import build_tree
+
class FileIssueTracker:
"""Base class for file-wide issue tracking.
@@ -338,7 +341,7 @@ class IntegrityChecker:
"""Instantiate the sanity checker.
Check files under the current directory.
Write a report of issues to log_file."""
- self.check_repo_path()
+ build_tree.check_repo_path()
self.logger = None
self.setup_logger(log_file)
self.issues_to_check = [
@@ -353,11 +356,6 @@ class IntegrityChecker:
MergeArtifactIssueTracker(),
]
- @staticmethod
- def check_repo_path():
- if not all(os.path.isdir(d) for d in ["include", "library", "tests"]):
- raise Exception("Must be run from Mbed TLS root")
-
def setup_logger(self, log_file, level=logging.INFO):
self.logger = logging.getLogger()
self.logger.setLevel(level)
diff --git a/tests/scripts/check_names.py b/tests/scripts/check_names.py
index e204487290..aece1ef060 100755
--- a/tests/scripts/check_names.py
+++ b/tests/scripts/check_names.py
@@ -56,6 +56,10 @@ import shutil
import subprocess
import logging
+import scripts_path # pylint: disable=unused-import
+from mbedtls_dev import build_tree
+
+
# Naming patterns to check against. These are defined outside the NameCheck
# class for ease of modification.
PUBLIC_MACRO_PATTERN = r"^(MBEDTLS|PSA)_[0-9A-Z_]*[0-9A-Z]$"
@@ -219,7 +223,7 @@ class CodeParser():
"""
def __init__(self, log):
self.log = log
- self.check_repo_path()
+ build_tree.check_repo_path()
# Memo for storing "glob expression": set(filepaths)
self.files = {}
@@ -228,15 +232,6 @@ class CodeParser():
# Note that "*" can match directory separators in exclude lists.
self.excluded_files = ["*/bn_mul", "*/compat-2.x.h"]
- @staticmethod
- def check_repo_path():
- """
- Check that the current working directory is the project root, and throw
- an exception if not.
- """
- if not all(os.path.isdir(d) for d in ["include", "library", "tests"]):
- raise Exception("This script must be run from Mbed TLS root")
-
def comprehensive_parse(self):
"""
Comprehensive ("default") function to call each parsing function and
From e188734f514939fa148a3d961a935f50bf8effe1 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Sun, 18 Sep 2022 21:27:37 +0200
Subject: [PATCH 07/10] Don't use parallel jobs for pylint
When pylint runs in parallel, it loses the ability to detect duplicated code
across modules. Duplicated code is usually a bad thing, so give pylint the
opportunity to let us know.
This makes pylint slightly slower, but going from 2 threads to 1 does not
make it anywhere close to twice as slow. On my machine, with Python 3.5,
pylint -j2 takes about 12s while single-threaded pylint takes about 16s
of wall clock time.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
tests/scripts/check-python-files.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/scripts/check-python-files.sh b/tests/scripts/check-python-files.sh
index dbf0365325..35319d3e1d 100755
--- a/tests/scripts/check-python-files.sh
+++ b/tests/scripts/check-python-files.sh
@@ -67,7 +67,7 @@ elif [ "$1" = "--can-mypy" ]; then
fi
echo 'Running pylint ...'
-$PYTHON -m pylint -j 2 scripts/mbedtls_dev/*.py scripts/*.py tests/scripts/*.py || {
+$PYTHON -m pylint scripts/mbedtls_dev/*.py scripts/*.py tests/scripts/*.py || {
echo >&2 "pylint reported errors"
ret=1
}
From cca6ce882924951e75d14979241bacd7cb4d09a2 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Sun, 18 Sep 2022 23:08:38 +0200
Subject: [PATCH 08/10] Fix directory mixup with generated data files
CMakeLists.txt was calling generate_psa_tests.py and siblings to list the
generated test data files with a --directory option, intended the output to
be this argument textually. This used to work, but no longer does, because
the --directory argument is relative to the current directory when the
Python script is invoked, and the script now shows an absolute path.
CMakeLists.txt now completely ignores the directory part of the listed data
file paths and builds its own. The base_xxx_files variables now contain
actual base names, without a "suites/" prefix. This makes it more robust
with respect to the behavior of the Python script, but it will break if
we put data files in multiple different directories one day.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
tests/CMakeLists.txt | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index b518e5833c..d89542a44d 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -16,38 +16,44 @@ endif()
# generated .data files will go there
file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/suites)
-# Get base names for generated files (starting at "suites/")
+# Get base names for generated files
execute_process(
COMMAND
${MBEDTLS_PYTHON_EXECUTABLE}
${CMAKE_CURRENT_SOURCE_DIR}/../tests/scripts/generate_bignum_tests.py
--list-for-cmake
- --directory suites
WORKING_DIRECTORY
${CMAKE_CURRENT_SOURCE_DIR}/..
OUTPUT_VARIABLE
base_bignum_generated_data_files)
+string(REGEX REPLACE "[^;]*/" ""
+ base_bignum_generated_data_files "${base_bignum_generated_data_files}")
execute_process(
COMMAND
${MBEDTLS_PYTHON_EXECUTABLE}
${CMAKE_CURRENT_SOURCE_DIR}/../tests/scripts/generate_psa_tests.py
--list-for-cmake
- --directory suites
WORKING_DIRECTORY
${CMAKE_CURRENT_SOURCE_DIR}/..
OUTPUT_VARIABLE
base_psa_generated_data_files)
+string(REGEX REPLACE "[^;]*/" ""
+ base_psa_generated_data_files "${base_psa_generated_data_files}")
-# Derive generated file paths in the build directory
-set(base_generated_data_files ${base_bignum_generated_data_files} ${base_psa_generated_data_files})
+# Derive generated file paths in the build directory. The generated data
+# files go into the suites/ subdirectory.
+set(base_generated_data_files
+ ${base_bignum_generated_data_files} ${base_psa_generated_data_files})
+string(REGEX REPLACE "([^;]+)" "suites/\\1"
+ all_generated_data_files "${base_generated_data_files}")
set(bignum_generated_data_files "")
set(psa_generated_data_files "")
foreach(file ${base_bignum_generated_data_files})
- list(APPEND bignum_generated_data_files ${CMAKE_CURRENT_BINARY_DIR}/${file})
+ list(APPEND bignum_generated_data_files ${CMAKE_CURRENT_BINARY_DIR}/suites/${file})
endforeach()
foreach(file ${base_psa_generated_data_files})
- list(APPEND psa_generated_data_files ${CMAKE_CURRENT_BINARY_DIR}/${file})
+ list(APPEND psa_generated_data_files ${CMAKE_CURRENT_BINARY_DIR}/suites/${file})
endforeach()
if(GEN_FILES)
@@ -87,7 +93,7 @@ if(GEN_FILES)
)
else()
- foreach(file ${base_generated_data_files})
+ foreach(file ${all_generated_data_files})
link_to_source(${file})
endforeach()
endif()
@@ -210,9 +216,9 @@ if(MSVC)
endif(MSVC)
file(GLOB test_suites RELATIVE "${CMAKE_CURRENT_SOURCE_DIR}" suites/*.data)
-list(APPEND test_suites ${base_generated_data_files})
+list(APPEND test_suites ${all_generated_data_files})
# If the generated .data files are present in the source tree, we just added
-# them twice, both through GLOB and through ${base_generated_data_files}.
+# them twice, both through GLOB and through ${all_generated_data_files}.
list(REMOVE_DUPLICATES test_suites)
list(SORT test_suites)
foreach(test_suite ${test_suites})
From b3ea98c60629124ab8b96dad012b7c45b944cdd9 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Wed, 21 Sep 2022 22:00:06 +0200
Subject: [PATCH 09/10] Replace the output file atomically
When writing the new .data file, first write the new content, then replace
the target. This way, there isn't a temporary state in which the file is
partially written. This temporary state can be misleading if the build is
interrupted. It's annoying if you're watching changes to the output and the
changes appear as emptying the file following by the new version appearing.
Now interrupted builds don't leave a file that appears to be up to date but
isn't, and when watching the output, there's a single transition to the new
version.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
scripts/mbedtls_dev/test_case.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/mbedtls_dev/test_case.py b/scripts/mbedtls_dev/test_case.py
index 43ddf203b6..8f08703678 100644
--- a/scripts/mbedtls_dev/test_case.py
+++ b/scripts/mbedtls_dev/test_case.py
@@ -92,9 +92,11 @@ def write_data_file(filename: str,
"""
if caller is None:
caller = os.path.basename(sys.argv[0])
- with open(filename, 'w') as out:
+ tempfile = filename + '.new'
+ with open(tempfile, 'w') as out:
out.write('# Automatically generated by {}. Do not edit!\n'
.format(caller))
for tc in test_cases:
tc.write(out)
out.write('\n# End of automatically generated file.\n')
+ os.replace(tempfile, filename)
From 31a8815f2541c9544ebeace1c7f2c28ab1dc0eaa Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 29 Sep 2022 18:48:41 +0200
Subject: [PATCH 10/10] Documentation typo
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
---
scripts/mbedtls_dev/__init__.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/mbedtls_dev/__init__.py b/scripts/mbedtls_dev/__init__.py
index c5bddaf74b..15b0d60dd3 100644
--- a/scripts/mbedtls_dev/__init__.py
+++ b/scripts/mbedtls_dev/__init__.py
@@ -1,3 +1,3 @@
# This file needs to exist to make mbedtls_dev a package.
# Among other things, this allows modules in this directory to make
-# relative impotrs.
+# relative imports.