Skip to content

build: Remove libssl from LDADD unless gui #14212

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
Merged
maflcko merged 1 commit into from
Sep 15, 2018
Merged

build: Remove libssl from LDADD unless gui #14212

maflcko merged 1 commit into from
Sep 15, 2018

Conversation

@maflcko
Copy link
Member

@maflcko maflcko 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
Copy link
Contributor

practicalswift commented Sep 13, 2018

utACK fa328ae9133b257e93a85db31df568b792c44bd5

@laanwj
Copy link
Member

laanwj commented Sep 13, 2018

right, bitcoind needs libcrypto not libssl
utACK fa328ae9133b257e93a85db31df568b792c44bd5

@laanwj
Copy link
Member

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

laanwj commented Sep 13, 2018 •
edited
Loading

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

@maflcko maflcko force-pushed the Mf1809-noLibSSL branch 2 times, most recently from fa4e316 to fa16f37 Compare September 13, 2018 17:19
@maflcko maflcko changed the title build: Remove libssl from LDADD unless gui build: Remove libssl Sep 13, 2018
@Sjors
Copy link
Member

Sjors commented Sep 13, 2018 •
edited
Loading

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

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

Sjors commented Sep 13, 2018 •
edited
Loading

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

@ken2812221
Copy link
Contributor

ken2812221 commented Sep 13, 2018 •
edited
Loading

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

@laanwj
Copy link
Member

laanwj 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
Copy link
Contributor

jnewbery commented Sep 13, 2018

utACK b0e2411144c1204793ed25e598963c7d8eae12f6

@Sjors
Copy link
Member

Sjors commented Sep 13, 2018 •
edited
Loading

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

@maflcko maflcko force-pushed the Mf1809-noLibSSL branch 2 times, most recently from 1902767 to 450b9f5 Compare September 13, 2018 19:51
@maflcko
Copy link
Member Author

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

@maflcko maflcko changed the title build: Remove libssl build: Remove libssl from LDADD unless gui Sep 13, 2018
@laanwj
Copy link
Member

laanwj commented Sep 13, 2018 •
edited
Loading

@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'

@maflcko
Copy link
Member Author

maflcko commented Sep 13, 2018

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

ken2812221
ken2812221 approved these changes Sep 14, 2018
View reviewed changes
Copy link
Contributor

@ken2812221 ken2812221 left a comment

Choose a reason for hiding this comment

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

utACK cccc362

@maflcko
Copy link
Member Author

maflcko commented Sep 14, 2018

I am going to merge this after the gitian build returns

@laanwj
Copy link
Member

laanwj commented Sep 14, 2018 via email

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 14, 2018

Note to reviewers: This pull request conflicts with the following ones:

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

DrahtBot commented Sep 15, 2018

Gitian builds for commit f09bc7e (master):

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

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Sep 15, 2018
Merge bitcoin#14212: build: Remove libssl from LDADD unless gui
c53e083 
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
@maflcko maflcko merged commit cccc362 into bitcoin:master Sep 15, 2018
@maflcko maflcko deleted the Mf1809-noLibSSL branch September 15, 2018 13:37
@Bushstar Bushstar mentioned this pull request Oct 11, 2018
Merged
@Sjors Sjors mentioned this pull request Dec 12, 2018
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ken2812221 ken2812221 ken2812221 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Build system

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maflcko @practicalswift @laanwj @Sjors @ken2812221 @jnewbery @DrahtBot @fanquake