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

Remove deprecated things from crypto_compat.h and dependent tests. #4344

Conversation

TRodziewicz
Copy link
Contributor

@TRodziewicz TRodziewicz commented Apr 13, 2021

Signed-off-by: TRodziewicz [email protected]

Description

Remove deprecated things in psa/crypto_compat.h. For more details see the originating issue.

Fixes: #4284

Status

READY

Requires Backporting

NO

Todos

  • Tests
  • Documentation
  • Changelog updated

@TRodziewicz TRodziewicz added needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) labels Apr 13, 2021
@TRodziewicz TRodziewicz self-assigned this Apr 13, 2021
Signed-off-by: TRodziewicz <[email protected]>
@TRodziewicz TRodziewicz 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-ci Needs to pass CI tests labels Apr 14, 2021
@mpg mpg added the mbedtls-3 label Apr 15, 2021
@mpg mpg changed the base branch from development to development_3.0 April 15, 2021 11:01
@mpg mpg dismissed gilles-peskine-arm’s stale review April 15, 2021 11:01

The base branch was changed.

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

This is looking good to me, but the CI is not fully happy:

******************************************************************

* check_doxygen_warnings: Check: doxygen warnings (builds the documentation) 

* Fri Apr 16 21:18:25 UTC 2021

******************************************************************

/var/lib/build/include/psa/crypto.h:779: warning: explicit link request to 'PSA_KEY_TYPE_GET_CURVE' could not be resolved

FAIL

^^^^check_doxygen_warnings: Check: doxygen warnings (builds the documentation): tests/scripts/doxygen.sh -> 1^^^^

@mpg mpg added needs-ci Needs to pass CI tests needs-work and removed 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 Apr 19, 2021
@mpg
Copy link
Contributor

mpg commented Apr 19, 2021

EDIT: IGNORE THIS - SEE BELOW!

Note: the doxygen warning only occurs in the pr-merge job, so I think before you can reproduce and fix you need to merge the current development_3.0 into your branch:

git checkout remove_deprecated_things_in_crypto_compat_h
git fetch public
git merge public/development_3.0

(Replace public with the name of the remote that points to ARMmbed/mbedtls.git, you can check with git remove -v. If you don't have a remote pointing to that repo yet, you can get one by running git remote add public [email protected]:ARMmbed/mbedtls.git before running the above commands.)

Then you should be able to reproduce the issue y running tests/scripts/doxygen.sh and fix it. (Note: if your doxygen version is too recent, you'll also get other warnings which you don't need to fix, just fix the one that's also found by the CI. The others are already tracked elsewhere.)

@gilles-peskine-arm
Copy link
Contributor

The CI problem is a bug introduced by #4174 and fixed by #4318. It's present in the development_3.0 branch, which was last synched from development_2.x between the merge of these two PR. The proper fix would be to update the 3.0 branch.

@mpg
Copy link
Contributor

mpg commented Apr 19, 2021

Oh, so this can be ignored for the purpose of merging this PR. Thanks Gille for clarifying that.

@gilles-peskine-arm gilles-peskine-arm added needs-preceding-pr Requires another PR to be merged first and removed needs-work labels Apr 19, 2021
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM and Gilles clarified that the doxygen issue was pre-existent and is already being fixed elsewhere, so we're good to go.

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Apr 19, 2021
@mpg mpg merged commit 16529bd into Mbed-TLS:development_3.0 Apr 19, 2021
@gilles-peskine-arm
Copy link
Contributor

Uh, sorry if I was unclear, I meant that we should wait to merge this PR until 3.0 is up-to-date. I've made a PR to update 3.0.

Never mind about merging this PR, let's just merge #4366 quickly.

@mpg
Copy link
Contributor

mpg commented Apr 19, 2021

Aw, I'll review 4366 quickly then.

daverodgman pushed a commit that referenced this pull request Apr 23, 2021
…_crypto_compat_h

Remove deprecated things from crypto_compat.h and dependent tests.
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 size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove deprecated things in psa/crypto_compat.h
4 participants