depends: Split libpng out of Qt #19751
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fanquake
reviewed
Aug 18, 2020
|
As https://bugreports.qt.io/browse/QTBUG-66388 seems resolved for Qt 5.12 could this change be skipped, and building PPC64 binaries postponed? |
|
They've already been postponed an annoyingly long time. Using upstream libpng is better anyway - the one bundled with Qt is at best a slower "bare minimum". |
Concept ACK. |
cdb0a09 to
9637970
Compare
|
Applied the proposed changes |
hebasto
reviewed
Aug 18, 2020
9637970 to
8d8d3f1
Compare
|
|
hebasto
approved these changes
Aug 19, 2020
There was a problem hiding this comment.
ACK 8d8d3f1, tested on Linux Mint 20 (x86_64):
- verified version and known vulnerabilities (http://libpng.org/pub/png/libpng.html)
- verified the download link and the downloaded archive hash
- locally built
libpng16.a, verified configure options and output
Gitian builds:
4995b8f64c966d17d9aa8db328ad32e07fc1c9d411c611a236d4129eb3b40608 bitcoin-8d8d3f139673-aarch64-linux-gnu-debug.tar.gz
f9e26ef0e9482ce08f8b5c55d42db23d3c8f428222311901fcdbfa972d4c1972 bitcoin-8d8d3f139673-aarch64-linux-gnu.tar.gz
36dcb911729493e70123a4d4a826ea2e1744e57fa34d43b2bb469764985363e5 bitcoin-8d8d3f139673-arm-linux-gnueabihf-debug.tar.gz
0074ce82fff383930bc426ca17cb49d1dcb8a2f5080186eb9d0a9a3ecd39bc33 bitcoin-8d8d3f139673-arm-linux-gnueabihf.tar.gz
328e5756d14428a5abe357ccfe67ed998d3bf78c238e34d9e71b2d40eb8be7e4 bitcoin-8d8d3f139673-riscv64-linux-gnu-debug.tar.gz
2a557d53a2b7efd88619318241460f7b9df0d25df072a9e746352c4095bc624b bitcoin-8d8d3f139673-riscv64-linux-gnu.tar.gz
f5a099afb3b7bc2a90f1b3b32258242a89fc286c171546d305bc8f7eb3d3610b bitcoin-8d8d3f139673-x86_64-linux-gnu-debug.tar.gz
660c86f13069dd7f189e1c5110fbeda53657a0a220b5ee6b76328ef0a283ef71 bitcoin-8d8d3f139673-x86_64-linux-gnu.tar.gz
b1d19a481ea6cc8279d8cc184139992fd3469b6be1bcf86576752aaf11d987c8 bitcoin-8d8d3f139673-win-unsigned.tar.gz
59127b06b566b243664ef5699a78c54841fc81b4122a046b1332b250dee081f2 bitcoin-8d8d3f139673-win64-debug.zip
7821e41feeaf713083132129eda25040d75e8d2c36229422854b0986efb5c2eb bitcoin-8d8d3f139673-win64-setup-unsigned.exe
b620958be279680e259ed1ca366f98f7a2880c32fdd42e9645efe44c8443e69d bitcoin-8d8d3f139673-win64.zip
5a46200cfb7e527d5b75ba50e615bff13ec76aa7022fc32978ab8baa9c160d2c bitcoin-8d8d3f139673-osx-unsigned.dmg
ce3790234531e8013471d1b7bd3c57df3c6474c4ef61d0cbb3744ff13553ebc6 bitcoin-8d8d3f139673-osx-unsigned.tar.gz
1053e6776209260d58d695543bad261690c1b71b90d52e96588e68d17c972cf7 bitcoin-8d8d3f139673-osx64.tar.gz
88224654414478c5602342e14ee8aedcbf31f7416d8e914b750314e062de756e src/bitcoin-8d8d3f139673.tar.gz
Tested bitcoin-qt gitian binaries on:
- Linux Mint 20 (x86_64)
- Windows 10 2004 (build 19041.450)
- macOS 10.15.6 (19G2021)
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
This was referenced Aug 20, 2020
Closed
Gitian builds
|
|
I slightly prefer using Qt's internal libpng if possible. We don't use libpng directly in any way only through Qt, and having to download and build less dependencies is good. But no strong opinion. |
|
Agree with @laanwj generally, no strong preference. The history here is that we've generally only ever self-built libs needed internally by qt if qt is the only user. zlib, for example, is used by a few things, so we built it ourselves rather than using qt's bundled copy. That said, if there's good reason for us to build it ourselves, I don't think there's any reason not to. And working around a stale/incompatible bundled libpng seems like decent reason. So, concept ACK. |
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
Sep 15, 2020
This is an alternative to bitcoin#19751 that fixes the build without requiring splitting out libpng. This patch can be dropped once we are building qt 5.12.0 or later.
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
Sep 15, 2020
This is an alternative to bitcoin#19751 that fixes the build without requiring splitting out libpng. This patch can be dropped once we are building qt 5.12.0 or later.
fanquake
added a commit
to fanquake/bitcoin
that referenced
this pull request
Sep 15, 2020
This is an alternative to bitcoin#19751 that fixes the build without requiring splitting out libpng. This patch can be dropped once we are building qt 5.12.0 or later.
|
I've posted an alternative to this PR in #19959. I think instead of splitting out libpng, we could just apply the upstream patch. |
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Sep 22, 2020
f07fb5a build: patch qt libpng to fix powerpc build (fanquake) Pull request description: This is an alternative to #19751 that fixes the build without requiring splitting out libpng. This patch can be dropped once we are building qt 5.12.0 or later. One of the [concerns posted in #19751](bitcoin/bitcoin#19751 (comment)) was: > the one bundled with Qt is at best a slower "bare minimum". However for our usage, I don't think the performance of libpng is a concern. If it is, I'd like to see some numbers/justification as to why we should be worried about it. This patch should be enough to unblock PowerPC binaries (combined with other changes) for 0.21.0, and for 0.22.0, when we switch to qt 5.15.x in depends, we should be able to drop it. Related upstream issue: https://bugreports.qt.io/browse/QTBUG-66388. ACKs for top commit: laanwj: ACK f07fb5a theuni: ACK f07fb5a. hebasto: ACK f07fb5a, the patch is the same as https://codereview.qt-project.org/gitweb?p=qt/qtbase.git;a=blobdiff;f=src/3rdparty/libpng/libpng.pro;h=a2f56669b47e09629b7a88a0964091404d6a9b06;hp=577b61d833f7842f3f0d6b1c94a3b3920006695c;hb=dc2aead842f4cdf74f9259d3606c53c8bdae2c6b;hpb=ceeecbae510af6e2d1ebbf865761e4761d404033 Tree-SHA512: 865b843f5049eca80215774274fb7ae0dacccc3dd7f4a2eec93a73057115dcea85e715f919f96441424f9dd902dd97f0a238d96d4074babcee66b30577104009
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 23, 2020
f07fb5a build: patch qt libpng to fix powerpc build (fanquake) Pull request description: This is an alternative to bitcoin#19751 that fixes the build without requiring splitting out libpng. This patch can be dropped once we are building qt 5.12.0 or later. One of the [concerns posted in bitcoin#19751](bitcoin#19751 (comment)) was: > the one bundled with Qt is at best a slower "bare minimum". However for our usage, I don't think the performance of libpng is a concern. If it is, I'd like to see some numbers/justification as to why we should be worried about it. This patch should be enough to unblock PowerPC binaries (combined with other changes) for 0.21.0, and for 0.22.0, when we switch to qt 5.15.x in depends, we should be able to drop it. Related upstream issue: https://bugreports.qt.io/browse/QTBUG-66388. ACKs for top commit: laanwj: ACK f07fb5a theuni: ACK f07fb5a. hebasto: ACK f07fb5a, the patch is the same as https://codereview.qt-project.org/gitweb?p=qt/qtbase.git;a=blobdiff;f=src/3rdparty/libpng/libpng.pro;h=a2f56669b47e09629b7a88a0964091404d6a9b06;hp=577b61d833f7842f3f0d6b1c94a3b3920006695c;hb=dc2aead842f4cdf74f9259d3606c53c8bdae2c6b;hpb=ceeecbae510af6e2d1ebbf865761e4761d404033 Tree-SHA512: 865b843f5049eca80215774274fb7ae0dacccc3dd7f4a2eec93a73057115dcea85e715f919f96441424f9dd902dd97f0a238d96d4074babcee66b30577104009
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just using upstream libpng from #14066
This should be testable/reviewable without a PPC64 machine (and once merged should avoid needing to rebuild Qt for #14066).