Version check MbedTLS instead of introducing a new define when initializing PSA Crypto API #516
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
teejusb
changed the title
teejusb
changed the title
teejusb
commented
May 8, 2024
| #if defined(IXWEBSOCKET_MBEDTLS_USE_PSA_CRYPTO) | ||
| psa_crypto_init(); | ||
| #endif | ||
| if (MBEDTLS_VERSION_MAJOR >= 3 && MBEDTLS_VERSION_MINOR >= 6 && MBEDTLS_VERSION_PATCH >= 0) |
There was a problem hiding this comment.
I can also add guards for MBEDTLS_VERSION_MAJOR etc if you'd like. I figured this would be a safe assumption so I opted to leave them out.
bsergean
approved these changes
May 8, 2024
|
thanks ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the define introduced in #514, in favor of just version checking the MbedTLS (defined here). I think this implementation is more portable as any projects dependent on this change can get it immediately when updating their MbedTLS library, instead of manually adding in a new define.
Details
It looks like brew updated their mbedtls version 2 months ago as per this link.
This update happened on March 28th, while the last IXWebSocket update before this changed happened one day earlier on March 27th.
Since there hadn't been any updates to this repo after March 28th, the GitHub action was not run. And since the failing test uses brew install mbedtls here, it is now getting version 3.6.0, instead of 3.5.2. I think this implies that even without the changes in #514, the runner would have started to fail on the next PR.
Interestingly, and also unknowingly, the changes in #514 were created to address exactly this issue :) The changes in my project to enable IXWebSocket to use that PR can be found here.