Skip to content

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Feb 2, 2020

Contribution description

Update to the latest upstream release

https://www.wolfssl.com/docs/wolfssl-changelog/

Unfortunately the new version introduced some unused parameter warnings, so I disabled them for this pkg.

Testing procedure

I ran both tests/pkg_wolfssl and tests/pkg_wolfcrypt-ed25519-verify on native and esp32.

Issues/PRs references

none

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 2, 2020
@chrysn
Copy link
Member

chrysn commented Feb 2, 2020

I know it's not required RIOT policy, but it's a Debian habit I'd consider useful here: Could you mention inside the patch where it was forwarded to upstream, if so, or whether it was cherry-picked from? That should help keeping track of patches we need to keep around as opposed to those we can get rid of eventually by being good downstreams.

Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

I've run the same tests on an nRF52840 (on the -dongle board), and they built and ran fine. I did not look into the changes in WolfSSL in particular other than skimming the change log, and there's nothing in there to indicate any downsides to an upgrade. Code size increase is, as expected, minimal (114k to 115k text for the self tests where increases might be expected, 23k by 8 byte increase for signature test).

@benpicco
Copy link
Contributor Author

tbh the patch was a bit too messy to upstream and all the different compile-time config options make it hard to get this right.

But there is a much simpler solution that is employed by other pkgs too…

Copy link
Contributor

@MichelRottleuthner MichelRottleuthner left a comment

Choose a reason for hiding this comment

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

Tested on native and pba-d-01-kw2x with examples/dtls-wolfssl, tests/pkg_wolfcrypt-ed25519-verify and tests/wolfssl-test. All good, apart from examples/dtls-wolfssl but this is also the case on master.
Just one non-blocking suggestion below. ACK

Update to the latest upstream release

https://www.wolfssl.com/docs/wolfssl-changelog/

This unfortunetaly introduced some unused parameter warnings, so disable
the warning via CFLAGS.
@benpicco benpicco requested a review from fjmolinas as a code owner March 2, 2020 14:12
@MichelRottleuthner MichelRottleuthner merged commit 10226aa into RIOT-OS:master Mar 2, 2020
@benpicco benpicco deleted the wolfssl-update branch March 2, 2020 14:57
@leandrolanzieri leandrolanzieri added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Mar 13, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants