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.

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

build: Remove libssl from LDADD unless gui #14212

Merged
merged 1 commit into from Sep 15, 2018

Conversation

Projects
None yet
8 participants
@MarcoFalke
Copy link
Member

commented Sep 13, 2018

libssl is only used by the gui, so no need to LDADD it to the other tools and binaries

Follow up of the commit which removed rpcssl: 40b556d

@practicalswift

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

utACK fa328ae9133b257e93a85db31df568b792c44bd5

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

right, bitcoind needs libcrypto not libssl
utACK fa328ae9133b257e93a85db31df568b792c44bd5

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

BTW—are you sure bitcoin-qt does need libssl, or does it only use it through qt? I know it uses some OpenSSL functionality directly but not from what library.

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

at the least it could be removed from src/Makefile.test.include as well

The latter gives a linker error:

…/bitcoin/src/qt/test/test_main.cpp:71: undefined reference to `OPENSSL_init_ssl'

Note, though, that this is the only occurence. Why does this need to be initialized? (removing it does not seem to have any influence on the tests passing)

FWIW I could purge direct use of LIBSSL completely, without any adverse effects: https://github.com/laanwj/bitcoin/tree/2018_10_marcofalke_noLibSSL probably want to do a gitian build just to be sure

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noLibSSL branch 2 times, most recently Sep 13, 2018

@MarcoFalke MarcoFalke changed the title build: Remove libssl from LDADD unless gui build: Remove libssl Sep 13, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noLibSSL branch Sep 13, 2018

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

If you're completely purging it, can libssl.pc also be removed from depends/packages/openssl.mk?

Succesfully compiled and lightly tested 23ec611 on macOS 10.13.6, including opening a BIP-70 payment request.

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

If you're completely purging it, can libssl.pc also be removed from depends/packages/openssl.mk?

I don't think removing it from depends is safe—Qt is still relying on it for https:// connections, which are necessary for BIP70. It's just we have no direct dependency on it anymore.

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@laanwj are you sure QT isn't using openssl instead of libssl? I'm generally confused by the distinction, but the QT docs mention:

When building Qt from source, the configuration system checks for the presence of the openssl/opensslv.h header provided by source or developer packages of OpenSSL.

Or is there some other place where QT does need libssl?

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noLibSSL branch Sep 13, 2018

@ken2812221

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@Sjors libssl is a library of openssl, you can take a look at https://github.com/openssl/openssl/blob/master/README#L27-L29

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@laanwj are you sure QT isn't using openssl instead of libssl? I'm generally confused by the distinction, but the QT docs mention:

As I understand it, OpenSSL is a combination of two libraries, "libcrypto" and "libssl", the first handles cryptography and certificate operations and randomness, the second networking. Our project uses only the former, which is why this PR works, however Qt (the library) uses both, as it handles both cryptography and TLS/SSL network conections.

I could be misunderstanding it, though.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

utACK b0e2411144c1204793ed25e598963c7d8eae12f6

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@ken2812221 @laanwj you're right, openssl/opensslv.h indeed refers to the whole kitchen sink, I misread the README:
schermafbeelding 2018-09-13 om 20 56 38

Probably worth mentioning in the commit message that this removes the direct dependency, but not the indirect one via QT, as pointed out above. Or perhaps add comment in openssl.mk why it's still there (although it won't compile if anyone tries to remove it).

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noLibSSL branch 2 times, most recently to 450b9f5 Sep 13, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1809-noLibSSL branch from 450b9f5 to cccc362 Sep 13, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

@laanwj Your branch to remove it altogether seems to fail: https://travis-ci.org/bitcoin/bitcoin/builds/428326156 (Note the included libssl header in the qt tests file: 6e996d3)

@MarcoFalke MarcoFalke changed the title build: Remove libssl build: Remove libssl from LDADD unless gui Sep 13, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@MarcoFalke argh—so it looks like that when Qt is linked statically, as in gitian, it relies on the bitcoin build pulling -lssl in. This is no problem when building against qt dynamically, which is why I didn't notice before. e.g, here's Qt indirectly referring to ssl functions:

/home/ubuntu/build/bitcoin/depends/i686-w64-mingw32/share/../lib/libQt5Network.a(qsslsocket_openssl_symbols.o):qsslsocket_openssl_symbols.cpp:(.text+0x4e1): un
defined reference to `SSL_accept'
/home/ubuntu/build/bitcoin/depends/i686-w64-mingw32/share/../lib/libQt5Network.a(qsslsocket_openssl_symbols.o):qsslsocket_openssl_symbols.cpp:(.text+0x4f1): un
defined reference to `SSL_clear'
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

Ah, anyway. I removed the SSL_LIB from test_bitcoin as you suggested (and some other changes as well)

@ken2812221
Copy link
Member

left a comment

utACK cccc362

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2018

I am going to merge this after the gitian build returns

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

Gitian builds for commit f09bc7e (master):

Gitian builds for commit d98354d6374d31886c2397119065be3dd15df2ff (master and this pull):

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Sep 15, 2018

Merge bitcoin#14212: build: Remove libssl from LDADD unless gui
cccc362 build: Remove libssl from LDADD unless gui (MarcoFalke)

Pull request description:

  libssl is only used by the gui, so no need to LDADD it to the other tools and binaries

  Follow up of the commit which removed rpcssl: 40b556d

Tree-SHA512: 9dbdf4faf40699cea3a37349ac83dbcacdaa062f5338416ff4ba77924c47d9e148b27218165c5aa3584a1ef4899e0fa237ff571208aa0b98803761e802d1e5dc

@MarcoFalke MarcoFalke merged commit cccc362 into bitcoin:master Sep 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1809-noLibSSL branch Sep 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.