scripts: Add dispatch include path and new generation location#307
scripts: Add dispatch include path and new generation location#307asmellby wants to merge 5 commits into
Conversation
PSA crypto driver dispatch files are generated into dispatch/ instead of core/ in newer TF-PSA-Crypto versions. API headers that rely on driver/dispatch implementation details are also moved from include/ to dispatch/include/. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>
gilles-peskine-arm
added
enhancement
github-project-automation
Bot
moved this to In Development
in Roadmap pull requests (new board)
|
(Priority note: this fits under our “PSA crypto driver standardization” work, for which we have allocated a little bandwidth this quarter. Thanks Aksel for contributing it!) |
gilles-peskine-arm
removed
the
needs-reviewer
gilles-peskine-arm
left a comment
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
I don't think tf_psa_crypto_version() is necessary. But if it is, it needs to pass the static checks. Otherwise looks good to me.
|
|
||
| if build_tree.looks_like_tf_psa_crypto_root("."): | ||
| new_dispatch_layout = (Path.cwd() / "dispatch" / "CMakeLists.txt").exists() | ||
| if build_tree.tf_psa_crypto_version() >= (1,1,0) and new_dispatch_layout: |
There was a problem hiding this comment.
What's the point of checking the version? I'd prefer to avoid relying on any version number.
There was a problem hiding this comment.
I did it to limit when the new layout was considered to releases that are "supposed to" have the new layout. But unconditionally allowing the new layout does mean we avoid the version check. Updated to remove the version check.
| else: | ||
| parser.add_argument('--include', '-I', | ||
| action='append', default=['tf-psa-crypto/include', | ||
| 'tf-psa-crypto/dispatch/include', |
There was a problem hiding this comment.
There's still a reference to core/psa_crypto_driver_wrappers.h in scripts/check_names.py, and even a reference to library/psa_crypto_driver_wrappers.h which only exists in the 3.6 branch. It was introduced in 634e0d2 and I think it's now obsolete (especially since check_names.py passes just fine with the generated files in dispatch), but I'm not completely sure. If it's obsolete, we might as well remove it now, although it would be strictly speaking out of scope.
There was a problem hiding this comment.
Good catch. Updated to include the new dispatch include path (which was missing) and generalizing the excluded autogen files. I agree that it looks like the script passes even if psa_crypto_driver_wrappers.h isn't excluded, so if you want me to, I can remove the exclusion instead. I don't know if the lack of "bad" symbols in the generated file is a long-term commitment or just how things are right now.
There was a problem hiding this comment.
The problem isn't with symbols that violate our naming conventions, but about symbols that look like a typo because check_names finds their use but not their definition or vice versa.
Since 634e0d2, we've both fixed a few bugs in check_names.py and changed the structure of the driver wrapper code. Today check_names.py passes on 3.6 if I remove all references to psa_crypto_driver_wrappers.h (all of them in exclude lists) (69fa1da). So please go ahead and do that.
gilles-peskine-arm
added
needs-work
and removed
needs-review
Assume the new dispatch location is possible for any TF-PSA-Crypto version to avoid needing to perform a version check. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>
Add the dispatch include directory to the list of public paths to check. Add the generated driver wrapper path to the list of excluded patterns. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>
gilles-peskine-arm
added
needs-review
gilles-peskine-arm
left a comment
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
LGTM, but let's not add to the complexity in check_names, please remove the psa_crypto_driver_wrappers.h exclusion.
| else: | ||
| parser.add_argument('--include', '-I', | ||
| action='append', default=['tf-psa-crypto/include', | ||
| 'tf-psa-crypto/dispatch/include', |
There was a problem hiding this comment.
The problem isn't with symbols that violate our naming conventions, but about symbols that look like a typo because check_names finds their use but not their definition or vice versa.
Since 634e0d2, we've both fixed a few bugs in check_names.py and changed the structure of the driver wrapper code. Today check_names.py passes on 3.6 if I remove all references to psa_crypto_driver_wrappers.h (all of them in exclude lists) (69fa1da). So please go ahead and do that.
The check_names.py script excluded the driver wrapper from being included in API checks. The limitations that led to that being necessary are no longer present, so remove the exclude for all possible driver wrapper locations across Mbed TLS and TF-PSA-Crypto. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>
Check for stray Doxygen commands in the dispatch include directory. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>
2 participants
Description
PSA crypto driver dispatch files are generated into
dispatch/instead ofcore/in newer TF-PSA-Crypto versions. API headers that rely on driver and dispatch implementation details are also moved frominclude/todispatch/include/.Update the framework scripts to work with both the old and new layouts.
PR checklist