depends: Prune X packages #16408

dongcarl · 2019-07-17T21:13:54Z

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.

Previously, in 683b7d7 and 0e75263, 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.

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.

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

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.

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

DrahtBot · 2019-07-18T00:31:49Z

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 · 2019-07-18T02:12:33Z

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 aa53cb7 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 · 2019-07-18T11:19:23Z

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 · 2019-07-18T17:43:20Z

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:

bitcoin/contrib/devtools/symbol-check.py

Line 72 in e5abb59

'libxcb.so.1', # part of X11

dongcarl · 2019-07-18T17:53:06Z

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

X11/cursorfont.h
X11/Xlib.h
X11/Xlibint.h
X11/Xlib-xcb.h
X11/Xutil.h

theuni · 2019-07-18T18:55:02Z

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.

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

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

dongcarl · 2019-07-18T21:45:24Z

@theuni Is 0c55d8b what you mean? If so, I'll look into upstreaming. This seems to work and eliminates libX11/Xlib 🎉

theuni · 2019-07-18T21:52:57Z

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

theuni

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.

theuni · 2019-07-18T22:32:54Z

build-aux/m4/bitcoin_qt.m4

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


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.

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)

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

Adding a dummy last argument seems to work

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

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.

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

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

ubuntu@a661ab7477ba:~/build/bitcoin/depends/i686-pc-linux-gnu/lib/pkgconfig$ cat Qt5XcbQpa.pc prefix=/home/ubuntu/build/bitcoin/depends/i686-pc-linux-gnu exec_prefix=${prefix} libdir=${prefix}/lib includedir=${prefix}/include Name: Qt5 XcbQpa Description: Qt XcbQpa module Version: 5.9.7 Libs: -L${libdir} -lQt5XcbQpa Libs.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 Cflags: -I${includedir}/QtXcbQpa Requires: Qt5Core Qt5Gui Qt5Core Qt5Gui Qt5ServiceSupport Qt5ThemeSupport Qt5EventDispatcherSupport Qt5FontDatabaseSupport

So, we should be good.

dongcarl · 2019-07-19T15:35:35Z

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

practicalswift · 2019-07-22T11:06:24Z

Concept ACK

Very nice find!

theuni

ACK 0c55d8b

theuni · 2019-07-22T18:34:48Z

build-aux/m4/bitcoin_qt.m4

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


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 · 2019-07-22T19:20:44Z

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

fanquake

ACK 0c55d8b

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:

 X11:
    Using system-provided XCB libraries .. no
    EGL on X11 ........................... no
    Xinput2 .............................. no
    XCB XKB .............................. yes
    XLib ................................. no
    XCB render ........................... yes
    XCB GLX .............................. yes
    XCB Xlib ............................. no
    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.

0c55d8b depends: qt: Patch to remove dep on libX11 (Carl Dong) 222e6cc gitignore: Actually pay attention to depends patches (Carl Dong) 65f8da0 symbol-check: Disallow libX11-*.so.* shared libraries (Carl Dong) 9245699 depends: libXext isn't needed by anyone (Carl Dong) 689d3b4 build-aux: Remove check for x11-xcb (Carl Dong) aa53cb7 depends: libX11: Make package headers-only (Carl Dong) 9a01ab0 depends: qt: Explicitly stop using Xlib/libX11 (Carl Dong) 1ec30b8 depends: xproto is only directly needed by libXau (Carl Dong) Pull request description: Related to: bitcoin#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. ACKs for top commit: theuni: ACK 0c55d8b fanquake: ACK 0c55d8b Tree-SHA512: 41f653a0f91bc0e0faac49713c0c6dfd8cb605f9c4e34eb75a790dd808ebf3e5c160f1dd40bc8fbc911ee718ea319313b526d63733c98ff62d8dffecb58caa01

Summary: ``` 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. ``` Backport of core [[bitcoin/bitcoin#16408 | PR16408]]. Depends on D5632. Test Plan: Run the Gitian builds. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5636

0c55d8b depends: qt: Patch to remove dep on libX11 (Carl Dong) 222e6cc gitignore: Actually pay attention to depends patches (Carl Dong) 65f8da0 symbol-check: Disallow libX11-*.so.* shared libraries (Carl Dong) 9245699 depends: libXext isn't needed by anyone (Carl Dong) 689d3b4 build-aux: Remove check for x11-xcb (Carl Dong) aa53cb7 depends: libX11: Make package headers-only (Carl Dong) 9a01ab0 depends: qt: Explicitly stop using Xlib/libX11 (Carl Dong) 1ec30b8 depends: xproto is only directly needed by libXau (Carl Dong) Pull request description: Related to: bitcoin#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. ACKs for top commit: theuni: ACK 0c55d8b fanquake: ACK 0c55d8b Tree-SHA512: 41f653a0f91bc0e0faac49713c0c6dfd8cb605f9c4e34eb75a790dd808ebf3e5c160f1dd40bc8fbc911ee718ea319313b526d63733c98ff62d8dffecb58caa01

67c6c5a depends: qt: Patch to remove dep on libX11 (Carl Dong) 4cfd7ff gitignore: Actually pay attention to depends patches (Carl Dong) 3bd798a symbol-check: Disallow libX11-*.so.* shared libraries (Carl Dong) a214141 depends: libXext isn't needed by anyone (Carl Dong) adf5772 build-aux: Remove check for x11-xcb (Carl Dong) bc7bc6a depends: libX11: Make package headers-only (Carl Dong) 4029218 depends: qt: Explicitly stop using Xlib/libX11 (Carl Dong) 6007eb6 depends: xproto is only directly needed by libXau (Carl Dong) Pull request description: Coming straight from bitcoin#16408 These X packages are not necessary to build our Qt dependency (Qt required them in earlier versions), so remove them. ACKs for top commit: random-zebra: utACK 67c6c5a furszy: utACK 67c6c5a Tree-SHA512: 30c078cf3c27e8dd26153e96456143dff434b0f3c814ab0a04eb97d49e2ceb5ed396c33b3592c82f2ed6bbe43d5891e3025748413267f511de87bb195d620d4f

0c55d8b depends: qt: Patch to remove dep on libX11 (Carl Dong) 222e6cc gitignore: Actually pay attention to depends patches (Carl Dong) 65f8da0 symbol-check: Disallow libX11-*.so.* shared libraries (Carl Dong) 9245699 depends: libXext isn't needed by anyone (Carl Dong) 689d3b4 build-aux: Remove check for x11-xcb (Carl Dong) aa53cb7 depends: libX11: Make package headers-only (Carl Dong) 9a01ab0 depends: qt: Explicitly stop using Xlib/libX11 (Carl Dong) 1ec30b8 depends: xproto is only directly needed by libXau (Carl Dong) Pull request description: Related to: bitcoin#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. ACKs for top commit: theuni: ACK 0c55d8b fanquake: ACK 0c55d8b Tree-SHA512: 41f653a0f91bc0e0faac49713c0c6dfd8cb605f9c4e34eb75a790dd808ebf3e5c160f1dd40bc8fbc911ee718ea319313b526d63733c98ff62d8dffecb58caa01

Summary: ``` 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. ``` Backport of core [[bitcoin/bitcoin#16408 | PR16408]]. Depends on D5632. Test Plan: Run the Gitian builds. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5636

dongcarl added 6 commits July 17, 2019 17:04

depends: xproto is only directly needed by libXau

1ec30b8

build-aux: Remove check for x11-xcb

689d3b4

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

depends: libXext isn't needed by anyone

9245699

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.

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

65f8da0

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

dongcarl added Build system Needs gitian build labels Jul 17, 2019

dongcarl requested review from theuni and fanquake July 17, 2019 21:14

dongcarl added 2 commits July 18, 2019 17:41

gitignore: Actually pay attention to depends patches

222e6cc

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

depends: qt: Patch to remove dep on libX11

0c55d8b

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

theuni suggested changes Jul 18, 2019

View reviewed changes

theuni approved these changes Jul 22, 2019

View reviewed changes

fanquake approved these changes Jul 23, 2019

View reviewed changes

fanquake removed the Needs gitian build label Jul 23, 2019

fanquake merged commit 0c55d8b into bitcoin:master Jul 23, 2019

fanquake mentioned this pull request Jul 23, 2019

build: packages that may be prunable from depends #16150

Closed

This was referenced Jul 23, 2019

depends: disable unused Qt features #16386

Merged

depends: cleanup package configure flags #16370

Merged

UdjinM6 mentioned this pull request Feb 18, 2021

depends|build: Make sure configuration detects Qt built with DEBUG=1 dashpay/dash#4005

Merged

Fuzzbawls mentioned this pull request Mar 24, 2021

[Depends] Prune un-necessary X packages PIVX-Project/PIVX#2263

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

depends: Prune X packages #16408

depends: Prune X packages #16408

Uh oh!

Conversation

dongcarl commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 18, 2019

Conflicts

Uh oh!

fanquake commented Jul 18, 2019

Uh oh!

laanwj commented Jul 18, 2019

Uh oh!

dongcarl commented Jul 18, 2019

Uh oh!

dongcarl commented Jul 18, 2019

Uh oh!

theuni commented Jul 18, 2019

Uh oh!

dongcarl commented Jul 18, 2019

Uh oh!

theuni commented Jul 18, 2019

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

theuni Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

dongcarl Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

theuni Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

dongcarl Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Jul 19, 2019

Uh oh!

practicalswift commented Jul 22, 2019

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

theuni Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

dongcarl commented Jul 22, 2019

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongcarl commented Jul 17, 2019 •

edited

Loading