Miscellaneous changes to scripts for PSA-Crypto enablement #7955

davidhorstmann-arm · 2023-07-18T17:13:11Z

Enable the scripts to be used either in Mbed TLS or in the PSA Cryptography repository.

Fixes Mbed-TLS/TF-PSA-Crypto#23

Specifically:

Change root directory detection logic in several places
Change filepaths in several places dependent on which repo we are in

PR checklist

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

changelog not required - internal script changes only
backport not required - The PSA Crypto repo is based on development only
tests not required - changes are only to scripts

daverodgman · 2023-07-19T11:58:18Z

Test comment

ronald-cron-arm

This looks overall good to me, things to improve in test_psa_compliance.py though.

tests/scripts/all.sh

tests/scripts/test_psa_compliance.py

Introduce changes needed to run all.sh in the psa-crypto repo. Where behaviour must differ, detect that we are in the psa-crypto repo by checking for the 'core' directory. Signed-off-by: David Horstmann <[email protected]>

When detecting the root dir, look both for PSA Crypto and Mbed TLS directories. Signed-off-by: David Horstmann <[email protected]>

Signed-off-by: David Horstmann <[email protected]>

Instead of: ! in_psa_crypto_repo() use in_mbedtls_repo() Signed-off-by: David Horstmann <[email protected]>

Use CMake to build the library out-of-source (rather than make) in tests/scripts/test_psa_compliance.py and add a script argument for the out-of-source build directory. Signed-off-by: David Horstmann <[email protected]>

Signed-off-by: David Horstmann <[email protected]>

(For consistency with all.sh) Signed-off-by: David Horstmann <[email protected]>

davidhorstmann-arm · 2023-08-24T14:24:13Z

Rebased on development.

1 minor conflict in all.sh:

<<<<<<< HEAD
        if ! git diff --quiet "$CONFIG_H"; then
            err_msg "Warning - the configuration file 'include/mbedtls/mbedtls_config.h' has been edited. "
=======
        if ! git diff --quiet $CONFIG_H; then
            err_msg "Warning - the configuration file '$CONFIG_H' has been edited. "
>>>>>>> 6383d1b8f2ce (Make all.sh PSA-crypto-friendly)

Resolved to add double quotes (") around $CONFIG_H and also use $CONFIG_H in the message:

        if ! git diff --quiet "$CONFIG_H"; then
            err_msg "Warning - the configuration file '$CONFIG_H' has been edited. "

scripts/mbedtls_dev/build_tree.py

tests/scripts/all.sh

tests/scripts/test_psa_compliance.py

Check for the 'drivers' and 'programs' directories additionally to the ones that are already there. Signed-off-by: David Horstmann <[email protected]>

Have separate in_mbedtls_repo() and in_psa_crypto_repo() functions Signed-off-by: David Horstmann <[email protected]>

Signed-off-by: David Horstmann <[email protected]>

Use the repo-specific test not just the Mbed TLS specific one. Signed-off-by: David Horstmann <[email protected]>

This makes it more repo-agnostic Signed-off-by: David Horstmann <[email protected]>

Either remove exclusive references to Mbed TLS or accompany them with references to "PSA Crypto". Signed-off-by: David Horstmann <[email protected]>

Signed-off-by: David Horstmann <[email protected]>

ronald-cron-arm

This looks almost good to me. Otherwise mypy is not happy it seems and the CI is failing.

tests/scripts/all.sh

tests/scripts/test_psa_compliance.py

Signed-off-by: David Horstmann <[email protected]>

Use CMake's -t option to build only the crypto target. Parameterize the crypto target to have the right name depending on whether this is Mbed TLS or PSA Crypto. Signed-off-by: David Horstmann <[email protected]>

Signed-off-by: David Horstmann <[email protected]>

This will prevent CMake from mistaking the build directory for the source directory Signed-off-by: David Horstmann <[email protected]>

Signed-off-by: David Horstmann <[email protected]>

This enables compatibility with older versions of CMake that do not have the abbreviated switch. Signed-off-by: David Horstmann <[email protected]>

ronald-cron-arm

This looks good to me, thanks.

paul-elliott-arm

LGTM

davidhorstmann-arm requested a review from ronald-cron-arm July 18, 2023 17:13

davidhorstmann-arm force-pushed the psa-crypto-script-changes branch from fd28e9a to c144d97 Compare July 19, 2023 10:01

daverodgman self-requested a review July 19, 2023 11:58

daverodgman removed their request for review July 19, 2023 11:58

ronald-cron-arm requested changes Jul 20, 2023

View reviewed changes

ronald-cron-arm added enhancement needs-work needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Aug 24, 2023

davidhorstmann-arm added 9 commits August 24, 2023 15:18

Make all.sh PSA-crypto-friendly

9a6c45b

Introduce changes needed to run all.sh in the psa-crypto repo. Where behaviour must differ, detect that we are in the psa-crypto repo by checking for the 'core' directory. Signed-off-by: David Horstmann <[email protected]>

Modify build_tree.py for the PSA Crypto repo

795d8b5

When detecting the root dir, look both for PSA Crypto and Mbed TLS directories. Signed-off-by: David Horstmann <[email protected]>

Modify test_psa_compliance.py for psa-crypto repo

1d09184

Signed-off-by: David Horstmann <[email protected]>

Support psa-crypto repo in psa_storage.py

42f42f4

Signed-off-by: David Horstmann <[email protected]>

Tweak test_psa_compliance pylint annotations

e31014a

Signed-off-by: David Horstmann <[email protected]>

Invert logic for repo detection in all.sh

76a7738

Instead of: ! in_psa_crypto_repo() use in_mbedtls_repo() Signed-off-by: David Horstmann <[email protected]>

Parameterize out of source build directory

4dcddcf

Use CMake to build the library out-of-source (rather than make) in tests/scripts/test_psa_compliance.py and add a script argument for the out-of-source build directory. Signed-off-by: David Horstmann <[email protected]>

Rename psa_crypto_lib_filename to just crypto_lib_filename

7f93d22

Signed-off-by: David Horstmann <[email protected]>

Rename is_psa_crypto -> in_psa_crypto_repo

0ac57ca

(For consistency with all.sh) Signed-off-by: David Horstmann <[email protected]>

davidhorstmann-arm force-pushed the psa-crypto-script-changes branch from ad6e0dd to 0ac57ca Compare August 24, 2023 14:18

davidhorstmann-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed needs-work labels Aug 24, 2023

ronald-cron-arm requested changes Aug 28, 2023

View reviewed changes

davidhorstmann-arm added 3 commits August 29, 2023 09:50

Improve directory coverage in PSA repo detection

2fde999

Check for the 'drivers' and 'programs' directories additionally to the ones that are already there. Signed-off-by: David Horstmann <[email protected]>

Separate directory discernment into 2 functions

d02b5f8

Have separate in_mbedtls_repo() and in_psa_crypto_repo() functions Signed-off-by: David Horstmann <[email protected]>

Use repo detection functions at start of all.sh

58cf7c6

Signed-off-by: David Horstmann <[email protected]>

davidhorstmann-arm added 4 commits August 29, 2023 10:25

Correctly detect presence of the built library

98af198

Use the repo-specific test not just the Mbed TLS specific one. Signed-off-by: David Horstmann <[email protected]>

Rename 'mbedtls_dir' -> 'root_dir'

f757069

This makes it more repo-agnostic Signed-off-by: David Horstmann <[email protected]>

Remove or qualify references to Mbed TLS

3b8984a

Either remove exclusive references to Mbed TLS or accompany them with references to "PSA Crypto". Signed-off-by: David Horstmann <[email protected]>

Disable pylint error in CMake command

2ba89be

Signed-off-by: David Horstmann <[email protected]>

ronald-cron-arm requested changes Aug 29, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

tests/scripts/test_psa_compliance.py Outdated Show resolved Hide resolved

davidhorstmann-arm added 3 commits August 29, 2023 13:46

Tidy up reference to Mbed TLS in help message

c69074d

Signed-off-by: David Horstmann <[email protected]>

Test PSA compliance: Build only the crypto target

beaee26

Use CMake's -t option to build only the crypto target. Parameterize the crypto target to have the right name depending on whether this is Mbed TLS or PSA Crypto. Signed-off-by: David Horstmann <[email protected]>

Appease pylint by renaming variables

b48822c

Signed-off-by: David Horstmann <[email protected]>

davidhorstmann-arm requested a review from paul-elliott-arm August 29, 2023 13:51

davidhorstmann-arm added 5 commits August 29, 2023 14:57

Move -B switch into a single argument

41c316d

This will prevent CMake from mistaking the build directory for the source directory Signed-off-by: David Horstmann <[email protected]>

Fix pylint errors

fd9264e

Signed-off-by: David Horstmann <[email protected]>

Add missing import in test_psa_compliance.py

9cc6b2f

Signed-off-by: David Horstmann <[email protected]>

Disable pylint error for non-uppercase names

3ed1871

Signed-off-by: David Horstmann <[email protected]>

Use '--target' instead of shortened '-t'

8f3ec8e

This enables compatibility with older versions of CMake that do not have the abbreviated switch. Signed-off-by: David Horstmann <[email protected]>

davidhorstmann-arm added priority-very-high Highest priority - prioritise this over other review work and removed priority-high High priority - will be reviewed soon labels Aug 30, 2023

ronald-cron-arm removed needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Aug 30, 2023

ronald-cron-arm approved these changes Aug 31, 2023

View reviewed changes

paul-elliott-arm approved these changes Aug 31, 2023

View reviewed changes

paul-elliott-arm added this pull request to the merge queue Aug 31, 2023

paul-elliott-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 Aug 31, 2023

Merged via the queue into Mbed-TLS:development with commit 6147511 Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous changes to scripts for PSA-Crypto enablement #7955

Miscellaneous changes to scripts for PSA-Crypto enablement #7955

davidhorstmann-arm commented Jul 18, 2023 •

edited by ronald-cron-arm

Loading

daverodgman commented Jul 19, 2023

ronald-cron-arm left a comment

davidhorstmann-arm commented Aug 24, 2023

ronald-cron-arm left a comment •

edited

Loading

ronald-cron-arm left a comment

paul-elliott-arm left a comment

Miscellaneous changes to scripts for PSA-Crypto enablement #7955

Miscellaneous changes to scripts for PSA-Crypto enablement #7955

Conversation

davidhorstmann-arm commented Jul 18, 2023 • edited by ronald-cron-arm Loading

PR checklist

daverodgman commented Jul 19, 2023

ronald-cron-arm left a comment

Choose a reason for hiding this comment

davidhorstmann-arm commented Aug 24, 2023

ronald-cron-arm left a comment • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

paul-elliott-arm left a comment

Choose a reason for hiding this comment

davidhorstmann-arm commented Jul 18, 2023 •

edited by ronald-cron-arm

Loading

ronald-cron-arm left a comment •

edited

Loading