- Notifications
You must be signed in to change notification settings - Fork425
OSX: fix OpenSSL detection for 10.11. Fixes #690#691
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
glynos commentedOct 3, 2016
The checks that have failed are to do with a configuration problem on Travis. I don't have access to an Apple environment.@deanberris can you confirm that this can be integrated? Also, does this fix need to be applied on the master branch? |
anonimal commentedOct 3, 2016
What spurred my need for a fix washttps://build.getmonero.org/builders/kovri-all-osx-10.11/builds/4 I can't easily provide a new fixed build report because I can't easily update our submodule to point to my branch without causing other issues. I did ssh into this machine though and confirm manually. |
anonimal commentedOct 3, 2016
AFAICT this will also be needed in master, I'll confirm right now. |
anonimal commentedOct 3, 2016
Yes, it's needed for master too. |
CMakeLists.txt Outdated
| if (APPLE) | ||
| # If we're on OS X check for Homebrew's copy of OpenSSL instead of Apple's | ||
| if (NOT OpenSSL_DIR) | ||
| execute_process (COMMAND brew --prefix openssl |
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.
I'm not a fan of requiring homebrew just like this unconditionally on APPLE. Personally, when I build the library I provide theOpenSSL_DIR option myself.
I'd be open to accepting this if you make it so that we can detect whether the user has homebrew installed (something that could be checked at configuration time).
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.
@deanberris Agreed. I'll make the changes. If possible, maybe I should only apply this to OSX 10.11 since other versions appear unaffected?
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.
I think optional homebrew applies even to before OSX 10.11.
anonimal commentedOct 6, 2016
@deanberris@glynos I've pushedacb020b. Its a little biased with the Homebrew warning but it will stay out of the way. Confirmed functional/build successful on 10.11 and earlier machines. Should I reopen this PR to master? |
deanberris left a comment
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.
LGTM -- thanks@anonimal!
deanberris commentedOct 7, 2016
Also, yes, please do the same to master -- that would be most appreciated. |
- Cherry-pick'sacb020b 0.13-release- Referencescpp-netlib#691
This is a confirmed fix for#690 and also confirmed to work on 10.9.5 (this should work on 10.10 as well).
The problem with this fix is the assumption that the user is using homebrew. If we shouldn't enforce this requisite, I'm open to ideas for an alternate fix.