gh-Mbed-TLS/mbedtls-framework

Watch

Fork 0

Fix generate_config_checks.py not satisfying make_generated_files --check --root #212

Merged

gilles-peskine-arm merged 5 commits from config-checks-generator-framework-fix-check-generated_files into main

2025-09-17 11:04:44 -05:00

gilles-peskine-arm commented

2025-09-16 15:12:50 -05:00

(Migrated from github.com)

When the current directory is the project root, have --list print relative paths, rather than absolute paths that start with
build_treeguess_project_root().

This is not only nice to humans, but also necessary for make_generated_files.py --root DIR --check: it calls this script with --list and expects a path that is relative to DIR, not an absolute path that is under the project root.

Fix the build in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404, hopefully for real this time.

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 https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404
development PR provided https://github.com/Mbed-TLS/mbedtls/pull/10340
3.6 PR not required because: new stuff

When the current directory is the project root, have `--list` print relative paths, rather than absolute paths that start with `build_treeguess_project_root()`. This is not only nice to humans, but also necessary for `make_generated_files.py --root DIR --check`: it calls this script with `--list` and expects a path that is relative to DIR, not an absolute path that is under the project root. Fix the build in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404, hopefully for real this time. ## 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: new stuff

mpg (Migrated from github.com) reviewed

2025-09-16 15:12:50 -05:00

gilles-peskine-arm commented

2025-09-17 03:02:57 -05:00

(Migrated from github.com)

This fixes all.sh tf_psa_crypto_check_generated_files. But it's not enough to fix all.sh check_generated_files (the mbedtls component) in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404.

There, scripts/make_generated_files.py --check believes that the new crypto generated files exist, since scripts/generate_config_checks.py exists in crypto. But make in mbedtls doesn't yet know how to build them.

This fixes `all.sh tf_psa_crypto_check_generated_files`. But it's not enough to fix [`all.sh check_generated_files` (the mbedtls component)](https://mbedtls.trustedfirmware.org/job/mbed-tls-tf-psa-crypto-multibranch/job/PR-404-head/5/execution/node/2570/log/) in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404. There, `scripts/make_generated_files.py --check` believes that the new crypto generated files exist, since `scripts/generate_config_checks.py` exists in crypto. But `make` in mbedtls doesn't yet know how to build them.

gilles-peskine-arm (Migrated from github.com) reviewed

2025-09-17 04:42:50 -05:00

scripts/make_generated_files.py

Is this a hack? Yes. But also, it's solving a genuine design problem. Right now, we'll only need it temporarily between the merge of https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404 and the merge of https://github.com/Mbed-TLS/mbedtls/pull/10340. But conceptually, we'll need it again any time we create a new generation script in TF-PSA-Crypto. That doesn't happen often, but it may well happen before we solve the design problem with generated files.

I'll file an issue about the core problem once I've had a bit of time to think about what the requirements are and whether there are incremental solutions that could help.

mpg (Migrated from github.com) reviewed

2025-09-17 05:23:52 -05:00

mpg (Migrated from github.com) left a comment

No description provided.

LGTM.

mpg (Migrated from github.com) reviewed

2025-09-17 05:27:03 -05:00

scripts/make_generated_files.py

FWIW, I wouldn't call that a dirty hack. It's an ad hoc solution to a real problem. Is there a better solution? Perhaps. But I don't find the current state particularly dirty.

gilles-peskine-arm (Migrated from github.com) reviewed

2025-09-17 05:27:46 -05:00

scripts/make_generated_files.py

I've filed https://github.com/Mbed-TLS/mbedtls-framework/issues/215

gilles-peskine-arm commented

2025-09-17 05:37:04 -05:00

(Migrated from github.com)

This may still not be the final form because tests/scripts/all.sh -k tf_psa_crypto_check_generated_files is passing on my machine but failing on https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404. I'm trying to figure out why. In mbedtls that can be due to insufficient cleanup but in crypto the check is done only in a fresh out-of-source build tree.

This may still not be the final form because `tests/scripts/all.sh -k tf_psa_crypto_check_generated_files` is passing on my machine but failing on https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/404. I'm trying to figure out why. In mbedtls that can be due to insufficient cleanup but in crypto the check is done only in a fresh out-of-source build tree.

gilles-peskine-arm commented

2025-09-17 05:52:42 -05:00

(Migrated from github.com)

In mbedtls that can be due to insufficient cleanup but in crypto the check is done only in a fresh out-of-source build tree.

No, this is the mbedtls check that's failing. And yes, it is due to insufficient cleanup, I have reproduced it. https://github.com/Mbed-TLS/mbedtls-framework/pull/212/commits/e2c15fbd66ffe5063595d6f2a3ffdbe10124c3a8 isn't doing its intended job.

> In mbedtls that can be due to insufficient cleanup but in crypto the check is done only in a fresh out-of-source build tree. No, this is the mbedtls check that's failing. And yes, it is due to insufficient cleanup, I have reproduced it. https://github.com/Mbed-TLS/mbedtls-framework/pull/212/commits/e2c15fbd66ffe5063595d6f2a3ffdbe10124c3a8 isn't doing its intended job.

minosgalanakis (Migrated from github.com) reviewed

2025-09-17 10:13:29 -05:00

minosgalanakis (Migrated from github.com) left a comment

looks good, two minor comments.

scripts/make_generated_files.py

I also agree with @mpg that seems like a valid optional input (pun intended) to address a transitoriy edge-case.

scripts/make_generated_files.py

Any reason we don't use the same logic as bellow?

if generation_script.optional and not bak_file.exists():

Any reason we don't use the same logic as bellow? `if generation_script.optional and not bak_file.exists():`

scripts/make_generated_files.py

Similarly, why are are not throwing an exception as above?

raise Exception(f"Expected generated file does not exist: {file}")

Similarly, why are are not throwing an exception as above? `raise Exception(f"Expected generated file does not exist: {file}")`

gilles-peskine-arm (Migrated from github.com) reviewed

2025-09-17 10:32:47 -05:00

scripts/make_generated_files.py

Here there's also “if file does not exist then raise exception”, independently of the optional business. A file that doesn't exist here is an expected error (i.e. morally you expect the function/script to return false, not to crash) which should have a nice error message (the exception is a compromise: it was easier to patch in that print + keep track of the fact that an error occurred). Below, it's not expected that the backup file created earlier would no longer exist, so we just let the file function raise an error that's harder to interpret.

Here there's also “if file does not exist then raise exception”, independently of the optional business. A file that doesn't exist here is an expected error (i.e. morally you expect the function/script to return false, not to crash) which should have a nice error message (the exception is a compromise: it was easier to patch in that `print` + keep track of the fact that an error occurred). Below, it's not expected that the backup file created earlier would no longer exist, so we just let the file function raise an error that's harder to interpret.