Merge pull request #6296 from gilles-peskine-arm/test_data_generation-pr_6093_followup

Minor fixes to test_data_generation.py

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)

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()

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 imports.

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.

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:

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:

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
@@ -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') + '"'

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.
 """
@@ -26,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 . import build_tree
-from mbedtls_dev import test_case
+from . import test_case
 
 T = TypeVar('T') #pylint: disable=invalid-name
 
@@ -138,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',
+        self.test_suite_directory = options.directory
-                                                    'tests/suites')
         # 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.
@@ -148,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')
@@ -186,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):

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
+# Derive generated file paths in the build directory. The generated data
-set(base_generated_data_files ${base_bignum_generated_data_files} ${base_psa_generated_data_files})
+# 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)
@@ -63,7 +69,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,14 +86,14 @@ 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
     )
 
 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})

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

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
 }

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)

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

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
 
@@ -74,18 +74,16 @@ 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.
+    """Return all pair combinations from input values."""
-
+    # The return value is cast, as older versions of mypy are unable to derive
-    The return value is cast, as older versions of mypy are unable to derive
+    # the specific type returned by itertools.combinations_with_replacement.
-    the specific type returned by itertools.combinations_with_replacement.
-    """
     return typing.cast(
         List[Tuple[T, T]],
         list(itertools.combinations_with_replacement(values, 2))
     )
 
 
-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 +233,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]))

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)