GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — OSX: fix OpenSSL detection for 10.11. Fixes #690 by anonimal · Pull Request #691 · cpp-netlib/cpp-netlib

Conversation

@anonimal

@anonimal anonimal commented Oct 3, 2016

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@anonimal
OSX: fix OpenSSL detection for 10.11. Fixes cpp-netlib#690
b8c47f9
@glynos

glynos commented Oct 3, 2016

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonimal

anonimal commented Oct 3, 2016

Copy link
Copy Markdown
Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonimal

anonimal commented Oct 3, 2016

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonimal

anonimal commented Oct 3, 2016

Copy link
Copy Markdown
Author

Yes, it's needed for master too.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
deanberris
deanberris requested changes Oct 5, 2016
View reviewed changes
Comment thread 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

@deanberris deanberris Oct 5, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@anonimal anonimal Oct 5, 2016

Copy link
Copy Markdown
Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@deanberris deanberris Oct 6, 2016

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@anonimal
OSX: update OpenSSL detection. Refs cpp-netlib#690 cpp-netlib#691
acb020b
@anonimal

anonimal commented Oct 6, 2016

Copy link
Copy Markdown
Author

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
deanberris
deanberris approved these changes Oct 7, 2016
View reviewed changes

@deanberris deanberris left a comment

Copy link
Copy Markdown
Member

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@deanberris
deanberris merged commit 6a7e92c into cpp-netlib:0.13-release Oct 7, 2016
@deanberris

deanberris commented Oct 7, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonimal added a commit to anonimal/cpp-netlib that referenced this pull request Oct 7, 2016
@anonimal
OSX: update OpenSSL detection. Closes cpp-netlib#690
fb6a375 
- Cherry-pick's acb020b 0.13-release
- References cpp-netlib#691
@anonimal
anonimal deleted the fix-690 branch October 7, 2016 15:42
This was referenced Oct 7, 2016
OSX: update OpenSSL detection. Closes #690 #694
Merged
OSX: build fails on 10.11 monero-project/kovri#395
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deanberris deanberris deanberris approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@anonimal @glynos @deanberris
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.