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.

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

Move some scripts to framework submodule #5

Closed
wants to merge 580 commits into from

Conversation

davidhorstmann-arm
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm commented Mar 26, 2024

Move generate_test_code.py, test_generate_test_code.py and code_style.py to the framework repository.

Synchronized with: Mbed-TLS/mbedtls#8991

gilles-peskine-arm and others added 30 commits December 22, 2022 16:36
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Bignum: Implement fixed width modular multiplication
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
…or-bn-asm

Check for Uncrustify errors in `code_style.py`
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
No intended behavior change.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Test accordingly.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
code_style.py: Support restyling only the specified files
Rename the function to 'fix_quasi_reduction' to better suite its functionality.
Also changed the name prefix to suite for the new module.

Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
Since code style is now enforced, the notice is wrong. Remove it to
avoid confusion.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
Whilst it is true that "silence is golden", no output at all could be
disconcerting and it makes searching in a CI log more difficult.

Add a simple status message that says "Checked N files, style ok".

Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
Instead of capturing the output of diff and printing it, let diff do its
own outputting and se the return code to decide what to do.

This also means that the conversion of stdout to UTF-8 is not necessary,
as the reason it was needed was for printing diffs of files with UTF-8
characters in them.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
This is no longer needed as we only print ASCII text directly

Signed-off-by: David Horstmann <david.horstmann@arm.com>
This prevents a return type error in a later function that uses the
dictionaries here properly typed.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: David Horstmann <david.horstmann@arm.com>
Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
…o_metadata-20221215

Add metadata tests for CCM* and TLS1.2-ECJPAKE-to-PMS
@davidhorstmann-arm davidhorstmann-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Mar 26, 2024
@davidhorstmann-arm davidhorstmann-arm added size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Mar 26, 2024
@davidhorstmann-arm davidhorstmann-arm changed the title Move trivial and uncontroversial scripts to framework submodule Move uncontroversial scripts to framework submodule Mar 26, 2024
@davidhorstmann-arm davidhorstmann-arm force-pushed the dev/davidhorstmann-arm/move-scripts branch from 76f8277 to 010053a Compare March 27, 2024 14:53
@davidhorstmann-arm
Copy link
Contributor Author

[Rebased to preserve git history of the files from the previous repo. The end result is the same]

@davidhorstmann-arm davidhorstmann-arm removed the size-s Estimated task size: small (~2d) label Mar 27, 2024
Copy link
Contributor

@Ryan-Everett-arm Ryan-Everett-arm left a comment

Choose a reason for hiding this comment

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

Reproduced locally with the same result, LGTM!

The module has been renamed to 'framework_dev'.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
This import is now unnecessary, as the script is in the same directory
as the module it imports in the framework submodule.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
Copy link
Contributor

@Ryan-Everett-arm Ryan-Everett-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@davidhorstmann-arm davidhorstmann-arm removed the needs-reviewer This PR needs someone to pick it up for review label Apr 8, 2024
@@ -0,0 +1,3 @@
# This file needs to exist to make framework_dev a package.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think framework_dev is right as a package name. Unless that's intended to be a sub-package of some mbedtls package? The idea with calling the directory mbedtls_dev is that you can use it in a program that isn't specific to Mbed TLS, for example a program that puts multiple crypto libraries together and uses some code from mbedtls_dev to obtain information about things like supported crypto mechanisms. So the name should keep mbedtls in it.

The current name is mbedtls_dev because it's for development of or related to Mbed TLS, as opposed to mbedtls which should be reserved for a Python package that contains wrappers around Mbed TLS's C APIs. We could keep that name here. Or maybe change it to mbedtls_framework?

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 am happy with mbedtls_dev but I was assuming we'd want to generalise it given that it'll be used in TF-PSA-Crypto. I'll change this back to mbedtls_dev.

@@ -0,0 +1,131 @@
"""Helper functions to parse C code in heavily constrained scenarios.
Copy link
Contributor

Choose a reason for hiding this comment

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

The code in c_parsing_helper.py, c_wrapper_generator.py and generate_psa_wrappers.py has not been reviewed. (We rushed these files in via the restricted repository as part of a large security fix.) We need to review them, and most of the code needs to be reviewed in the next few weeks for the client-server testing project.

Moving the code to a new repository would be a good opportunity to review that code since it'll appear as new in the GitHub interface. For the review and rework to be manageable, these files should be in their own pull request.

Cc @minosgalanakis

@@ -0,0 +1,1277 @@
#!/usr/bin/env python3
Copy link
Contributor

Choose a reason for hiding this comment

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

The first commit in the history is "Rename generate_code.py -> generate_test_code.py". It's a bit of an annoying edge case, but it would be nice to get the history from before the renaming too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be non-trivial, but there does seem to be a tool that may be able to do it. I'll give it a try.

@davidhorstmann-arm davidhorstmann-arm changed the title Move uncontroversial scripts to framework submodule Move ~uncontroversial~ scripts to framework submodule Apr 24, 2024
@davidhorstmann-arm davidhorstmann-arm changed the title Move ~uncontroversial~ scripts to framework submodule Move some scripts to framework submodule Apr 24, 2024
@ronald-cron-arm
Copy link
Contributor

It should be beneficial to first move only the files in mbedtls_dev. I have created an issue for that: #13. @davidhorstmann-arm, please have a look.

@ronald-cron-arm ronald-cron-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels May 2, 2024
@ronald-cron-arm
Copy link
Contributor

Closing as superseded by more separate PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-work priority-high High priority - will be reviewed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet