Opened 4 weeks ago

Last modified 3 weeks ago

#23602 merge_ready defect

Detect homebrew OpenSSL on OSX (was:Fix compilation on macOS)

Reported by: hellais Owned by:
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-backport, 032-backport, 031-backport-maybe, 030-backport-maybe
Cc: pastly Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by hellais)

On macOS the build toolchain is broken in the latest master (commit://4a3b61a5b32d4df7d627f993deec97f1db451910).

This is due to the fact that macOS comes with an ancient version of openssl that the tor autoconf script will resolve to first. Instead, if the user has installed homebrew, it should first pick the homebrew version.

In this branch I added a patch to the autoconf script that fixes this: https://github.com/hellais/tor/commit/e55a198c517bb5e40acd395e562891c4927e54a9.

More details about the fix are in the commit message and comment in the code.

I tested that this works as expected on macOS 10.12.6 with homebrew 1.3.2 and openssl 1.0.2l.

Child Tickets

Change History (14)

comment:1 Changed 4 weeks ago by pastly

Cc: pastly added

comment:2 Changed 4 weeks ago by hellais

Description: modified (diff)

comment:3 Changed 4 weeks ago by cypherpunks

Status: newneeds_review

comment:4 Changed 4 weeks ago by pastly

Milestone: Tor: 0.3.3.x-final

Works for me on 10.11.6 with homebrew 1.3.2 and homebrew openssl 1.0.2l.

Still compiles on Linux too ;)

comment:5 Changed 4 weeks ago by nickm

Status: needs_reviewneeds_revision

I think we need a different fix here. Look at how we're calling TOR_SEARCH_LIBRARY: there are two problems.

The first problem is that we are detecting OpenSSL by looking for RAND_add(). That was once okay, but now that we require OpenSSL 1.0.1, we should look for a function that was introduced in OpenSSL 1.0.1. We should probably make sure that it's something that libressl also has, if possible.

(We could probably get away with looking for a function that was introduced in 1.0.0, since we're trying to avoid 0.9.8 in particular.)

The second problem is that our search path doesn't include /usr/local/opt/ and /usr/local/opt/openssl/. But we could fix that just by adding them.

comment:6 Changed 4 weeks ago by hellais

The first problem is that we are detecting OpenSSL by looking for RAND_add(). That was once okay, but now that we require OpenSSL 1.0.1, we should look for a function that was introduced in OpenSSL 1.0.1. We should probably make sure that it's something that libressl also has, if possible.

Correct me if I am wrong, but isn't this a separate issue unrelated to making the build system work on macOS with homebrew?

Even if it were to pass in this case, users will in almost every case want to use the homebrew version of libraries if they have it installed.

The second problem is that our search path doesn't include /usr/local/opt/ and /usr/local/opt/openssl/. But we could fix that just by adding them.

That is actually how I originally implemented the fix for this, but that will actually not work since the order in which libraries are searched for needs to change in macOS. The lookup order there is made to first look for system libraries (in this case the crummy macOS default ones) and only after the ones in the special prefixes. On macOS the order should be inverted.

comment:7 Changed 4 weeks ago by nickm

Your questions are the answers to each other! :)

If we make it so that only OpenSSL >= 1.0.1 passes the test, then the system libraries will get skipped.

comment:8 Changed 4 weeks ago by nickm

Summary: Fix compilation on macOSDetect homebrew OpenSSL on OSX (was:Fix compilation on macOS)

comment:9 in reply to:  5 Changed 4 weeks ago by nickm

Replying to nickm:

Now that we require OpenSSL 1.0.1, we should look for a function that was introduced in OpenSSL 1.0.1. We should probably make sure that it's something that libressl also has, if possible.

TLSv1_1_method() seems like a good candidate.

comment:10 Changed 4 weeks ago by nickm

Status: needs_revisionneeds_review

Branch ticket23602_029 in my public repository has an example of what I'm thinking of here. It's based on maint-0.2.9, but should merge cleanly to master. It works for me on OSX with homebrew. Does it work for you?

comment:11 Changed 4 weeks ago by pastly

Works for me on OSX 029 and on master

comment:12 Changed 4 weeks ago by teor

Keywords: 029-backport 032-backport 031-backport-maybe 030-backport-maybe added
Status: needs_reviewmerge_ready

Homebrew has always worked for me, but I pass custom paths to ./configure.
Getting it working with minimal intervention would be great.

./configure && make test works for me on macOS 10.12 with homebrew and the standard developer tools. I think I might have a recent clang custom installed, too.

comment:13 Changed 4 weeks ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.1.x-final

Hm. I'm okay merging this in 0.3.2 for now. Let's see if it causes any trouble there before we think about backporting further.

comment:14 Changed 3 weeks ago by hellais

Cherry picking da36def955ca749230896c219251ba52181688c0 into master it works for me on macOS 10.12.6 and Homebrew 1.3.4 without any additional configuration:

./autogen.sh && ./configure && make
Note: See TracTickets for help on using tickets.