Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Modify check generated files script to work with TF PSA Crypto too #8523
Changes from 12 commits
b4f1ee0
c9f8386
b10cc7a
d3f8443
0bb761c
c1750bb
e58128e
d289b8b
0eb2dc1
4291bc2
5556f90
13ecb69
79cae20
b42c50b
772056c
d35b94b
755d321
d0c3076
8932404
beec452
cdbf2fd
fc60e9b
6130a61
e8f3789
08c6dc4
46588de
56bee03
d1f2934
db80b23
99030e2
04c446c
3a06906
10769bc
f05b768
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
torepo_root
in this file. Then comes up the question of what do we do with: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
andguess_tf_psa_crypto_root
and useguess_repo_root
here. We would then have:Otherwise, please look at the occurrences of
library
in the file and update the ones that have to be updated.There was a problem hiding this comment.
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
vsguess_crypto_root
(currently same asguess_repo_root
). I don't think we needguess_mbedtls_root
as part of the interface. But it can stay for a while as a transitional alias forguess_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
andcrypto_builtins_directory
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In build_tree.py? I am not sure what you mean by that.
There was a problem hiding this comment.
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 ofbuild_tree
.There was a problem hiding this comment.
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 thelibrary
directory in the case of mbedtls (directory where the PSA core code is located in mbedtls) and the path to thecore
directory in the case of TF-PSA-Crypto?There was a problem hiding this comment.
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 renamembedtls_root
torepo_root
otherwise I would have the impression to let things in a bad state but probably ok to keepdef_arg_repo_root = build_tree.guess_mbedtls_root()
for the time being. @tom-daubney-arm @gilles-peskine-arm what do you think?There was a problem hiding this comment.
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
vscore
). Since this detection is not specific togenerate_driver_wrappers.py
, we should add it directly tobuild_tree.py
rather than add it togenerate_driver_wrappers.py
and do extra work to move it later.There was a problem hiding this comment.
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 thelibrary
directory in the case of mbedtls (directory that currently contains the PSA core code in mbedtls) and the path to thecore
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 tocore
as well.There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
.