Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 34 commits into from
Dec 11, 2023
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
b4f1ee0
Remove superfluous leading whitespace
tom-daubney-arm Nov 13, 2023
c9f8386
Modify check-generated-files.sh to work in both repos
tom-daubney-arm Nov 13, 2023
b10cc7a
Modify generate_driver_wrappers.py to work in both repos
tom-daubney-arm Nov 14, 2023
d3f8443
Further modify check-generated-files.sh
tom-daubney-arm Nov 14, 2023
0bb761c
Remove further extraneous whitespace in lcov script
tom-daubney-arm Nov 14, 2023
c1750bb
Apply correct license to generate_driver_wrappers.py
tom-daubney-arm Nov 14, 2023
e58128e
Refactor repository detection
tom-daubney-arm Nov 14, 2023
d289b8b
Stylise TF-PSA-Crypto correctly
tom-daubney-arm Nov 14, 2023
0eb2dc1
Call the right function
tom-daubney-arm Nov 14, 2023
4291bc2
Remove trailing whitespace
tom-daubney-arm Nov 14, 2023
5556f90
Rename variables in script
tom-daubney-arm Nov 15, 2023
13ecb69
Introduce function to return library/core directory
tom-daubney-arm Nov 16, 2023
79cae20
Remove useless line
tom-daubney-arm Nov 22, 2023
b42c50b
Make use of new crypto_core_directory function
tom-daubney-arm Nov 22, 2023
772056c
Replace repo_root with project_root
tom-daubney-arm Nov 22, 2023
d35b94b
Improve implementation of crypto_core_directory
tom-daubney-arm Nov 22, 2023
755d321
Rename guess_mbedtls_root to guess_project_root
tom-daubney-arm Nov 22, 2023
d0c3076
Make use of crypto_core_directory function in script
tom-daubney-arm Nov 23, 2023
8932404
Introduce project_crypto_name in build_tree.py
tom-daubney-arm Nov 23, 2023
beec452
Use os.path.join in crypto_core_directory
tom-daubney-arm Nov 24, 2023
cdbf2fd
Add documentation for new public functions
tom-daubney-arm Nov 24, 2023
fc60e9b
Make function calls consistent
tom-daubney-arm Nov 24, 2023
6130a61
Remove unused variable
tom-daubney-arm Nov 24, 2023
e8f3789
Revert change that removed in_tf_psa_crypto_repo variable
tom-daubney-arm Nov 24, 2023
08c6dc4
Rename project_crypto_name
tom-daubney-arm Nov 30, 2023
46588de
Improve documentation of crypto_core_directory
tom-daubney-arm Nov 30, 2023
56bee03
Rename variable for better clarity
tom-daubney-arm Nov 30, 2023
d1f2934
Introduce guess_mbedtls_root
tom-daubney-arm Nov 30, 2023
db80b23
Introduce guess_tf_psa_crypto_root
tom-daubney-arm Nov 30, 2023
99030e2
Remove trailing whitespace
tom-daubney-arm Dec 1, 2023
04c446c
Modify crypto_core_directory to also return a relative path
tom-daubney-arm Dec 1, 2023
3a06906
Use guess_mbedtls_root in Mbed-TLS-only script
tom-daubney-arm Dec 1, 2023
10769bc
Fix bad whitespace in keyword argument assignment
tom-daubney-arm Dec 1, 2023
f05b768
Use existing variable containing full path
tom-daubney-arm Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion scripts/generate_driver_wrappers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,15 @@ def main() -> int:

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

library_dir = ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

gilles-peskine-arm marked this conversation as resolved.
Show resolved Hide resolved
if build_tree.looks_like_mbedtls_root(mbedtls_root):
library_dir = 'library'
elif build_tree.looks_like_tf_psa_crypto_root(mbedtls_root):
library_dir = 'core'

output_directory = args.output_directory if args.output_directory is not None else \
os.path.join(mbedtls_root, 'library')
os.path.join(mbedtls_root, library_dir)

template_directory = args.template_dir if args.template_dir is not None else \
os.path.join(mbedtls_root,
'scripts',
Expand Down
4 changes: 2 additions & 2 deletions scripts/lcov.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ set -eu

# Repository detection
in_mbedtls_build_dir () {
test -d library
}
test -d library
}

# Collect stats and build a HTML report.
lcov_library_report () {
Expand Down
39 changes: 28 additions & 11 deletions tests/scripts/check-generated-files.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,20 @@ EOF
exit
fi

if [ -d library -a -d include -a -d tests ]; then :; else
echo "Must be run from Mbed TLS root" >&2
in_mbedtls_repo () {
test -d include -a -d library -a -d programs -a -d tests
}

in_tf_psa_crypto_repo () {
test -d include -a -d core -a -d drivers -a -d programs -a -d tests
}

if in_mbedtls_repo; then
library_dir='library'
elif in_tf_psa_crypto_repo; then
library_dir='core'
else
echo "Must be run from Mbed TLS root or TF-PSA-Crypto root" >&2
exit 1
fi

Expand Down Expand Up @@ -114,16 +126,21 @@ check()
# - **/CMakeLists.txt (to (re)build them with cmake)
# - scripts/make_generated_files.bat (to generate them under Windows)

check scripts/generate_errors.pl library/error.c
check scripts/generate_query_config.pl programs/test/query_config.c
check scripts/generate_driver_wrappers.py library/psa_crypto_driver_wrappers.h library/psa_crypto_driver_wrappers_no_static.c
check scripts/generate_features.pl library/version_features.c
check scripts/generate_ssl_debug_helpers.py library/ssl_debug_helpers_generated.c
# generate_visualc_files enumerates source files (library/*.c). It doesn't
# care about their content, but the files must exist. So it must run after
# the step that creates or updates these files.
check scripts/generate_visualc_files.pl visualc/VS2013
# These checks are common to Mbed TLS and TF-PSA-Crypto
check scripts/generate_psa_constants.py programs/psa/psa_constant_names_generated.c
check tests/scripts/generate_bignum_tests.py $(tests/scripts/generate_bignum_tests.py --list)
check tests/scripts/generate_ecp_tests.py $(tests/scripts/generate_ecp_tests.py --list)
check tests/scripts/generate_psa_tests.py $(tests/scripts/generate_psa_tests.py --list)
check scripts/generate_driver_wrappers.py $library_dir/psa_crypto_driver_wrappers.h $library_dir/psa_crypto_driver_wrappers_no_static.c

# Additional checks for Mbed TLS only
if in_mbedtls_repo; then
check scripts/generate_errors.pl library/error.c
check scripts/generate_query_config.pl programs/test/query_config.c
check scripts/generate_features.pl library/version_features.c
check scripts/generate_ssl_debug_helpers.py library/ssl_debug_helpers_generated.c
# generate_visualc_files enumerates source files (library/*.c). It doesn't
# care about their content, but the files must exist. So it must run after
# the step that creates or updates these files.
check scripts/generate_visualc_files.pl visualc/VS2013
fi