-
Notifications
You must be signed in to change notification settings - Fork 36
Split test_psa_compliance.py #209
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
base: main
Are you sure you want to change the base?
Split test_psa_compliance.py #209
Conversation
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This will allow consuming branches to each have their executable entry point. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
…thms Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
…odgy) Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
When looking for constructors, do complain if we see an unusual macro with a parameter: there's a significant chance that it's something new that will require specific handling. As a consequence, we need to explicitly skip more things that are known not to be constructors. Keep ignoring macros without parameters that don't look like constructors for the types we care about. Those probably don't matter. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
7557dd0
to
ca98305
Compare
Having separate patch files has several benefits: * They're available for integrators who wouldn't use our script to test compliance. * We keep them separate so they're easier for us to keep track of, and apply separately if needed. * No need to cheat with unchanged empty lines (normally represented by a line containing a single space in a patch file) to keep `check_files.py` happy. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
…same name mypy can't deal with two modules with the same basename on its command line. We don't normally want modules with the same name in different directories, to avoid confusion, but it can happen occasionally while moving files across repositories. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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.
Initial feedback for the patch file handling, still need to go through the rest of the PR.
An unchanged blank line results in a line containing a single space in the diff. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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.
Looking pretty good to me! I just have a question that I'd like answered before I can approve - my other point is completely optional.
@@ -59,12 +61,12 @@ def test_compliance(library_build_dir: str, | |||
subprocess.check_call(['git', 'fetch', PSA_ARCH_TESTS_REPO, psa_arch_tests_ref]) | |||
subprocess.check_call(['git', 'checkout', '--force', 'FETCH_HEAD']) | |||
|
|||
if patch: | |||
if patch_files: |
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 don't understand the logic behind running git reset
only if we have a patch to apply. It seems to me that git reset
is needed if we had a patch to apply the last time we ran this script (and we are re-using the repo), which might not be the same as we have one now. Also, I don't think calling git reset
when not strictly needed does any harm. So, I'd be inclined to just call it unconditionally. Am I missing something?
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 wanted to minimize changes in 3.6 which doesn't need any of the new fancy stuff. Also, arguably, it makes things worse for local debugging where you might want to edit the cached psa-arch-test tree until you get it right. On the other hand, I agree with you that I would have made git reset
unconditional if I was doing this from scratch. I'll defer to Bence's preference on that since he know this script's history a lot better than we do.
Split the PSA API compliance script
test_psa_compliance.py
into an engine plus a pre-branch runner, like with many similar scripts. Now each branch has the knowledge of which version of psa-arch-tests it runs against and how to adjust the results if needed.Also do the bare minimum to allow TF-PSA-Crypto to declare SPAKE2+ key types and algorithms in Mbed-TLS/TF-PSA-Crypto#453.
Fixes #158.
PR checklist
Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.