Modify check generated files script to work with TF PSA Crypto too #8523

tom-daubney-arm · 2023-11-13T10:10:08Z

Description

Modify check generated files script to work with TF PSA Crypto as well as Mbed TLS

Note for reviewers: I am not sure if I am calling all the necessary checks for TF-PSA-Crypto. For example I am unsure if /scripts/generate_driver_wrappers.py needs to be checked on the TF-PSA-Crypto side. If it does then this will need adapting either as part of this work or separately.

Also please note, the first commit removes some extraneous whitespace in lcov.sh that I noticed this morning.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog not required - non user-visible change
backport not required - enhancement
tests not required - sufficient testing already in place

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Signed-off-by: Thomas Daubney <[email protected]>

Make the script work in both Mbed TLS and TF PSA Crypto. Signed-off-by: Thomas Daubney <[email protected]>

gilles-peskine-arm · 2023-11-13T11:11:09Z

tests/scripts/check-generated-files.sh

+IN_MBEDTLS=0
+if [ -d library -a -d include -a -d tests ]; then
+    IN_MBEDTLS=1
+elif [ -d core -a -d include -a -d tests ]; then :;


Deciding between mbedtls and tf-psa-crypto from library vs core feels fragile. How about testing the presence of include/mbedtls vs include/tf_psa_crypto?

Please do it like in all.sh in this PR to keep things consistent. include/tf_psa_crypto was introduced recently thus not used yet in the repo root detection checks. We could use it and in that case we would update all such checks in all *.sh files.

Sure, will do. I did think that however I thought the way I was doing it here look more consistent within this particular script. Happy to change it though

Should we introduce a scripts/which_repo.sh? Or a scripts/repo_name.txt?

For Python code there should be a function in build_tree.py.

@gilles-peskine-arm I am just wondering why doing it this way seems more fragile than using the include directory as you suggest? It's not clear to me why one approach is more robust than the other.

@gilles-peskine-arm By "use a regular file" do you mean we should simply have a file named mbedtls in the root directory of Mbed TLS and then the same idea of TF-PSA-Crypto and then we test for it's existence with the proposed scripts/which-repo.sh script?

No, I thought each repository should have a file with the same name, and we'd use the content of that file to identify the repository.

Cool ok. In this PR I think it is probably best if I do it the way @ronald-cron-arm suggests in his first comment and then I will raise a subsequent PR that implements this file idea and modifies how repo detection is done across both the repositories. Does that sound alright to everyone here?

The standardisation of repository detection is now being tracked with issue #8524 .

Add repository detection and conditional setting of library_dir variable. Signed-off-by: Thomas Daubney <[email protected]>

Add further modifications to repo detection and calling the checks. Signed-off-by: Thomas Daubney <[email protected]>

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm · 2023-11-14T12:08:39Z

Note to reviewers: I have changed the repo detection in tests/scripts/check-generated-files.sh as discussed and also made a minimal change the scripts/generate_driver_wrappers.py to get it to work in both the repositories. After this I will work on issue #8524 to unify how repo detection is done.

scripts/generate_driver_wrappers.py

tests/scripts/check-generated-files.sh

Signed-off-by: Thomas Daubney <[email protected]>

Correct erroneous function call Signed-off-by: Thomas Daubney <[email protected]>

Signed-off-by: Thomas Daubney <[email protected]>

ronald-cron-arm · 2023-11-15T08:32:14Z

scripts/generate_driver_wrappers.py

@@ -178,8 +178,15 @@ def main() -> int:

    mbedtls_root = os.path.abspath(args.mbedtls_root)

+    library_dir = ''


It would be better to change mbedtls_root to repo_root in this file. Then comes up the question of what do we do with:

def_arg_mbedtls_root = build_tree.guess_mbedtls_root()

In build_tree.py, guess_mbedtls_root actually detects the mbedtls or TF-PSA-Crypto root.
Probably we should have in build_tree.py, three functions: guess_repo_root, guess_mbedtls_root and guess_tf_psa_crypto_root and use guess_repo_root here. We would then have:

def_arg_repo_root = build_tree.guess_repo_root()

Otherwise, please look at the occurrences of library in the file and update the ones that have to be updated.

Ultimately we may need different functions for when Mbed TLS has TF-PSA-crypto as a submodule: guess_repo_root vs guess_crypto_root (currently same as guess_repo_root). I don't think we need guess_mbedtls_root as part of the interface. But it can stay for a while as a transitional alias for guess_repo_root.

For implementation, we can have a single function that works in all cases. The goal of this function is just to find how many times to go to the ultimate parent directory, it doesn't care about the exact names of the toplevel directories.

And maybe we should introduce functions for crypto_core_directory and crypto_builtins_directory?

And maybe we should introduce functions for crypto_core_directory and crypto_builtins_directory?

In build_tree.py? I am not sure what you mean by that.

generate_driver_wrappers needs to know where to put its output: that's different in mbedtls vs tpc. This kind of knowledge is the job of build_tree.

Thus it would be a function in build_tree.py, let's say crypto_core_directory that would return the path of the library directory in the case of mbedtls (directory where the PSA core code is located in mbedtls) and the path to the core directory in the case of TF-PSA-Crypto?

The work on build_tree seems to get things a bit too far for this PR I'd say. I still feel the need to rename mbedtls_root to repo_root otherwise I would have the impression to let things in a bad state but probably ok to keep
def_arg_repo_root = build_tree.guess_mbedtls_root() for the time being. @tom-daubney-arm @gilles-peskine-arm what do you think?

This PR is adding dual-repo support to generate_driver_wrappers.py. This involves some new directory name detection code. (library vs core). Since this detection is not specific to generate_driver_wrappers.py, we should add it directly to build_tree.py rather than add it to generate_driver_wrappers.py and do extra work to move it later.

Thus, what about if we add a function crypto_core_directory in build_tree.py, that returns the path of the library directory in the case of mbedtls (directory that currently contains the PSA core code in mbedtls) and the path to the core directory in the case of TF-PSA-Crypto? Then, when mbedtls is restructured to host TF-PSA-Crypto in the mbedtls case the function will return the path to core as well.

@ronald-cron-arm This seems sensible to me. Any further work on repository detection is being tracked in #8524 and so can be sorted out when I do that ticket, which will be immediately after this ticket. Is that ok with you @gilles-peskine-arm ?

I've made what I think are the suggested improvements around variable naming. I may have gone a little further than was intended so I can walk it a back a bit if needed but I have scrubbed all mention of mbedtls in the variable names and functions that are local to scripts/generate_driver_wrappers.py.

Rename some variables in generate_driver_wrappers.py now that the script has to work in two repositories as opposed to just mbed tls. Signed-off-by: Thomas Daubney <[email protected]>

Add crypto_core_directory in build_tree.py so that the libary/core directory can be returned based on what repository we are in. Signed-off-by: Thomas Daubney <[email protected]>

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm · 2023-11-30T17:40:05Z

Note to reviewers that this is currently not ready for re-review. I still need to address one further issue which I will do tomorrow.

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm · 2023-12-04T09:45:18Z

Note for reviewers: I think the current CI failures here are a failed job rather than failed tests so please re-review when you can, thanks. @gilles-peskine-arm @ronald-cron-arm

gilles-peskine-arm

LGTM

gilles-peskine-arm · 2023-12-06T11:09:59Z

scripts/mbedtls_dev/build_tree.py

+        root = guess_project_root()
+    if looks_like_mbedtls_root(root):
+        return root


This won't find the mbedtls root if called from a TF-PSA-Crypto submodule of mbedtls. That's ok for now, we'll just have to improve it later.

Thanks for the heads up, and for the review.

scripts/generate_driver_wrappers.py

Signed-off-by: Thomas Daubney <[email protected]>

ronald-cron-arm

LGTM, thanks.

gilles-peskine-arm

LGTM

tom-daubney-arm added 2 commits November 13, 2023 09:50

Remove superfluous leading whitespace

b4f1ee0

Signed-off-by: Thomas Daubney <[email protected]>

Modify check-generated-files.sh to work in both repos

c9f8386

Make the script work in both Mbed TLS and TF PSA Crypto. Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm added enhancement needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-reviewer This PR needs someone to pick it up for review labels Nov 13, 2023

tom-daubney-arm requested review from ronald-cron-arm and gabor-mezei-arm November 13, 2023 10:10

gilles-peskine-arm reviewed Nov 13, 2023

View reviewed changes

tom-daubney-arm added needs-work priority-high High priority - will be reviewed soon and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 13, 2023

tom-daubney-arm added 3 commits November 14, 2023 12:02

Modify generate_driver_wrappers.py to work in both repos

b10cc7a

Add repository detection and conditional setting of library_dir variable. Signed-off-by: Thomas Daubney <[email protected]>

Further modify check-generated-files.sh

d3f8443

Add further modifications to repo detection and calling the checks. Signed-off-by: Thomas Daubney <[email protected]>

Remove further extraneous whitespace in lcov script

0bb761c

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Nov 14, 2023

ronald-cron-arm requested changes Nov 14, 2023

View reviewed changes

scripts/generate_driver_wrappers.py Outdated Show resolved Hide resolved

tests/scripts/check-generated-files.sh Outdated Show resolved Hide resolved

tests/scripts/check-generated-files.sh Outdated Show resolved Hide resolved

tom-daubney-arm added 3 commits November 14, 2023 15:16

Apply correct license to generate_driver_wrappers.py

c1750bb

Signed-off-by: Thomas Daubney <[email protected]>

Refactor repository detection

e58128e

Signed-off-by: Thomas Daubney <[email protected]>

Stylise TF-PSA-Crypto correctly

d289b8b

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm requested a review from ronald-cron-arm November 14, 2023 15:43

tom-daubney-arm added 2 commits November 14, 2023 16:59

Call the right function

0eb2dc1

Correct erroneous function call Signed-off-by: Thomas Daubney <[email protected]>

Remove trailing whitespace

4291bc2

Signed-off-by: Thomas Daubney <[email protected]>

ronald-cron-arm requested changes Nov 15, 2023

View reviewed changes

Rename variables in script

5556f90

Rename some variables in generate_driver_wrappers.py now that the script has to work in two repositories as opposed to just mbed tls. Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm requested a review from ronald-cron-arm November 15, 2023 16:53

tom-daubney-arm added the needs-work label Nov 16, 2023

Introduce function to return library/core directory

13ecb69

Add crypto_core_directory in build_tree.py so that the libary/core directory can be returned based on what repository we are in. Signed-off-by: Thomas Daubney <[email protected]>

Introduce guess_tf_psa_crypto_root

db80b23

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm added 2 commits December 1, 2023 09:52

Remove trailing whitespace

99030e2

Signed-off-by: Thomas Daubney <[email protected]>

Modify crypto_core_directory to also return a relative path

04c446c

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm removed the needs-work label Dec 1, 2023

tom-daubney-arm added 2 commits December 1, 2023 18:27

Use guess_mbedtls_root in Mbed-TLS-only script

3a06906

Signed-off-by: Thomas Daubney <[email protected]>

Fix bad whitespace in keyword argument assignment

10769bc

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels Dec 2, 2023

tom-daubney-arm requested a review from ronald-cron-arm December 4, 2023 09:45

tom-daubney-arm mentioned this pull request Dec 6, 2023

Add code style check Mbed-TLS/TF-PSA-Crypto#93

Open

gilles-peskine-arm previously approved these changes Dec 6, 2023

View reviewed changes

ronald-cron-arm linked an issue Dec 6, 2023 that may be closed by this pull request

Add generated file checks Mbed-TLS/TF-PSA-Crypto#50

Open

2 tasks

ronald-cron-arm requested changes Dec 8, 2023

View reviewed changes

scripts/generate_driver_wrappers.py Outdated Show resolved Hide resolved

Use existing variable containing full path

f05b768

Signed-off-by: Thomas Daubney <[email protected]>

tom-daubney-arm dismissed gilles-peskine-arm’s stale review via f05b768 December 8, 2023 09:48

tom-daubney-arm requested review from gilles-peskine-arm and ronald-cron-arm December 8, 2023 09:48

ronald-cron-arm approved these changes Dec 8, 2023

View reviewed changes

gilles-peskine-arm approved these changes Dec 11, 2023

View reviewed changes

gilles-peskine-arm 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 Dec 11, 2023

gilles-peskine-arm added this pull request to the merge queue Dec 11, 2023

Merged via the queue into Mbed-TLS:development with commit b4362d2 Dec 11, 2023
6 checks passed

tom-daubney-arm mentioned this pull request Dec 14, 2023

Add component for check_generated_files testing Mbed-TLS/TF-PSA-Crypto#91

Open

daverodgman mentioned this pull request Mar 18, 2024

Codestyle autogen fix #8939

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify check generated files script to work with TF PSA Crypto too #8523

Modify check generated files script to work with TF PSA Crypto too #8523

tom-daubney-arm commented Nov 13, 2023 •

edited

Loading

gilles-peskine-arm Nov 13, 2023

ronald-cron-arm Nov 13, 2023

tom-daubney-arm Nov 13, 2023

gilles-peskine-arm Nov 13, 2023

tom-daubney-arm Nov 13, 2023

tom-daubney-arm Nov 13, 2023

gilles-peskine-arm Nov 13, 2023

tom-daubney-arm Nov 13, 2023

ronald-cron-arm Nov 13, 2023

tom-daubney-arm Nov 13, 2023

tom-daubney-arm commented Nov 14, 2023

ronald-cron-arm Nov 15, 2023

gilles-peskine-arm Nov 15, 2023

ronald-cron-arm Nov 15, 2023 •

edited

Loading

gilles-peskine-arm Nov 15, 2023

ronald-cron-arm Nov 15, 2023 •

edited

Loading

ronald-cron-arm Nov 15, 2023

gilles-peskine-arm Nov 15, 2023

ronald-cron-arm Nov 15, 2023

tom-daubney-arm Nov 15, 2023

tom-daubney-arm Nov 15, 2023

tom-daubney-arm commented Nov 30, 2023 •

edited

Loading

tom-daubney-arm commented Dec 4, 2023

gilles-peskine-arm left a comment

gilles-peskine-arm Dec 6, 2023

tom-daubney-arm Dec 6, 2023

ronald-cron-arm left a comment

gilles-peskine-arm left a comment

		@@ -178,8 +178,15 @@ def main() -> int:

		mbedtls_root = os.path.abspath(args.mbedtls_root)

		library_dir = ''

Modify check generated files script to work with TF PSA Crypto too #8523

Modify check generated files script to work with TF PSA Crypto too #8523

Conversation

tom-daubney-arm commented Nov 13, 2023 • edited Loading

Description

PR checklist

Notes for the submitter

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-daubney-arm commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-daubney-arm commented Nov 30, 2023 • edited Loading

tom-daubney-arm commented Dec 4, 2023

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

tom-daubney-arm commented Nov 13, 2023 •

edited

Loading

ronald-cron-arm Nov 15, 2023 •

edited

Loading

ronald-cron-arm Nov 15, 2023 •

edited

Loading

tom-daubney-arm commented Nov 30, 2023 •

edited

Loading