-
Notifications
You must be signed in to change notification settings - Fork 557
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
wolfSSL: Update to 5.7.2 #9578
wolfSSL: Update to 5.7.2 #9578
Conversation
Thanks for your contribution! |
SecTrustEvaluateWithError requires macOS 10.14, you should use that SDK instead of skipping macOS entirely 🙂 |
How can I get it to use that version? Do you have an example? |
And the windows error https://buildkite.com/julialang/yggdrasil/builds/13798#01926c26-02b0-4d93-ac29-084611c7bd56/667-952 looks like an error in the build system, which I haven't looked into, but doesn't sound impossible to fix |
Yggdrasil/A/AMReX/build_tarballs.jl Lines 16 to 17 in 46da732
Yggdrasil/A/AMReX/build_tarballs.jl Lines 24 to 31 in 46da732
|
Thanks, tried something different, still waiting to see if it worked. |
W/wolfSSL/build_tarballs.jl
Outdated
if [[ "${target}" == *-apple-darwin* ]]; then | ||
CFLAGS="-mmacosx-version-min=10.14" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You're matching also aarch64-apple-darwin, which doesn't have the same problem (we use the 11.0 SDK there)
- it isn't sufficient to pretend to have the 10.14 SDK, you actually need to use it as suggested in wolfSSL: Update to 5.7.2 #9578 (comment) 🙂
Side node, export MACOSX_DEPLOYMENT_TARGET=10.14
is a nicer way to accomplish this, since our compiler wrappers read the MACOSX_DEPLOYMENT_TARGET
env var, the way you do it the compiler gets duplicate and conflicting -mmacosx-version-min
options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Did that and it worked. Any chance we can merge this in without the Windows build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying something for the windows build now.
This should be ready now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please upstream this patch so that we'll be able to drop it in the future? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened a PR here: wolfSSL/wolfssl#8068. It'll be a while until that's merged and released though.
Off-topic: @giordano It seems that we now require "Update to latest master" button to be hit before merging is allowed. Is that by design - since it probably increases the CI load a fair bit. Sorry to ask here, but I saw it on a couple of PRs (and it may have already been discussed somewhere I didn't see it). |
Thanks for pointing it out, that was not intended, my mistake while I was reviewing branch protection settings, I disabled that. |
* wolfSSL: Update to 5.7.2 * wolfSSL: Exclude failing platforms * wolfSSL: Try a fix for macOS * wolfSSL: Try the fix for macOS * wolfSSL: Fix Windows build --------- Co-authored-by: Viral B. Shah <[email protected]>
No description provided.