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

Jump to bottom

[PSA Client Testing] Refactored code wrapping code #9224

Closed
wants to merge 2 commits into from
Closed

[PSA Client Testing] Refactored code wrapping code #9224

wants to merge 2 commits into from

Conversation

minosgalanakis
Copy link
Contributor

@minosgalanakis minosgalanakis commented Jun 3, 2024 •
edited by gilles-peskine-arm
Loading

Description

This pr introduces a PSAWrapperclass which abstracts the generic code-wrapper code and makes PSATestWrapper inherit from it.

It also migrates all the relevant code in its own sub-package since there will be many more relevant modules planned for dynamic c code generation.

Resolves #8961
Depends on Mbed-TLS/mbedtls-framework#25.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog not required
  • 3.6 backport TODO
  • 2.28 backport not required. New feature
  • tests not required. code generating scripts have been verified manually.

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.

@minosgalanakis minosgalanakis added component-platform Portability layer and build scripts size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jun 3, 2024
tom-cosgrove-arm
tom-cosgrove-arm reviewed Jun 3, 2024
View reviewed changes
tests/scripts/code_wrapper/psa_test_wrapper.py Outdated
stream: str,
in_headers: Collection[str] = DEFAULTS["input_headers"]) -> None:
super().__init__(output_h_f, output_c_f, in_headers)# type: ignore[arg-type]
self.set_stream(stream)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at end of file (that's pretty much the limit of my Python 😊)

@gilles-peskine-arm
Copy link
Contributor

I can review this but please resolve the conflicts first.

@minosgalanakis minosgalanakis force-pushed the minos/feature/crypto_client_testing_wrapper branch 2 times, most recently from 21b8880 to c7d55f8 Compare June 4, 2024 13:59
@minosgalanakis minosgalanakis 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 and removed needs-work labels Jun 4, 2024
@gilles-peskine-arm gilles-peskine-arm self-requested a review June 5, 2024 16:48
@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jun 5, 2024
gilles-peskine-arm
gilles-peskine-arm requested changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

I'm sorry but this approach of duplicating files is not practical. Since #9200, generate_psa_wrappers.py is in the framework repository, and needs to be edited there.

tests/scripts/generate_psa_wrappers_future.py Outdated
# Copyright The Mbed TLS Contributors
# SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later

### NOTE. This is a future version of the file located at
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't duplicate code between the two repositories. It might work for work on the simulator, but it blocks all other work that would use the PSA wrappers, and making the PSA wrappers available to other work is the whole point of separating it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any current work that needs to modify the PSA wrappers? If not, this approach should be okay, with a piece at the end of the quarter to reconcile the code in the repositories. Otherwise the work on the framework repo is going to slow this down significantly

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 has been addressed in PR #25

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 5, 2024
@minosgalanakis
Copy link
Contributor Author

Related framework PR #25

@minosgalanakis minosgalanakis added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 10, 2024
@minosgalanakis minosgalanakis force-pushed the minos/feature/crypto_client_testing_wrapper branch from 4828520 to d8071b2 Compare June 11, 2024 13:18
@davidhorstmann-arm davidhorstmann-arm mentioned this pull request Jun 11, 2024
Closed
4 tasks
valeriosetti
valeriosetti previously approved these changes Jun 12, 2024
View reviewed changes
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. I just edited the description stating that it depends on Mbed-TLS/mbedtls-framework#25, so we should merge that one first, then come back on this and merge it. I will also add the proper label

@valeriosetti valeriosetti added the needs-preceding-pr Requires another PR to be merged first label Jun 12, 2024
@minosgalanakis minosgalanakis force-pushed the minos/feature/crypto_client_testing_wrapper branch 2 times, most recently from 9425050 to 473245f Compare June 13, 2024 10:53
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-preceding-pr Requires another PR to be merged first needs-reviewer This PR needs someone to pick it up for review labels Jun 26, 2024
@minosgalanakis minosgalanakis force-pushed the minos/feature/crypto_client_testing_wrapper branch 2 times, most recently from f70b715 to 102918d Compare July 5, 2024 08:51
@gilles-peskine-arm
Copy link
Contributor

Since there is now a new subdirectory under scripts/mbedtls_framework, we need to make tests/scripts/check-python-files.sh look at it when invoking Pylint.

bump

@minosgalanakis minosgalanakis force-pushed the minos/feature/crypto_client_testing_wrapper branch from 102918d to 3b40ce3 Compare July 15, 2024 13:53
@gilles-peskine-arm
Copy link
Contributor

As of 3b40ce3 there is still a conflict in the framework, and the CI is failing. Please rebase and fix the content of the framework to pass the CI.

@minosgalanakis minosgalanakis force-pushed the minos/feature/crypto_client_testing_wrapper branch 5 times, most recently from f7c2bcb to 39350a6 Compare July 22, 2024 15:07
gilles-peskine-arm
gilles-peskine-arm previously approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-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

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. and removed needs-work labels Jul 22, 2024
@gilles-peskine-arm
Copy link
Contributor

We'll need a 3.6 backport to update the generated files.

@gilles-peskine-arm
Copy link
Contributor

The CI is failing in make generated_files and this seems unrelated to this PR. It looks like something changed in the framework. Can you please try rebasing on top of the latest development and see if things work?

@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 22, 2024
@minosgalanakis minosgalanakis force-pushed the minos/feature/crypto_client_testing_wrapper branch from 39350a6 to eae4ce7 Compare July 23, 2024 15:12
@minosgalanakis
Copy link
Contributor Author

This pr is no longer required as the framework pointer has been updated as of #9394

@gilles-peskine-arm
Copy link
Contributor

This pull request was still required because it was supposed to extend check-python-files.sh to cover the files in the code_wrapper subdirectory. Now pylint is complaining quite a bit about those files. I'll take care of it, I've already fixed most of the problems in my working copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm left review comments

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@valeriosetti valeriosetti valeriosetti left review comments

@davidhorstmann-arm davidhorstmann-arm Awaiting requested review from davidhorstmann-arm

Assignees
No one assigned
Labels
component-platform Portability layer and build scripts needs-backports Backports are missing or are pending review and approval. needs-work priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
No open projects
Roadmap Board for Mbed TLS
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Python libraries to generate wrapper code for PSA functions
4 participants
@minosgalanakis @gilles-peskine-arm @tom-cosgrove-arm @valeriosetti