build: Remove libssl from LDADD unless gui #14212

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1809-noLibSSL changing 3 files +4 −4
  1. MarcoFalke commented at 2:25 PM on September 13, 2018: member

    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: 40b556d3742a1f65d67e2d4c760d0b13fe8be5b7

  2. fanquake added the label Build system on Sep 13, 2018
  3. practicalswift commented at 3:01 PM on September 13, 2018: contributor

    utACK fa328ae9133b257e93a85db31df568b792c44bd5

  4. laanwj commented at 3:55 PM on September 13, 2018: member

    right, bitcoind needs libcrypto not libssl utACK fa328ae9133b257e93a85db31df568b792c44bd5

  5. laanwj commented at 3:57 PM on September 13, 2018: member

    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.

  6. laanwj commented at 3:59 PM on September 13, 2018: member

    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

  7. MarcoFalke force-pushed on Sep 13, 2018
  8. MarcoFalke force-pushed on Sep 13, 2018
  9. MarcoFalke renamed this:
    build: Remove libssl from LDADD unless gui
    build: Remove libssl
    on Sep 13, 2018
  10. MarcoFalke force-pushed on Sep 13, 2018
  11. Sjors commented at 6:32 PM on September 13, 2018: member

    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.

  12. laanwj commented at 6:34 PM on September 13, 2018: member

    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.

  13. Sjors commented at 6:41 PM on September 13, 2018: member

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

  14. MarcoFalke force-pushed on Sep 13, 2018
  15. ken2812221 commented at 6:46 PM on September 13, 2018: contributor

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

  16. laanwj commented at 6:47 PM on September 13, 2018: member

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

  17. jnewbery commented at 6:51 PM on September 13, 2018: member

    utACK b0e2411144c1204793ed25e598963c7d8eae12f6

  18. Sjors commented at 7:00 PM on September 13, 2018: member

    @ken2812221 @laanwj you're right, openssl/opensslv.h indeed refers to the whole kitchen sink, I misread the README: <img width="564" alt="schermafbeelding 2018-09-13 om 20 56 38" src="https://user-images.githubusercontent.com/10217/45509345-90acf980-b797-11e8-82bc-00a86ea68bd3.png">

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

  19. MarcoFalke force-pushed on Sep 13, 2018
  20. MarcoFalke force-pushed on Sep 13, 2018
  21. build: Remove libssl from LDADD unless gui cccc362d62
  22. MarcoFalke force-pushed on Sep 13, 2018
  23. MarcoFalke commented at 8:33 PM on September 13, 2018: member

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

  24. MarcoFalke renamed this:
    build: Remove libssl
    build: Remove libssl from LDADD unless gui
    on Sep 13, 2018
  25. laanwj commented at 9:01 PM on September 13, 2018: member

    @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'
  26. MarcoFalke commented at 9:04 PM on September 13, 2018: member

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

  27. ken2812221 approved
  28. ken2812221 commented at 3:47 AM on September 14, 2018: contributor

    utACK cccc362d62b0ba9475b1ac47d5908f77f7eb5d21

  29. MarcoFalke added the label Needs gitian build on Sep 14, 2018
  30. MarcoFalke commented at 12:59 PM on September 14, 2018: member

    I am going to merge this after the gitian build returns

  31. laanwj commented at 1:58 PM on September 14, 2018: member

    +1

    On Fri, Sep 14, 2018, 14:59 MarcoFalke notifications@github.com wrote:

    I am going to merge this after the gitian build returns

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/14212#issuecomment-421350613, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutnwnCsdev2hGBN-L_cE8mrM3TNZhks5ua6hAgaJpZM4WnXUW .

  32. DrahtBot commented at 11:00 PM on September 14, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->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.

  33. DrahtBot commented at 9:09 AM on September 15, 2018: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit f09bc7ec9859bba6d1df765adb1030d276b8f626 (master):

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

  34. DrahtBot removed the label Needs gitian build on Sep 15, 2018
  35. MarcoFalke referenced this in commit c53e083a49 on Sep 15, 2018
  36. MarcoFalke merged this on Sep 15, 2018
  37. MarcoFalke closed this on Sep 15, 2018
  38. MarcoFalke deleted the branch on Sep 15, 2018
  39. Munkybooty referenced this in commit 23c410285a on Jul 8, 2021
  40. Munkybooty referenced this in commit eadfd02148 on Jul 9, 2021
  41. Munkybooty referenced this in commit 4ea443ff6f on Jul 11, 2021
  42. Munkybooty referenced this in commit a41973a351 on Jul 12, 2021
  43. MarcoFalke locked this on Sep 8, 2021
Contributors
MarcoFalkepracticalswiftlaanwjSjorsken2812221jnewberyDrahtBot
Labels
Build system
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 06:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me