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

Add various configuration components #151

Merged

Conversation

Harry-Ramsey
Copy link
Contributor

@Harry-Ramsey Harry-Ramsey commented Jan 13, 2025

This commit adds various configuration components to TF-PSA-Crypto. These components have been adapted from Mbed TLS to use CMake. Closes #125.

This pull request has a dependency on: #164

PR checklist

  • changelog not required because: testing enhancement.
  • framework PR not required.
  • mbedtls PR not required.
  • tests provided.

@Harry-Ramsey Harry-Ramsey self-assigned this Jan 13, 2025
@Harry-Ramsey Harry-Ramsey force-pushed the components-configuration branch 4 times, most recently from 7ebfae7 to 1bd7e54 Compare January 16, 2025 15:26
@Harry-Ramsey Harry-Ramsey added the needs-preceding-pr Requires another PR to be merged first label Jan 21, 2025
@Harry-Ramsey Harry-Ramsey force-pushed the components-configuration branch from 1bd7e54 to 815aac7 Compare January 21, 2025 09:33
@Harry-Ramsey Harry-Ramsey added the needs-ci Needs to pass CI tests label Jan 21, 2025
@Harry-Ramsey Harry-Ramsey force-pushed the components-configuration branch from 815aac7 to c9df6f6 Compare January 27, 2025 12:44
@Harry-Ramsey Harry-Ramsey force-pushed the components-configuration branch from c9df6f6 to 4666429 Compare February 4, 2025 14:26
@Harry-Ramsey Harry-Ramsey force-pushed the components-configuration branch 5 times, most recently from 89d9656 to 795636d Compare February 19, 2025 16:20
@Harry-Ramsey Harry-Ramsey removed needs-preceding-pr Requires another PR to be merged first needs-ci Needs to pass CI tests labels Feb 20, 2025
@Harry-Ramsey Harry-Ramsey force-pushed the components-configuration branch 3 times, most recently from 776dd16 to 1c7c66a Compare February 20, 2025 15:38
@ronald-cron-arm ronald-cron-arm added enhancement New feature or request needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Feb 21, 2025
@valeriosetti valeriosetti self-requested a review February 21, 2025 10:34
################################################################

component_tf_psa_crypto_test_default_out_of_box () {
msg "build: make, default config (out-of-box)" # ~1min
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be:

Suggested change
msg "build: make, default config (out-of-box)" # ~1min
msg "build: cmake, default config (out-of-box)" # ~1min

but actually I have a question about this: specifying cmake made sense in the main repo because there you can either build with make or cmake, but in tf-psa-crypto there's only 1 option and that's cmake. Can't we just fix all the message strings in this file by removing cmake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that is a better solution since it is just not necessary in a single build system project.

Comment on lines 96 to 98
# TF-M configuration needs a TF-M platform. A tweaked version of
# the configuration that works on mainstream platforms is in
# configs/config-tfm.h, tested via test-ref-configs.pl.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the last part of this sentence should be removed:

Suggested change
# TF-M configuration needs a TF-M platform. A tweaked version of
# the configuration that works on mainstream platforms is in
# configs/config-tfm.h, tested via test-ref-configs.pl.
# TF-M configuration needs a TF-M platform.

because it looks like a leftover from when this test component is executed from the main repo. In the tf-psa-crypto there's no configs/config-tfm.h as far as I can see.

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.

I only have some minor comments, but the PR looks almost OK to me.

@Harry-Ramsey Harry-Ramsey force-pushed the components-configuration branch from 1c7c66a to 055e3c5 Compare February 21, 2025 13:17
valeriosetti
valeriosetti previously approved these changes Feb 21, 2025
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.

LGTM, thanks!

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 good to me, just spotted some remaining references to "make" in logs.

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.

LGTM, thanks.

@ronald-cron-arm ronald-cron-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-reviewer This PR needs someone to pick it up for review labels Feb 21, 2025
@Harry-Ramsey Harry-Ramsey force-pushed the components-configuration branch from 784627d to aaf836a Compare February 24, 2025 08:35
Restore test crypto configuration files in the state they were in Mbed
TLS just before the split as in the merge of PR 9809, commit

Signed-off-by: Harry Ramsey <[email protected]>
This commit updates the documentation for user-config-for-test.h due to
it being moved to TF-PSA-Crypto.

Signed-off-by: Harry Ramsey <[email protected]>
This commit adds various configuration components to TF-PSA-Crypto.
These components have been adapted from Mbed TLS to use CMake.

Signed-off-by: Harry Ramsey <[email protected]>
This commit adds a new cmake flag to the build_tfm component in
components-configuration.sh.

The purpose of the flag is to disable CMake compiler tests since it will
always try to build an executable which will always fail on linking due
to architecture mismatch.

Signed-off-by: Harry Ramsey <[email protected]>
This commit removes multiple unnecessary C flags from
components-configuration and replaces some with CMake build types.

Signed-off-by: Harry Ramsey <[email protected]>
This commit adds a new component test_default_no_deprecated to
components-configuration.sh.

Signed-off-by: Harry Ramsey <[email protected]>
This commit removes references to cmake in the echo messages since
TF-PSA-Crypto has only a CMake build system.

Signed-off-by: Harry Ramsey <[email protected]>
@ronald-cron-arm ronald-cron-arm force-pushed the components-configuration branch from aaf836a to 7a8ff01 Compare March 5, 2025 07:47
@ronald-cron-arm ronald-cron-arm added this pull request to the merge queue Mar 5, 2025
Merged via the queue into Mbed-TLS:development with commit 20054a1 Mar 5, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports enhancement New feature or request priority-high High priority - will be reviewed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add components-configurations.sh
3 participants