depends: Remove Qt build-time dependencies #29923
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
laanwj
force-pushed
the
2024-04-qtsowrap
branch
3 times, most recently
from
534b230
to
6cd5e8a
Compare
|
Instead of Qt's dependencies, maybe we should wrap Qt itself this way? Or do C++ templates break that too much? |
This is basically not possible with a C++ library. It is thanks to C's lack of name mangling and its straightforward linkage model that makes this possible. Also Qt has a ton of symbols compared to these libraries. And even if you narrow it down to only the used symbols, it's much harder to keep track of. Conceptually, a few low-level X11 (or in the future, Wayland) and font libraries are much more likely to be on the user's system than a compatible version of Qt. That will only become worse with Qt 5 versus 6. This PR, as is, doesn't change the needed libraries, only how they're imported. |
|
One important thing to test here is that the resulting There's no inherent reason to assume this will make compatibility worse, but it currently loads all symbols from the headers for the respective libraries (https://github.com/laanwj/qtsowrap?tab=readme-ov-file#versions) which might enforce the lower bound on version stronger than used to be done. Might consider downgrading the headers, if this turns out to be a problem. Or add a list in the descriptor per library of the functions actually used. But this is more work to maintain than a list of version numbers. |
|
Ubuntu 22.04 has the following version of libxcb-xfixes: 1.14. That's kind of strange! That's the same one we have. Edit: that was it! Ubuntu 22.04 now works. |
Guix builds (on x86_64)
|
|
i've added some review notes for the Qt patch here: https://github.com/laanwj/qtsowrap?tab=readme-ov-file#patching-qt What is important to ensure is that |
laanwj
force-pushed
the
2024-04-qtsowrap
branch
from
39e891d
to
eb60c77
Compare
laanwj
force-pushed
the
2024-04-qtsowrap
branch
from
8bb49c6
to
7784a09
Compare
|
Rebased for merge conflict (trivial: only modify/deletes in packages that go away) and to apply fixup commits. |
laanwj
force-pushed
the
2024-04-qtsowrap
branch
from
7784a09
to
3f2c26b
Compare
|
Rebased for #29985 (add/add). |
This was required by xkbcommon build, which is no longer a dependency for build.
|
All outstanding TODOs were done. Rebased to master, collected fixups. |
There was a problem hiding this comment.
Concept ACK. This looks like amazing work, great!
I'm not sure how to help test. Using arm64 macOS 14.4.1, this branch builds cleanly, the unit tests pass, and the GUI runs fine. Don't see any issues on first read of the code.
$ clang --version
Homebrew clang version 18.1.5
Target: arm64-apple-darwin23.4.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
Thanks for testing! To be clear, this shouldn't affect MacOS at all, Qt on MacOS (and Windows) doesn't have any of the huge bag of dependencies that Qt Linux/UNIX has. |
|
I did test an earlier draft of this pull request and the only thing I noticed was that on a fresh desktop install of a Linux distro, sometimes a package would be missing, and the GUI would fail to start. If this is expected, I guess the docs should be adjusted to clarify that users would have to install the packages themselves? |
Do you have specific output or details? That isn't expected behavior. This PR doesn't change the libraries required to be installed on the user's system, all the dependencies that this removes were already built only to link against. The only difference is that they're loaded at runtime using One advantage is that this allows to be more tolerant about which libraries are installed on the user's system, by having control over the loading, not less. |
The DrahtBot builds (as well as the log outputs) are deleted by now, as they are short-lived test-only assets. I'll create fresh DrahtBot builds and try again with the current state of this pull request. |
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]
|
In the spirit of the halving, let's (approximately) halve the number of packages in depends. Remove the following packages:
This is done by loading all of Qt's system dependencies at run-time, replacing them with an auto-generated wrapper library
qtsowrap:See https://github.com/laanwj/qtsowrap for details.
After this,
bitcoin-qthas the same direct linking dependencies asbitcoindand other binaries:(libdl.so.X needed too depending on the libc version)
For comparison,
bitcoin-qtfrom release 27.0 has the following:This does not affect Windows or MacOS, which already do not have this problem of having to build half the OS in the dependency chain.
This will make it possible to add Wayland support to the release binaries without requiring the Wayland (or X) libraries to be installed on every system that runs them.
Reviewing/Testing
bitcoin-qtbinaries work on a range of different Linux distributions (also older ones that people still care about).initialize_happens before any calls to a library, as calling early will cause a crash. Forgetting to wrap a header will be found during linking, as the symbol will be wrong, so it's not a review concern.TODOs (all done)
(before merge, probably)