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

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.
dongcarl added the label Build system on May 17, 2019
dongcarl added the label Needs gitian build on May 17, 2019
dongcarl force-pushed on May 17, 2019
dongcarl marked this as ready for review on May 17, 2019
practicalswift commented at 5:31 PM on May 17, 2019: contributor

Concept ACK
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.
dongcarl force-pushed on May 17, 2019
fanquake commented at 10:19 PM on May 17, 2019: member

Concept ACK
DrahtBot removed the label Needs gitian build on May 18, 2019
dongcarl requested review from theuni on May 20, 2019
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.
dongcarl force-pushed on May 22, 2019
dongcarl commented at 8:06 PM on May 22, 2019: member

Added more detailed justifications to depends/packages.md.
fanquake commented at 9:15 PM on May 22, 2019: member

Rebooted all the tests.

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.

fanquake commented at 6:00 PM on May 23, 2019: member

utACK 27226e6
practicalswift commented at 7:08 AM on May 24, 2019: contributor

utACK 27226e64625e3e35edba8f1c55954340362d23b2
jb55 commented at 12:35 PM on May 24, 2019: member

utACK 27226e64625e3e35edba8f1c55954340362d23b2
MarcoFalke deleted a comment on May 24, 2019
dongcarl force-pushed on May 28, 2019
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.
MarcoFalke added the label Needs gitian build on May 28, 2019
depends: Build secondary deps statically.
Secondary dependencies don't need to be shared.
3560d8cebf
dongcarl force-pushed on May 29, 2019
theuni commented at 6:07 PM on May 29, 2019: member

Concept ACK, thanks for picking this up!

Reviewing.

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

libX11-xcb.so.1.0.0
libX11.la
libX11.so
libX11.so.6
libX11.so.6.3.0
libXau.a
libXau.la
+libXext.a
libXext.la
-libXext.so
-libXext.so.6
-libXext.so.6.4.0
libboost_chrono-mt.a
libboost_filesystem-mt.a
libboost_prg_exec_monitor-mt.a
libboost_system-mt.a
libboost_test_exec_monitor-mt.a
libboost_thread-mt.a
libboost_timer-mt.a
libboost_unit_test_framework-mt.a
libcrypto.a
libdb-4.8.a
libdb.a
libdb_cxx-4.8.a
libdb_cxx.a
+libdbus-1.a
libdbus-1.la
-libdbus-1.so
-libdbus-1.so.3
-libdbus-1.so.3.14.11
libevent.a
libevent.la
libevent_core.a
libevent_core.la
libevent_extra.a
libevent_extra.la
libevent_pthreads.a
libevent_pthreads.la
+libexpat.a
libexpat.la
-libexpat.so
-libexpat.so.1
-libexpat.so.1.6.8
libfontconfig.la
libfontconfig.so
libfontconfig.so.1
libfontconfig.so.1.9.2
libfreetype.la
libfreetype.so
libfreetype.so.6

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.
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
laanwj referenced this in commit 03858b23fe on Jun 5, 2019
DrahtBot added the label Needs rebase on Jun 5, 2019
DrahtBot commented at 2:48 PM on June 5, 2019: member

Needs rebase
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.
fanquake closed this on Jun 5, 2019
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.
MarcoFalke reopened this on Jun 5, 2019
MarcoFalke closed this on Jun 5, 2019
MarcoFalke removed the label Needs gitian build on Jun 6, 2019
sidhujag referenced this in commit 676f25849d on Jun 6, 2019
laanwj removed the label Needs rebase on Oct 24, 2019
deadalnix referenced this in commit 4e1ea4c5cb on Apr 1, 2020
ftrader referenced this in commit 076e67d4e2 on Aug 13, 2021
DrahtBot locked this on Dec 16, 2021