Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 7, 2016

Conversation

anonimal
Copy link

@anonimal anonimal commented Oct 3, 2016

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.

@glynos
Copy link
Member

glynos commented Oct 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
Copy link
Author

anonimal commented Oct 3, 2016

What spurred my need for a fix was https://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
Copy link
Author

anonimal commented Oct 3, 2016

AFAICT this will also be needed in master, I'll confirm right now.

@anonimal
Copy link
Author

anonimal commented Oct 3, 2016

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
Copy link
Member

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 the OpenSSL_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).

Copy link
Author

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?

Copy link
Member

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
Copy link
Author

anonimal commented Oct 6, 2016

@deanberris @glynos I've pushed acb020b. 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?

Copy link
Member

@deanberris deanberris left a 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 deanberris merged commit 6a7e92c into cpp-netlib:0.13-release Oct 7, 2016
@deanberris
Copy link
Member

Also, yes, please do the same to master -- that would be most appreciated.

anonimal added a commit to anonimal/cpp-netlib that referenced this pull request Oct 7, 2016
@anonimal anonimal deleted the fix-690 branch October 7, 2016 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants