- 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
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? |
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. |
AFAICT this will also be needed in master, I'll confirm right now. |
Yes, it's needed for master too. |
| 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.
@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? |
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!
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.