depends: Build secondary deps statically. #16041

pull dongcarl wants to merge 1 commits into bitcoin:master from dongcarl:2019-05-depends-static-if-secondary-dep changing 5 files +28 −4
  1. dongcarl commented at 3:56 pm on May 17, 2019: member

    Secondary dependencies don’t need to be shared. Comes at theuni’s request.

    To do: add documentation so future secondary dependencies aren’t shared. DONE

    Let’s merge this first as #15844 is based on this PR.

  2. dongcarl added the label Build system on May 17, 2019
  3. dongcarl added the label Needs gitian build on May 17, 2019
  4. dongcarl force-pushed on May 17, 2019
  5. dongcarl marked this as ready for review on May 17, 2019
  6. practicalswift commented at 5:31 pm on May 17, 2019: contributor
    Concept ACK
  7. DrahtBot commented at 5:44 pm on May 17, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15844 (depends: Purge libtool archives by dongcarl)
    • #15277 ([Help Wanted] contrib: Enable building in Guix containers by dongcarl)

    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.

  8. dongcarl force-pushed on May 17, 2019
  9. fanquake commented at 10:19 pm on May 17, 2019: member
    Concept ACK
  10. DrahtBot removed the label Needs gitian build on May 18, 2019
  11. dongcarl requested review from theuni on May 20, 2019
  12. in depends/packages.md:8 in 256b204ad2 outdated 
    4@@ -5,6 +5,9 @@ The package "mylib" will be used here as an example
5 
6 General tips:
7 - mylib_foo is written as $(package)_foo in order to make recipes more similar.
8+- Secondary dependency packages relative to the bitcoin binaries/libraries (i.e.
    sipa commented at 6:33 pm on May 22, 2019:
    What is the rationale for this? Does this improve build times, or compatibility of some kind?
    dongcarl commented at 7:34 pm on May 22, 2019:

    @sipa @MarcoFalke

    It improves general build reliability.

    For example, when linking an executable against a shared library libprimary that has its own shared dependency libsecondary, we may need to specify the path to libsecondary on the link command using the -rpath/-rpath-link options, it is not sufficient to just say libprimary.

    For us, it’s much easier to just link a static libsecondary into a shared libprimary. Especially because in our case, we are linking against a dummy libprimary anyway that we’ll throw away. We don’t care if the end-user has a static or dynamic libseconday, that’s not our concern. With a static libseconday, when we need to link libprimary into our executable, there’s no dependency chain to worry about, libprimary has all the symbols.

    Perhaps @theuni can add more.

    MarcoFalke commented at 7:40 pm on May 22, 2019:
    This might be helpful to add as a comment.
  13. dongcarl force-pushed on May 22, 2019
  14. dongcarl commented at 8:06 pm on May 22, 2019: member
    Added more detailed justifications to depends/packages.md.
  15. fanquake commented at 9:15 pm on May 22, 2019: member
    Rebooted all the tests.
  16. in depends/packages.md:169 in 27226e6462 outdated 
    163+not sufficient to just say `libprimary`.
164+
165+For us, it's much easier to just link a static `libsecondary` into a shared
166+`libprimary`. Especially because in our case, we are linking against a dummy
167+`libprimary` anyway that we'll throw away. We don't care if the end-user has a
168+static or dynamic `libseconday`, that's not our concern. With a static
    fanquake commented at 6:00 pm on May 23, 2019:
    nit: s/libseconday/libsecondary here and below
    fanquake commented at 10:19 am on May 29, 2019:
    @dongcarl given that you’ve pushed again can you fix this up?
    dongcarl commented at 3:05 pm on May 29, 2019:
    Done! Sorry I missed this.
  17. fanquake commented at 6:00 pm on May 23, 2019: member
    utACK 27226e6
  18. practicalswift commented at 7:08 am on May 24, 2019: contributor
    utACK 27226e64625e3e35edba8f1c55954340362d23b2
  19. jb55 commented at 12:35 pm on May 24, 2019: member
    utACK 27226e64625e3e35edba8f1c55954340362d23b2
  20. MarcoFalke deleted a comment on May 24, 2019
  21. dongcarl force-pushed on May 28, 2019
  22. dongcarl commented at 3:00 pm on May 28, 2019: member

    Update: squashed the two commits.

    Let’s merge this first as #15844 is based on this PR.

  23. MarcoFalke added the label Needs gitian build on May 28, 2019
  24. depends: Build secondary deps statically. 
    Secondary dependencies don't need to be shared.
    3560d8cebf
  25. dongcarl force-pushed on May 29, 2019
  26. theuni commented at 6:07 pm on May 29, 2019: member

    Concept ACK, thanks for picking this up!

    Reviewing.

  27. fanquake commented at 6:14 pm on May 29, 2019: member

    diffoscope x86_64-pc-linux-gnu/lib x86_64-pc-linux-gnu-3560d8c/lib

     0libX11-xcb.so.1.0.0
 1libX11.la
 2libX11.so
 3libX11.so.6
 4libX11.so.6.3.0
 5libXau.a
 6libXau.la
 7+libXext.a
 8libXext.la
 9-libXext.so
10-libXext.so.6
11-libXext.so.6.4.0
12libboost_chrono-mt.a
13libboost_filesystem-mt.a
14libboost_prg_exec_monitor-mt.a
15libboost_system-mt.a
16libboost_test_exec_monitor-mt.a
17libboost_thread-mt.a
18libboost_timer-mt.a
19libboost_unit_test_framework-mt.a
20libcrypto.a
21libdb-4.8.a
22libdb.a
23libdb_cxx-4.8.a
24libdb_cxx.a
25+libdbus-1.a
26libdbus-1.la
27-libdbus-1.so
28-libdbus-1.so.3
29-libdbus-1.so.3.14.11
30libevent.a
31libevent.la
32libevent_core.a
33libevent_core.la
34libevent_extra.a
35libevent_extra.la
36libevent_pthreads.a
37libevent_pthreads.la
38+libexpat.a
39libexpat.la
40-libexpat.so
41-libexpat.so.1
42-libexpat.so.1.6.8
43libfontconfig.la
44libfontconfig.so
45libfontconfig.so.1
46libfontconfig.so.1.9.2
47libfreetype.la
48libfreetype.so
49libfreetype.so.6
  28. theuni commented at 6:21 pm on May 29, 2019: member

    A few things about this make me nervous, so I’m going to look in more depth.

    • Some of these static libs (libXext for example) obviously still work when compiled as non-pic, implying that they never end up linked into anything. In these cases where they’re not used, we may be able to avoid the dependencies altogether. Maybe only headers are needed?
    • I’m nervous about duplicate symbols in static libs where versioning tricks might have otherwise been used for shared libs. Qt, for example, builds internal copies of some of our libs. I can’t come up with any tangible problems though, so this point is very thin.
    • I would’ve expected these changes to require a little more hand-holding for pkg-config. For example, I would have expected this to be necessary.
  29. dongcarl commented at 9:14 pm on May 31, 2019: member

    In this PR we identify 4 secondary deps that should be built statically:

    • dbus
    • expat
    • libXext
    • xtrans

    However, only expat required being built --with-pic for depends builds to succeed, which is rather odd. Therefore, the rest of the deps must not be linked in, or are headers-only.

    Results of my investigation (please verify these as I might be completely wrong):

    libXext

    The only package that we declare as depending on libXext is qt, and it doesn’t actually… Tested by removing the package definition and references to it, everything seems to compile fine.

    xtrans

    It seems that xtrans is just code+headers and not a library that needs to be linked in. We can verify this by looking at the build outputs.

    Its configure is very confused as to why we supply it with --disable-{static,shared}. It is, however, required as a build-time dependency for libX11.

    dbus

    The only reference to dbus is in the configure options for qt: -dbus-runtime. Which indicates to qt to dynamically open libdbus-1 at runtime (with dlopen?). I believe this means that we don’t actually use the dbus we build. (This one I’m the least sure about) @theuni

  30. laanwj referenced this in commit 03858b23fe on Jun 5, 2019
  31. DrahtBot added the label Needs rebase on Jun 5, 2019
  32. DrahtBot commented at 2:48 pm on June 5, 2019: member
  33. fanquake commented at 2:56 pm on June 5, 2019: member
    This has been merged as part of #15844. I’ve opened #16150 to keep track of followup depends changes.
  34. fanquake closed this on Jun 5, 2019
  35. laanwj commented at 3:04 pm on June 5, 2019: member
    Oops. Didn’t realize this was a separate PR and reviewed this as part of #15844.
  36. MarcoFalke reopened this on Jun 5, 2019
  37. MarcoFalke closed this on Jun 5, 2019
  38. MarcoFalke removed the label Needs gitian build on Jun 6, 2019
  39. sidhujag referenced this in commit 676f25849d on Jun 6, 2019
  40. laanwj removed the label Needs rebase on Oct 24, 2019
  41. deadalnix referenced this in commit 4e1ea4c5cb on Apr 1, 2020
  42. ftrader referenced this in commit 076e67d4e2 on Aug 13, 2021
  43. DrahtBot locked this on Dec 16, 2021


dongcarl practicalswift DrahtBot fanquake sipa MarcoFalke jb55 theuni laanwj


theuni

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: 2024-12-19 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me