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 programs/psa to tf-psa-crypto #9717

Open
wants to merge 20 commits into
base: development
Choose a base branch
from

Conversation

Harry-Ramsey
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey commented Oct 22, 2024

Description

This pull request moves program/psa to tf-psa-crypto/programs/psa.
This pull request cloes #9267.

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.

  • changelog not required because: for repo split.
  • development PR provided.
  • framework PR not required.
  • 3.6 PR provided # | not required because: for repo split.
  • 2.28 PR not required because: for repo split.
  • tests provided.

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.

@Harry-Ramsey Harry-Ramsey self-assigned this Oct 22, 2024
@@ -0,0 +1,83 @@
MBEDTLS_TEST_PATH = ../tests
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no make build system in tf-psa-crypto and we do not plan to add one thus we need to just adapt the make files in mbedtls to the new location of the PSA programs. That's how we have done it for test suites, the crypto library ...

@Harry-Ramsey Harry-Ramsey force-pushed the move-programs-psa-tf-psa-crypto-development branch 7 times, most recently from 6893477 to 0d8c743 Compare October 31, 2024 17:18
@Harry-Ramsey
Copy link
Contributor Author

Hopefully now if CI passes this is the correct implementation for enabling programs to be moved to tf-psa-crypto.

@Harry-Ramsey Harry-Ramsey marked this pull request as ready for review October 31, 2024 17:54
@Harry-Ramsey Harry-Ramsey added enhancement component-psa PSA keystore/dispatch layer (storage, drivers, …) size-s Estimated task size: small (~2d) labels Oct 31, 2024
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

This looks quite good to me but there are things I would like to improve, see my comments. Otherwise I haven't checked but it is likely that with the changes in this PR the PSA programs key_ladder_demo and psa_hash are no longer executed as part of run_demos.py. Please check that, thanks.

@@ -8,7 +8,7 @@ if (NOT WIN32)
endif()
add_subdirectory(hash)
add_subdirectory(pkey)
add_subdirectory(psa)
add_subdirectory(../tf-psa-crypto/programs/psa ../tf-psa-crypto/programs/psa)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a comment. Eventually we want to move that to tf-psa-cypto/programs/CMakeLists.txt but it will be easier when #9615 is completed.

programs/Makefile Outdated Show resolved Hide resolved
programs/Makefile Outdated Show resolved Hide resolved
scripts/generate_psa_constants.py Outdated Show resolved Hide resolved
scripts/generate_psa_constants.py Outdated Show resolved Hide resolved
tests/psa-client-server/psasim/src/aut_psa_aead_encrypt.c Outdated Show resolved Hide resolved
tf-psa-crypto/programs/psa/aead_demo.c Outdated Show resolved Hide resolved
tf-psa-crypto/programs/psa/hmac_demo.c Outdated Show resolved Hide resolved
programs/Makefile Outdated Show resolved Hide resolved
programs/Makefile Outdated Show resolved Hide resolved
@Harry-Ramsey Harry-Ramsey force-pushed the move-programs-psa-tf-psa-crypto-development branch 2 times, most recently from 35876cd to 0df7120 Compare November 5, 2024 09:36
@Harry-Ramsey
Copy link
Contributor Author

This should pass CI now and be ready for a proper review since I had to fix a few empty commit issues.

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

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

The last changes look good to me, thanks.

However the run_demos.py issue does not seem to be addressed.
If I do:

make neat
make
./tests/scripts/run_demos.py

the programs key_ladder_demo and psa_hash are not run. They are on development.
Please adapt run_demos.py as necessary.

Other point, please update programs/.gitignore and tf-psa-crypto/programs/.gitignore according to the file moves.

programs/Makefile Outdated Show resolved Hide resolved
programs/Makefile Outdated Show resolved Hide resolved
programs/Makefile Outdated Show resolved Hide resolved
@Harry-Ramsey Harry-Ramsey force-pushed the move-programs-psa-tf-psa-crypto-development branch 2 times, most recently from 72f1ba7 to d461efb Compare November 18, 2024 11:50
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-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

tf-psa-crypto/.gitignore Outdated Show resolved Hide resolved
tf-psa-crypto/programs/demo_common.sh Outdated Show resolved Hide resolved
tests/scripts/run_demos.py Outdated Show resolved Hide resolved
@ronald-cron-arm
Copy link
Contributor

ronald-cron-arm commented Nov 20, 2024

@Harry-Ramsey sorry about that but it needs to be rebased: two conflicts.

This commit moves psa programs from the programs/psa directory to
tf-psa-crypto/programs/psa directory.

Signed-off-by: Harry Ramsey <[email protected]>
This commit fixes paths for programs/psa to tf-psa-crypto/programs/psa.

Signed-off-by: Harry Ramsey <[email protected]>
This commit adjusts the paths of programs/psa to tf-psa-crypto/programs.

Signed-off-by: Harry Ramsey <[email protected]>
This commit adjusts the paths of programs/psa to tf-psa-crypto/programs
in CMakeLists.txt.

Signed-off-by: Harry Ramsey <[email protected]>
This commit updates the paths in generate_visualc_files.pl to update the
generated .sln file with the new paths for psa programs.

Signed-off-by: Harry Ramsey <[email protected]>
This commit refactors the Makefile in the programs directory to remove
unused variables and consistent naming schemes.

Signed-off-by: Harry Ramsey <[email protected]>
This commit moves generate_psa_constants.py to tf-psa-crypto and updates
the paths inside the script necessary for that move.

Signed-off-by: Harry Ramsey <[email protected]>
This commit refactors comments refering to tf-psa-crypto for the
correct path upon repo split.

Signed-off-by: Harry Ramsey <[email protected]>
This commit uses static paths in the makefile to create programs since
the script generate_visualc_files.pl cannot substitute variable paths.

Signed-off-by: Harry Ramsey <[email protected]>
This commit fixes incorrect paths to generate_psa_constants.py after
being moves to tf-psa-crypto.

Signed-off-by: Harry Ramsey <[email protected]>
This commit updates the path to the generate_psa_constants.py script in
programs/psa/CMakeList.txt file.

Signed-off-by: Harry Ramsey <[email protected]>
This commit updates the path to generate_psa_constants.py in
check-generated-files.sh

Signed-off-by: Harry Ramsey <[email protected]>
This commit fixes make_generated_files.bat as it requires the user to be
inside the tf-psa-directory to invoke generate_psa_constants.py.

Signed-off-by: Harry Ramsey <[email protected]>
Update gitignore in programs and tf-psa-crypto following the move of
multiple files.

Signed-off-by: Harry Ramsey <[email protected]>
This commit updates the path to programs in run_demos.py to
tf-psa-crypto/programs.

Signed-off-by: Harry Ramsey <[email protected]>
This commit updates a recipe path in programs/Makefile.

Signed-off-by: Harry Ramsey <[email protected]>
This commit reverts tf-psa-crypto program CC output informing the user
what file is being compiled.

Signed-off-by: Harry Ramsey <[email protected]>
This commit moves programs from tf-psa-crypto/.gitignore to
tf-psa-crypto/programs/.gitignore.

Signed-off-by: Harry Ramsey <[email protected]>
This commit removes the duplicate demo_common.sh file in tf-psa-crypto
and instead use the demo_common.sh file in mbedtls/programs.

Signed-off-by: Harry Ramsey <[email protected]>
This commit concatenates the running of program demos between Mbed TLS
and TF-PSA-Crypto.

Signed-off-by: Harry Ramsey <[email protected]>
@Harry-Ramsey Harry-Ramsey force-pushed the move-programs-psa-tf-psa-crypto-development branch from ce7b71c to 7fa6cae Compare November 20, 2024 16:22
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. This looks good to me but an issue in check_generated_files reported by the CI (due to the rebase?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-psa PSA keystore/dispatch layer (storage, drivers, …) enhancement needs-review Every commit must be reviewed by at least two team members, size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants