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

Replace MBEDTLS_MD_CAN_SHA256 with PSA_WANT_ALG_SHA_256 #31

Closed

Conversation

eleuzi01
Copy link
Contributor

Issue #9112 in mbedtls

Part of #9173

@eleuzi01 eleuzi01 added enhancement New feature or request 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 priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Jun 26, 2024
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

@tom-daubney-arm tom-daubney-arm self-requested a review July 3, 2024 09:07
@tom-daubney-arm tom-daubney-arm removed the needs-reviewer This PR needs someone to pick it up for review label Jul 3, 2024
tom-daubney-arm
tom-daubney-arm previously approved these changes Jul 3, 2024
Copy link
Contributor

@tom-daubney-arm tom-daubney-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!

@tom-daubney-arm tom-daubney-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 3, 2024
@@ -37,7 +37,7 @@ class TestData:
Take in test_suite_pkcs7.data file.
Allow for new tests to be added.
"""
mandatory_dep = "MBEDTLS_MD_CAN_SHA256"
mandatory_dep = "PSA_WANT_ALG_SHA_256"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks right for 4.0, but doesn't it break 3.6? In the framework, we'll need to have dependencies that are conditional on the version.

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 blocking this until we have a confirmation that it doesn't break 3.6 (or 4.0 for that matter).

@@ -37,7 +38,13 @@ class TestData:
Take in test_suite_pkcs7.data file.
Allow for new tests to be added.
"""
mandatory_dep = "PSA_WANT_ALG_SHA_256"

#temporary solution to determine correct dependency macrosbetween 3.6 and 4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space between macros and between

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise please reference the issue #51 and probably better to rebase your two commits on top of main.

Signed-off-by: Elena Uziunaite <elena.uziunaite@arm.com>
Temporary solution to be resolved in Mbed-TLS#51.

Signed-off-by: Elena Uziunaite <elena.uziunaite@arm.com>
@eleuzi01 eleuzi01 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 needs-ci Needs to pass CI tests labels Sep 25, 2024
@@ -37,7 +38,14 @@ class TestData:
Take in test_suite_pkcs7.data file.
Allow for new tests to be added.
"""
mandatory_dep = "MBEDTLS_MD_CAN_SHA256"

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

nit, unnecessary blank line

@eleuzi01 eleuzi01 closed this Sep 26, 2024
@eleuzi01
Copy link
Contributor Author

Moved changes to PR #34, therefore closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 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 priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
Development

Successfully merging this pull request may close these issues.

6 participants