Introduce config_checks_generator.py #196

gilles-peskine-arm · 2025-07-31T20:09:55Z

Infrastructure to generate config check headers, to be included in ${PROJECT}_config.c. This is just some basic logic, there will be a follow-up with more sophisticated check.

Contributes to Mbed-TLS/mbedtls#10147. See #164 for a the next step.

PR checklist

Please add the numbers (or links) of the associated pull requests for consuming branches. You can omit branches where this pull request is not needed.

TF-PSA-Crypto PR provided Introduce generated config checks in crypto TF-PSA-Crypto#404
development PR provided Introduce generated config checks in mbedtls mbedtls#10340
3.6 PR not required because: not getting included in 3.6

mpg

Looks pretty good to me, just a few minor points/thoughts.

scripts/mbedtls_framework/config_checks_generator.py

mpg

LGTM, thanks!

minosgalanakis

I have some minor comments at the design. I won't specifically request changes, but I would like to discuss them.

Otherwise the code is sensibe and does functioanlly what is expected to do

minosgalanakis · 2025-09-15T14:33:52Z

scripts/mbedtls_framework/config_checks_generator.py

+
+        Use the given prefix for auxiliary macro names.
+        """
+        methods = {


I am not sure if declaring a dictionary just to use it as an enum map is neccessary. Since we are matching the names we could get away with something like

method = getattr(self, position.name.lower())

That's shorter but I don't think it's clearer. And it's not type-safe. So I don't see it as better.

In general I would rather we refrain from using enums in python until we upgrade to 3.10 and have match, unless:

It is clearly an object that is meant to allign with an enum already in C

We save time and complexity

We need to refer to them using their name, or number, or iterate on their items in sequence.

Otherwise we are just adding complexity without much benefit. My proposal was geared towards implemeting 2.

Either way I won't block this Pr for the choice of tooling.

minosgalanakis · 2025-09-15T14:53:58Z

scripts/mbedtls_framework/config_checks_generator.py

+
+    def __init__(self, name: str, version: str, suggestion: str = '') -> None:
+        super().__init__(name, suggestion)
+        self.version = version


I would include the version in the Checker class, as an optional parameter and forgo the inheritance completely and put the subclass as an attribute which defines how the affected method change.

e.g

class Checker: """Description of checks for one option. By default, this class triggers an error if the option is set after def __init__(self, name: str, suggestion: str = '') -> None: self.name = name self.suggestion = suggestion self.version = version self.checker_type = self.determine_type(name, suggestion, version)

Of-course this seems to be the first of many so you have better scope of what this should do in its final form. I just suggest that if we could reduce complexity it would be worth it.

For clarity what I propose when invoked would look like:

from mbedtls_framework.config_checks_generator import * \ #pylint: disable=wildcard-import,unused-wildcard-import MBEDTLS_CHECKS = BranchData( header_directory='library', header_prefix='mbedtls_', project_cpp_prefix='MBEDTLS', checkers=[ checker('MBEDTLS_KEY_EXCHANGE_RSA_ENABLED', 'Mbed TLS 4.0', check_for="Removed") ], )

Of-course this seems to be the first of many so you have better scope of what this should do in its final form. I just suggest that if we could reduce complexity it would be worth it.

I'm not sure but I suspect that it only looks like it could be simpler because the classes here aren't the full story. Can you please take a look at #164 with the added classes (not ready for review yet, but I'm not planning any more changes to that part before 1.0), and see if your comment is still applicable there?

I see your point. Using inheritance may be the simplest and easier to maintain approach.

minosgalanakis · 2025-09-15T16:18:38Z

scripts/mbedtls_framework/config_checks_generator.py

+                      code, flags=re.M)
+
+
+class BranchData(typing.NamedTuple):


This is an immutable object, that is used as the source of thruth for the whole logic. I would consider using a dataclass

@dataclass(frozen=True) class BranchData:

p.s in that case we can also use the post_init to do input sanitatisation

What would be the benefits of a dataclass over a NamedTuple?

I only foresee a single place in each repository where we'll create an instance of that class, so I don't think we need to do any input sanitization.

Not much in that case, and depends on how we wish to use that.

If that a small immutable, input object, named.tuple is the best choice. If in the future there are plans to do things like conditional immutability, or input checking, then dataclass allows you to attach those extra functionalities.

a small immutable, input object

Right, here this is exactly what it is.

minosgalanakis · 2025-09-15T16:55:54Z

scripts/mbedtls_framework/config_checks_generator.py

+    for position in Position:
+        generator = HeaderGenerator(branch_data, position)
+        if list_only:
+            print(os.path.join(directory, generator.output_file_name()))


Looking at the companion pr

COMMAND ${TF_PSA_CRYPTO_PYTHON_EXECUTABLE} ${TF_PSA_CRYPTO_DIR}/scripts/generate_config_checks.py --list "" WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/.. OUTPUT_VARIABLE TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS) # Turn newline-terminated non-empty list into semicolon-separated list. string(REPLACE "\n" ";" TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS "${TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS}") string(REGEX REPLACE ";\$" "" TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS "${TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS}") # Prepend the binary dir to all element of TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS, # using features that exist in CMake 3.5.1. string(REPLACE ";" ";${CMAKE_CURRENT_BINARY_DIR}/" TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS "${TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS}") set(TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS "${CMAKE_CURRENT_BINARY_DIR}/${TF_PSA_CRYPTO_GENERATED_CONFIG_CHECKS_HEADERS}")

It could make sense to do this formatting in Pyhton rather than cmake?

maybe rename list_only to list_human, and add a new list_cmake for the properly formatted output?

Yeah, good point. Especially since the old CMake version on the CI lacks a lot of string manipulation features. We already have scripts with a --list-for-cmake option.

Thanks for adding this. I was thinking about it. and it would be nice ( not now, when we have bandwith ) to standardize it across the libraries and our scripting.

The need:

To differentiate between a user calling a python script, and the build system calling it.

Proposal:

We agree on a common data format, and an api that cmake can consume by default.

We add a boolean optional, disabled by default option to all scripts --cmake

We implement the changes, and remove complexity from the cmake side, only leaving behind a minimal set of common parsing utility methods.

If you agree it would be usefull I could make an issue.

But that is way beyond the scope the 1.0

Sounds good, but also, sounds like the kind of grand refactoring that we'll never find the time to do 😢

Infrastructure to generate config check headers, to be included in `${PROJECT}_config.c`. This commit creates a module with the core logic, and support for three generated files, to be included before the user config, just after the user config, and after finalizing the user config. This commit provides a basic `Checker` class. Future commits will add derived classes for common cases such as options set in the wrong project. This module is meant to be used in an executable script in each project, containing the project-specific names to use and the list of options. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

For the time being, only do this if the generation script `scripts/generate_config_checks.py` exists in the consuming repository. This way we can use this framework revision before the scripts are added to the consuming repositories. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Similar to `generate_*_tests.py`, let Python rather than Cmake worry about constructing a CMake list. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2025-09-15T18:02:54Z

This branch was too old to pass the CI, so I rebased in on top of the head of main. The previous version is available at https://github.com/gilles-peskine-arm/mbedtls-framework/tree/config-checks-generator-framework-2. I've also pushed a new commit "Add option --list-for-cmake".

minosgalanakis

LGTM

mpg

LGTM, thanks!

gilles-peskine-arm added this to Roadmap pull requests (new board) Jul 31, 2025

gilles-peskine-arm added needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Jul 31, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Jul 31, 2025

This was referenced Jul 31, 2025

Introduce generated config checks in crypto Mbed-TLS/TF-PSA-Crypto#404

Merged

Introduce generated config checks in mbedtls Mbed-TLS/mbedtls#10340

Merged

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Aug 2, 2025

mpg reviewed Sep 1, 2025

View reviewed changes

scripts/mbedtls_framework/config_checks_generator.py Show resolved Hide resolved

scripts/mbedtls_framework/config_checks_generator.py Outdated Show resolved Hide resolved

scripts/mbedtls_framework/config_checks_generator.py Show resolved Hide resolved

mpg previously approved these changes Sep 2, 2025

View reviewed changes

minosgalanakis self-requested a review September 15, 2025 10:54

minosgalanakis reviewed Sep 15, 2025

View reviewed changes

gilles-peskine-arm added 5 commits September 15, 2025 19:58

Pacify check_names.py

110e61a

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Improve comments

6235dbb

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add option --list-for-cmake

7ca97be

Similar to `generate_*_tests.py`, let Python rather than Cmake worry about constructing a CMake list. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm dismissed mpg’s stale review via 7ca97be September 15, 2025 18:01

gilles-peskine-arm force-pushed the config-checks-generator-framework branch from 9bcb4d0 to 7ca97be Compare September 15, 2025 18:01

mpg removed the needs-reviewer This PR needs someone to pick it up for review label Sep 16, 2025

minosgalanakis approved these changes Sep 16, 2025

View reviewed changes

mpg approved these changes Sep 16, 2025

View reviewed changes

github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Sep 16, 2025

mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 16, 2025

mpg merged commit e3c6f9d into Mbed-TLS:main Sep 16, 2025
4 checks passed

github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Sep 16, 2025

This was referenced Sep 16, 2025

Fix generate_config_checks.py not actually generating the files #211

Merged

Redesign of knowledge about generated files #215

Open

Introduce config_checks_generator.py #196

Introduce config_checks_generator.py #196

Conversation

gilles-peskine-arm commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minosgalanakis Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Sep 15, 2025

Uh oh!

minosgalanakis left a comment

Choose a reason for hiding this comment

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm commented Jul 31, 2025 •

edited

Loading

minosgalanakis Sep 16, 2025 •

edited

Loading