gh-Mbed-TLS/mbedtls-framework
Watch
1
0
Fork
You've already forked mbedtls-framework
 0
Code Issues 39 Pull requests 42 Releases Activity

Introduce config_checks_generator.py #196

Merged
gilles-peskine-arm merged 5 commits from config-checks-generator-framework into main 2025-09-16 04:40:40 -05:00
Conversation 18 Commits 5 Files changed 2 +242
gilles-peskine-arm commented 2025-07-31 15:09:55 -05:00 (Migrated from github.com)

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 https://github.com/Mbed-TLS/mbedtls/issues/10147. See https://github.com/Mbed-TLS/mbedtls-framework/pull/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.

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 https://github.com/Mbed-TLS/mbedtls/issues/10147. See https://github.com/Mbed-TLS/mbedtls-framework/pull/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. - [x] **TF-PSA-Crypto PR** provided https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404 - [x] **development PR** provided https://github.com/Mbed-TLS/mbedtls/pull/10340 - [x] **3.6 PR** not required because: not getting included in 3.6
mpg (Migrated from github.com) reviewed 2025-09-01 04:19:58 -05:00
mpg (Migrated from github.com) left a comment

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

Looks pretty good to me, just a few minor points/thoughts.
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +1,228 @@
"""Generate C preprocessor code to check for bad configurations.
mpg (Migrated from github.com) commented 2025-08-29 05:31:16 -05:00

Typo: s/to/for/ ?

Typo: s/to/for/ ?
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +29,4 @@
class Checker:
"""Description of checks for one option.
By default, this class triggers an error if the option is set after
mpg (Migrated from github.com) commented 2025-08-29 05:29:37 -05:00

Minor: I think I'd slightly prefer if the base class did nothing, but OK.

Minor: I think I'd slightly prefer if the base class did nothing, but OK.
scripts/mbedtls_framework/config_checks_generator.py
mpg (Migrated from github.com) commented 2025-09-01 04:16:02 -05:00

Ok, may this is me not being object-oriented enough, but I don't find this approach of "get the text from the parent method and then patch it" very readable. I'd find it much more readable to just have

        return f'''
        #if defined({self.name}) //no-check-names
        #  error {self._quoted_message()}
        #endif
        '''

here directly, and I don't think that's an unacceptable amount of duplication. (I mean, it's 2 lines of pretty trivial code.)

Ok, may this is me not being object-oriented enough, but I don't find this approach of "get the text from the parent method and then patch it" very readable. I'd find it much more readable to just have ``` return f''' #if defined({self.name}) //no-check-names # error {self._quoted_message()} #endif ''' ``` here directly, and I don't think that's an unacceptable amount of duplication. (I mean, it's 2 lines of pretty trivial code.)
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-01 12:30:22 -05:00
scripts/mbedtls_framework/config_checks_generator.py
gilles-peskine-arm (Migrated from github.com) commented 2025-09-01 12:30:22 -05:00

Point taken about readability, but I do think the design makes sense. Conceptually, we want the same C code for a removed option as for an internal option. The only difference is the need to add an annotation for check-names.

I've added a comment to explain (I really should have done that from the start). Is that acceptable?

Point taken about readability, but I do think the design makes sense. Conceptually, we want the same C code for a removed option as for an internal option. The only difference is the need to add an annotation for check-names. I've added a comment to explain (I really should have done that from the start). Is that acceptable?
mpg (Migrated from github.com) reviewed 2025-09-02 04:03:42 -05:00
mpg (Migrated from github.com) left a comment
No description provided.
LGTM, thanks!
minosgalanakis (Migrated from github.com) reviewed 2025-09-15 11:37:21 -05:00
minosgalanakis (Migrated from github.com) left a comment

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

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
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +92,4 @@
Use the given prefix for auxiliary macro names.
"""
methods = {
minosgalanakis (Migrated from github.com) commented 2025-09-15 09:33:52 -05:00

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

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())`
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +111,4 @@
def __init__(self, name: str, version: str, suggestion: str = '') -> None:
super().__init__(name, suggestion)
self.version = version
minosgalanakis (Migrated from github.com) commented 2025-09-15 09:53:58 -05:00

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")
    ],
)
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") ], ) ~~~~~
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +131,4 @@
code, flags=re.M)
class BranchData(typing.NamedTuple):
minosgalanakis (Migrated from github.com) commented 2025-09-15 11:18:38 -05:00

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

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__](https://testdriven.io/tips/94ca0d2c-4c0d-4b8f-9762-6daa11d504f7/) to do input sanitatisation
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-15 11:51:11 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +131,4 @@
code, flags=re.M)
class BranchData(typing.NamedTuple):
gilles-peskine-arm (Migrated from github.com) commented 2025-09-15 11:51:11 -05:00

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.

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.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-15 11:52:32 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +92,4 @@
Use the given prefix for auxiliary macro names.
"""
methods = {
gilles-peskine-arm (Migrated from github.com) commented 2025-09-15 11:52:32 -05:00

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.

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.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-15 11:55:35 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +111,4 @@
def __init__(self, name: str, version: str, suggestion: str = '') -> None:
super().__init__(name, suggestion)
self.version = version
gilles-peskine-arm (Migrated from github.com) commented 2025-09-15 11:55:35 -05:00

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 https://github.com/Mbed-TLS/mbedtls-framework/pull/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?

> 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 https://github.com/Mbed-TLS/mbedtls-framework/pull/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?
minosgalanakis (Migrated from github.com) reviewed 2025-09-15 11:55:54 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +1,228 @@
"""Generate C preprocessor code to check for bad configurations.
minosgalanakis (Migrated from github.com) commented 2025-09-15 11:55:54 -05:00

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?

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?
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-15 12:01:37 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +1,228 @@
"""Generate C preprocessor code to check for bad configurations.
gilles-peskine-arm (Migrated from github.com) commented 2025-09-15 12:01:37 -05:00

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.

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.
gilles-peskine-arm commented 2025-09-15 13:02:54 -05:00 (Migrated from github.com)

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

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 (Migrated from github.com) reviewed 2025-09-16 04:16:53 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +92,4 @@
Use the given prefix for auxiliary macro names.
"""
methods = {
minosgalanakis (Migrated from github.com) commented 2025-09-16 04:16:52 -05:00

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

  1. It is clearly an object that is meant to allign with an enum already in C
  2. We save time and complexity
  3. 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.

In general I would rather we refrain from using enums in python until we upgrade to 3.10 and have [match](https://peps.python.org/pep-0636/), unless: 1. It is clearly an object that is meant to allign with an enum already in C 2. We save time and complexity 3. 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 (Migrated from github.com) reviewed 2025-09-16 04:18:38 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +111,4 @@
def __init__(self, name: str, version: str, suggestion: str = '') -> None:
super().__init__(name, suggestion)
self.version = version
minosgalanakis (Migrated from github.com) commented 2025-09-16 04:18:38 -05:00

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

I see your point. Using inheritance may be the simplest and easier to maintain approach.
minosgalanakis (Migrated from github.com) reviewed 2025-09-16 04:21:09 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +131,4 @@
code, flags=re.M)
class BranchData(typing.NamedTuple):
minosgalanakis (Migrated from github.com) commented 2025-09-16 04:21:09 -05:00

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.

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.
minosgalanakis (Migrated from github.com) reviewed 2025-09-16 04:25:49 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +1,228 @@
"""Generate C preprocessor code to check for bad configurations.
minosgalanakis (Migrated from github.com) commented 2025-09-16 04:25:49 -05:00

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

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**
minosgalanakis (Migrated from github.com) approved these changes 2025-09-16 04:26:34 -05:00
minosgalanakis (Migrated from github.com) left a comment

LGTM

LGTM
mpg (Migrated from github.com) approved these changes 2025-09-16 04:38:45 -05:00
mpg (Migrated from github.com) left a comment

LGTM, thanks!

LGTM, thanks!
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-16 04:45:47 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +131,4 @@
code, flags=re.M)
class BranchData(typing.NamedTuple):
gilles-peskine-arm (Migrated from github.com) commented 2025-09-16 04:45:47 -05:00

a small immutable, input object

Right, here this is exactly what it is.

> a small immutable, input object Right, here this is exactly what it is.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-16 04:46:57 -05:00
scripts/mbedtls_framework/config_checks_generator.py
@ -0,0 +1,228 @@
"""Generate C preprocessor code to check for bad configurations.
gilles-peskine-arm (Migrated from github.com) commented 2025-09-16 04:46:57 -05:00

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

Sounds good, but also, sounds like the kind of grand refactoring that we'll never find the time to do :cry:
Sign in to join this conversation.
Reviewers
No reviewers
minosgalanakis
mpg
Labels
Clear labels   
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good-first-issue
Good for newcomers

  
help-wanted
This issue is not being actively worked on, but PRs welcome.

  
historical-reviewed
Reviewed & agreed to keep legacy PR/issue

  
historical-reviewing
Currently reviewing (for legacy PR/issues)

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-ci
Needs to pass CI tests

  
needs-design-approval

   
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
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

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
approved
bug
dependencies
DO-NOT-MERGE
enhancement
good-first-issue
help-wanted
historical-reviewed
historical-reviewing
needs-adoption
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
single-reviewer
size-l
size-m
size-s
size-xs
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/mbedtls-framework!196
No description provided.