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

Fix compiler warnings in test_suite_pk.function #9250

Merged
merged 4 commits into from
Jun 24, 2024

Conversation

eleuzi01
Copy link
Contributor

@eleuzi01 eleuzi01 commented Jun 13, 2024

Description

A fix for compiler warnings in test_suite_pk.function when SECP192R1 or/and SECP192K1 are disabled, see #9029

PR checklist

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

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.

@eleuzi01 eleuzi01 added bug component-crypto Crypto primitives and low-level interfaces size-xs Estimated task size: extra small (a few hours at most) labels Jun 13, 2024
@eleuzi01 eleuzi01 self-assigned this Jun 13, 2024
Signed-off-by: Elena Uziunaite <[email protected]>
@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 labels Jun 14, 2024
@sezrab sezrab self-requested a review June 17, 2024 13:54
Copy link
Contributor

@sezrab sezrab left a comment

Choose a reason for hiding this comment

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

Looks good. Should it have a change-log since these are user-facing changes?

minosgalanakis
minosgalanakis previously approved these changes Jun 17, 2024
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@minosgalanakis
Copy link
Contributor

Looks good. Should it have a change-log since these are user-facing changes?

We do not normally do changelog entries for test code.

We should have 3.6 backports though.

sezrab
sezrab previously approved these changes Jun 17, 2024
@gilles-peskine-arm
Copy link
Contributor

We don't write changelog entries for test improvements, but this is a build failure, and it's user-reported, so I think we should have a changelog entry.

@gilles-peskine-arm gilles-peskine-arm added needs-backports Backports are missing or are pending review and approval. and removed needs-reviewer This PR needs someone to pick it up for review labels Jun 17, 2024
Signed-off-by: Elena Uziunaite <[email protected]>
@eleuzi01 eleuzi01 dismissed stale reviews from sezrab and minosgalanakis via 4d9fb8b June 17, 2024 16:15
@eleuzi01
Copy link
Contributor Author

We don't write changelog entries for test improvements, but this is a build failure, and it's user-reported, so I think we should have a changelog entry.

Done, let me know if it's good.

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

@@ -0,0 +1,3 @@
Bugfix
* Fix redefinition warnings when SECP192R1 and/or SECP192K1 are disabled.
Fixes #9029.
Copy link
Contributor

Choose a reason for hiding this comment

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

The last line is missing a newline at the end. That confuses some tools including assemble_changelog.py.

Signed-off-by: Elena Uziunaite <[email protected]>
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

@eleuzi01 eleuzi01 removed the needs-backports Backports are missing or are pending review and approval. label Jun 18, 2024
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

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

LGTM

@eleuzi01 eleuzi01 added needs-ci Needs to pass CI tests 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 Jun 18, 2024
@eleuzi01 eleuzi01 added needs-ci Needs to pass CI tests and removed needs-ci Needs to pass CI tests labels Jun 20, 2024
@eleuzi01 eleuzi01 removed the needs-ci Needs to pass CI tests label Jun 24, 2024
@paul-elliott-arm
Copy link
Member

Ignoring Open CI fail, as internal passed.

@paul-elliott-arm paul-elliott-arm added this pull request to the merge queue Jun 24, 2024
Merged via the queue into Mbed-TLS:development with commit d9392f0 Jun 24, 2024
5 of 6 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 bug component-crypto Crypto primitives and low-level interfaces 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