Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Statically linking openssl | libevent in tor #5

Merged
merged 1 commit into from
May 18, 2018
Merged

Statically linking openssl | libevent in tor #5

merged 1 commit into from
May 18, 2018

Conversation

jumde
Copy link
Contributor

@jumde jumde commented May 17, 2018 •
edited
Loading

Fixes: #4

@jumde jumde requested a review from darkdh May 17, 2018 19:22
darkdh
darkdh reviewed May 17, 2018
View reviewed changes
build_darwin.sh Outdated
tar -xvzf openssl-$OPENSSL_VERSION.tar.gz && \
cd openssl-$OPENSSL_VERSION && \
./config --prefix=$PWD/install no-shared no-dso && \
make
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add make test for openssl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.0m fails make test. 1.0o is not linkable so disabled that for now. I'm debugging on how to address this issue.

../certs/demo/pca-cert.pem: C = AU, ST = Queensland, O = CryptSoft Pty Ltd, CN = Test PCA (1024 bit)
error 18 at 0 depth lookup:self signed certificate
C = AU, ST = Queensland, O = CryptSoft Pty Ltd, CN = Test PCA (1024 bit)
error 10 at 0 depth lookup:certificate has expired
OK
make[1]: *** [test_verify] Error 2
make: *** [tests] Error 2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is it on Linux?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make test appears to pass for OpenSSL on Linux. I don't know what the discrepancy is offhand.

darkdh
darkdh reviewed May 17, 2018
View reviewed changes
build_darwin.sh Outdated
./configure && make && make install
tar -xvzf tor-$TOR_VERSION.tar.gz
cd tor-$TOR_VERSION && \
./configure --enable-static-libevent \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just --enable-static-tor? Also add --disable-asciidoc cause we don't need that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it enables --enable-static-zlib as well. I'll add --disable-asciidoc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it.

riastradh-brave
riastradh-brave suggested changes May 17, 2018
View reviewed changes
build_linux.sh Outdated
@@ -4,4 +4,4 @@ set -eu
docker rm -fv tor-brave || true
docker build -t tor-brave -f Dockerfile-linux --build-arg tor_version=$TOR_VERSION --build-arg zlib_version=$ZLIB_VERSION --build-arg libevent_version=$LIBEVENT_VERSION --build-arg openssl_version=$OPENSSL_VERSION .
docker run --name tor-brave -d tor-brave
docker cp tor-brave:/tor-$TOR_VERSION/src/or/tor tor-linux-$TOR_VERSION-brave-$BRAVE_TOR_VERSION
docker cp tor-brave:/tor-$TOR_VERSION/src/or/tor tor-$TOR_VERSION-linux-brave-$BRAVE_TOR_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate commit, or mention this in the commit message?

build_win32.sh
cd ..
mv Tor/tor-win32-$TOR_VERSION-brave-$BRAVE_TOR_VERSION.zip .
mv Tor/tor-$TOR_VERSION-win32-brave-$BRAVE_TOR_VERSION.zip .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate commit, or mention this in the commit message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to the commit message

build_darwin.sh Outdated
--enable-static-openssl \
--with-libevent-dir=$PWD/../libevent-$LIBEVENT_VERSION/install \
--with-openssl-dir=$PWD/../openssl-$OPENSSL_VERSION/install && \
make && make check
cd ..

cp tor-0.3.2.10/src/or/tor tor-darwin-$TOR_VERSION-brave-$BRAVE_TOR_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check the list of libraries this links against and fail if /usr/local or /opt/local appears in it? (otool -L or something should do it.)

riastradh-brave
riastradh-brave previously requested changes May 18, 2018
View reviewed changes
Copy link
Contributor

@riastradh-brave riastradh-brave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the test suites taking so long it's making it too hard to pay attention during long end-to-end builds to effectively iterate changes? If so, maybe we should use a makefile instead of a shell script, so we can at least save incremental progress?

build_darwin.sh Outdated
LDFLAGS="-L$PWD/../openssl-$OPENSSL_VERSION/" \
CPPFLAGS="-I$PWD/../openssl-$OPENSSL_VERSION/include" \
LDFLAGS="-L/usr/local/opt/openssl/lib" \
CPPFLAGS="-I/usr/local/opt/openssl/include" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using the system-wide /usr/local/opt version instead of the local newly built version?

build_darwin.sh Outdated
--prefix=$PWD/install \
--disable-shared \
--enable-static \
--with-pic && \
make && make check
make && make install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is make check removed?

Dockerfile-linux Outdated
@@ -30,11 +30,11 @@ RUN \
--disable-shared \
--enable-static \
--with-pic && \
make && make check && make install
make && make install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is make check removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it for testing since it takes about 30 mins. Putting it back in soon.

@jumde jumde force-pushed the enabling_static_libevent_openssl_darwin branch from d170a3c to 1c9830b Compare May 18, 2018 18:28
jumde
jumde commented May 18, 2018
View reviewed changes
build_darwin.sh
tar -xvzf openssl-$OPENSSL_VERSION.tar.gz && \
cd openssl-$OPENSSL_VERSION && \
./config --prefix=$PWD/install no-shared no-dso && \
make
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make test for darwin fails because of expired certs. Some forums say that it is because of a conflict with xcode and brew.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Can you put a comment in here explaining this, and quoting the error so we can see if it's still around in the future?

Copy link
Contributor Author

@jumde jumde May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check this issue: #6

@jumde jumde force-pushed the enabling_static_libevent_openssl_darwin branch from 1c9830b to 9b194ab Compare May 18, 2018 18:47
diracdeltas
diracdeltas reviewed May 18, 2018
View reviewed changes
env.sh Outdated
export TOR_HASH=60df77c31dcf94fdd686c8ca8c34f3b70243b33a7344ecc0b719d5ca2617cbee

export LIBEVENT_KEY=B86086848EF8686D
export TOR_KEY=FE43009C4607B1FB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a huge deal, but i think you want the full key fingerprint here. ex: 2133BC600AB133E1D826D173FE43009C4607B1FB for tor

@jumde jumde force-pushed the enabling_static_libevent_openssl_darwin branch 2 times, most recently from 51770c4 to 1168dcf Compare May 18, 2018 21:01
@jumde jumde dismissed riastradh-brave’s stale review May 18, 2018 21:04

Code updated per comments

@jumde
1. Statically linking openssl | libevent in tor for mac
3fbbe05 
2. Correct binary naming for windows and linux
3. Move hashes/keys to env.sh
@jumde jumde force-pushed the enabling_static_libevent_openssl_darwin branch from 2357836 to 3fbbe05 Compare May 18, 2018 21:33
@jumde jumde merged commit 11eea37 into master May 18, 2018
@mihaiplesa mihaiplesa deleted the enabling_static_libevent_openssl_darwin branch January 19, 2022 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh left review comments

@diracdeltas diracdeltas diracdeltas left review comments

@riastradh-brave riastradh-brave riastradh-brave approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jumde @diracdeltas @darkdh @riastradh-brave