depends: Prune X packages

dongcarl commented at 9:13 pm on July 17, 2019: member

Related to: #16150

We noticed that we could build QT without using XLib/libX11 as a library. XLib/libX11’s headers are still used, and a minimal configure.ac has been added to eliminate overly-enthusiastic configure-time dependencies that aren’t actually required to obtain the headers.

This also means that we eliminate XLib/libX11 as required shared libraries at runtime, which is desirable.

See commit messages for more details.

Reviewers: I am least sure about the minimal configure.ac, as I’m not too familiar with the autoconf syntax. Any improvements w/re robustness would be welcome.

depends: xproto is only directly needed by libXau 1ec30b8fbe

depends: qt: Explicitly stop using Xlib/libX11

Previously, in 683b7d7a3fc1b9240333faf3d04497aa61583016 and
0e752637a26cf75187864a466db9a92540a2d3c8, we accidentally broke QT's
ability to pick up Xlib thru the config.gui.tests.xlib configuration
test, which also means that config.gui.libraries.xcb_xlib wasn't run.

This resulted in a QT build that was implicitly -no-xcb-lib and
-no-feature-xlib.

This is actually a desired behaviour, as it means less required shared
objects for our final bitcoin-qt binary. Specifically, it eliminated the
libX11-xcb.so.1 and libX11.so.6 requirements.

In this commit, we explicitly build without Xlib. We should continue to
track upstream ticket https://bugreports.qt.io/browse/QTBUG-61452 which
talks about adding a -no-xlib (non-hidden) flag instead of the
-no-feature-xlib (hidden) flag.

9a01ab04e1

depends: libX11: Make package headers-only

We're no longer building QT with libX11/XLib, however, libX11/XLib
headers are still required for parts of QT. In this commit we add a
minimal configure.ac for libX11/XLib that is headers-only.

This change allows us to remove all of libX11/XLib's dependencies.

aa53cb7a2f

build-aux: Remove check for x11-xcb

We're no longer building QT with libX11/XLib, so it doesn't make sense
to check for the x11-xcb package.

689d3b4a03

depends: libXext isn't needed by anyone

libXext was only needed (as a library) by QT when it was using
XLib/libX11 (as a library), now that we're building QT without
XLib/libX11, we can safely remove libXext.

924569914e

symbol-check: Disallow libX11-*.so.* shared libraries

They should no longer be needed as we build QT without libX11/XLib
libraries now.

65f8da08df

dongcarl added the label Build system on Jul 17, 2019

dongcarl added the label Needs gitian build on Jul 17, 2019

dongcarl requested review from theuni on Jul 17, 2019

dongcarl requested review from fanquake on Jul 17, 2019

DrahtBot commented at 0:31 am on July 18, 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:

#16370 (depends: cleanup package configure flags by fanquake)
#14066 (gitian-linux: Build binaries for 64-bit POWER by luke-jr)

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.

fanquake commented at 2:12 am on July 18, 2019: member

Concept ACK. Assuming this works without breaking anything else it’s a good cleanup & simplification to depends and makes the behavior we want explicit. Will review shortly.

In https://github.com/bitcoin/bitcoin/pull/16408/commits/aa53cb7a2f04a59a4722c662e67b7a6ec04e32b5 you say:

however, libX11/XLib headers are still required for parts of QT.

Is what it’s required for documented somewhere?

There was some related discussion in the #bitcoin-builds channel. Logs here.

laanwj commented at 11:19 am on July 18, 2019: member

As I have always understood it, XCB is a newer, more efficient replacement for the X11 protocol. In that case what you’d lose here is support for pre-XCB servers. XCB is old as the hills by now (initial release 2001 according to wikipedia ), so Concept ACK.

dongcarl commented at 5:43 pm on July 18, 2019: member

Some primary source information on XCB vs libX11/XLib: https://www.x.org/wiki/guide/xlib-and-xcb/

XCB isn’t a replacement for the X11 protocol, but rather a replacement for the helper library used to talk to the X server (previously libX11/XLib).

Quote about libX11/XLib:

It was designed to look like a traditional library API, hiding the fact that calls result in protocol requests to a server. Calls that don’t require a response from the X server are queued in a buffer to be sent as a batch of requests to the server. Those that require a response flush all the buffered requests and then block until the response is received.

Quote about XCB:

XCB on the other hand, provides functions generated directly from the protocol descriptions in an “obvious” mechanistic way. XCB functions map directly onto the protocol, with separate functions to put requests into the outgoing buffer and to read results back from the X server asynchronously later.

Note that we already have XCB as a required library: https://github.com/bitcoin/bitcoin/blob/e5abb59a9a66723dd1d9a01f65e467636eb24f2d/contrib/devtools/symbol-check.py#L72

dongcarl commented at 5:53 pm on July 18, 2019: member

@fanquake

however, libX11/XLib headers are still required for parts of QT.

Is what it’s required for documented somewhere?

Unfortunately, it’s not documented anywhere, but looking at the code for QT, the following files from libX11/Xlib are still being included:

0X11/cursorfont.h
1X11/Xlib.h
2X11/Xlibint.h
3X11/Xlib-xcb.h
4X11/Xutil.h

theuni commented at 6:55 pm on July 18, 2019: member

Concept ACK. I definitely tried to do this originally, but it simply wasn’t possible with older qt versions. I believe they still required a few of the synchronous libX11 calls, but apparently that’s no longer the case. Great find, @dongcarl!

The need for headers is worrisome. While likely harmless, it could also be the case that some libX11 symbols are linked in and remain unresolved, with the linker managing to figure out that they’re dead paths. Completely unlikely, though. @dongcarl: Assuming the likely scenario that this is just an upstream qt bug, how involved would it be to add the proper ifdefs in their code? If it’s not TOO nasty, I’d prefer to carry+upstream that patch than to require unnecessary headers.

gitignore: Actually pay attention to depends patches

There was a previous attempt to achieve this, and it was bad. This
works.

222e6cc520

depends: qt: Patch to remove dep on libX11

We can actually patch QT to remove its dependency on libX11's headers.
It turns it this wasn't that hard.

0c55d8b581

dongcarl commented at 9:45 pm on July 18, 2019: member

@theuni Is 0c55d8b58186ba69fffc147cd02b174450dac578 what you mean? If so, I’ll look into upstreaming. This seems to work and eliminates libX11/Xlib :tada:

theuni commented at 9:52 pm on July 18, 2019: member

@dongcarl wooo, yes! Need to look in-detail, but that’s awesome, and the final patch should definitely be upstreamed.

in build-aux/m4/bitcoin_qt.m4:358 in 0c55d8b581

354@@ -355,7 +355,6 @@ AC_DEFUN([_BITCOIN_QT_FIND_STATIC_PLUGINS],[
355          PKG_CHECK_MODULES([QTFB], [Qt5FbSupport], [QT_LIBS="-lQt5FbSupport $QT_LIBS"])
356                 fi
357        if test "x$TARGET_OS" = xlinux; then
358-         PKG_CHECK_MODULES([X11XCB], [x11-xcb], [QT_LIBS="$X11XCB_LIBS $QT_LIBS"])

theuni commented at 10:32 pm on July 18, 2019:

We can’t just remove this, as it should still be necessary for non-depends builds. In other words, whatever we change this to needs to work before AND after removing the dependencies.

I think it should suffice to simply continue if the X11XCB check fails, and assume that qt was built without it as a dependency.

More correctly, we may be able to test a qt feature flag in one of the .pc files, or compile a stub that makes sure that QT_CONFIG(xcb_xlib) isn’t set if libX11 is missing.

dongcarl commented at 3:33 pm on July 19, 2019:

I think it should suffice to simply continue if the X11XCB check fails, and assume that qt was built without it as a dependency.

Played around with this a little bit…

Adding an empty last argument seems to trigger the default action-if-not-found behaviour of “end the execution with an error for not having found the dependency” (source)

0PKG_CHECK_MODULES([X11XCB], [x11-xcb], [QT_LIBS="$X11XCB_LIBS $QT_LIBS"], [])

Adding a dummy last argument seems to work

0PKG_CHECK_MODULES([X11XCB], [x11-xcb], [QT_LIBS="$X11XCB_LIBS $QT_LIBS"], [QT_LIBS="$QT_LIBS"])

I don’t know autotools that well so perhaps @theuni can tell me what the idiomatic thing to do is here.

theuni commented at 6:34 pm on July 22, 2019:

I just looked into this. The .pc file for QTXCBQPA adds “-lX11-xcb” as necessary, so our pkg-config check is redundant anyway (unless it was done to fix an ordering problem, which doesn’t seem to be the case).

So nevermind my complaint above, removal is fine.

dongcarl commented at 6:47 pm on July 22, 2019:

I double-checked @theuni’s result with a Gitian build of 76e2cded477bc483ec610212bdadf21fe35292d4, which was before any XLib pickup failure.

Here is depends/i686-pc-linux-gnu/lib/pkgconfig/Qt5XcbQpa.pc:

 0ubuntu@a661ab7477ba:~/build/bitcoin/depends/i686-pc-linux-gnu/lib/pkgconfig$ cat Qt5XcbQpa.pc
 1prefix=/home/ubuntu/build/bitcoin/depends/i686-pc-linux-gnu
 2exec_prefix=${prefix}
 3libdir=${prefix}/lib
 4includedir=${prefix}/include
 5
 6
 7Name: Qt5 XcbQpa
 8Description: Qt XcbQpa module
 9Version: 5.9.7
10Libs: -L${libdir} -lQt5XcbQpa
11Libs.private: -L/home/ubuntu/build/bitcoin/depends/i686-pc-linux-gnu/lib -lQt5ServiceSupport -lQt5ThemeSupport -lQt5DBus -lQt5EventDispatcherSupport -lQt5FontDatabaseSupport -L//home/ubuntu/build/bitcoin/depends/i686-pc-linux-gnu/lib -lfontconfig -lfreetype -lQt5Gui -lqtlibpng -lqtharfbuzz -lQt5Core -lm -lz -lqtpcre2 -lpthread -lX11-xcb -lX11 -lxcb-static -lxcb -ldl
12Cflags: -I${includedir}/QtXcbQpa
13Requires: Qt5Core Qt5Gui Qt5Core Qt5Gui Qt5ServiceSupport Qt5ThemeSupport Qt5EventDispatcherSupport Qt5FontDatabaseSupport

So, we should be good.

theuni changes_requested

theuni commented at 10:36 pm on July 18, 2019: member

The patch and depends changes look good. At first glance it looks like there are a few cleanups to make to the patch, but after looking more deeply I agree with what you have here.

Looks good to me after cleaning up the qt detection.

dongcarl commented at 3:35 pm on July 19, 2019: member

For the record, here’s the QT upstream tracking for my changes: https://codereview.qt-project.org/c/qt/qtbase/+/268206

practicalswift commented at 11:06 am on July 22, 2019: contributor

Concept ACK

Very nice find!

theuni approved

theuni commented at 6:36 pm on July 22, 2019: member

ACK 0c55d8b58186ba69fffc147cd02b174450dac578

dongcarl commented at 7:20 pm on July 22, 2019: member

Perhaps it’s good to preserve the history here. Please let me know if people think this patchset needs squashing.

fanquake approved

fanquake commented at 7:06 am on July 23, 2019: member

ACK 0c55d8b58186ba69fffc147cd02b174450dac578

Please let me know if people think this patchset needs squashing.

I think these commits are ok as-is. Good explanations for each.

I did some testing on macOS and Debian. Checked that XLib is being disabled when building Qt:

0 X11:
1    Using system-provided XCB libraries .. no
2    EGL on X11 ........................... no
3    Xinput2 .............................. no
4    XCB XKB .............................. yes
5    XLib ................................. no
6    XCB render ........................... yes
7    XCB GLX .............................. yes
8    XCB Xlib ............................. no
9    Using system-provided xkbcommon ...... no

Checked that bitcoin-qt on Debian actually ran.

Overall a really nice cleanup. Going to merge this now so that the build system work can continue.

fanquake removed the label Needs gitian build on Jul 23, 2019

fanquake referenced this in commit e6e99d4f75 on Jul 23, 2019

fanquake merged this on Jul 23, 2019

fanquake closed this on Jul 23, 2019

deadalnix referenced this in commit 53d3336696 on Apr 2, 2020

xdustinface referenced this in commit ce6ddadd15 on Feb 18, 2021

xdustinface referenced this in commit 086bb0bae8 on Feb 18, 2021

furszy referenced this in commit e2085e57f7 on Mar 31, 2021

gades referenced this in commit 4f931f3bea on Jun 25, 2021

ftrader referenced this in commit 66c8e65678 on Aug 13, 2021

DrahtBot locked this on Dec 16, 2021

depends: Prune X packages #16408

Conflicts