depends: Split libpng out of Qt

luke-jr commented at 5:24 pm on August 17, 2020: member

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

MarcoFalke added the label Needs gitian build on Aug 17, 2020

MarcoFalke added the label Needs Guix build on Aug 17, 2020

MarcoFalke added the label Build system on Aug 17, 2020

in depends/packages/libpng.mk:2 in cdb0a09e1b outdated

0@@ -0,0 +1,23 @@
1+package=libpng
2+$(package)_version=1.6.34

fanquake commented at 4:23 am on August 18, 2020:

If we are going to add this to depends, we should at least add the most recent release (1.6.37); unless there’s some reason not to, in which case, we should document why.

in depends/packages/libpng.mk:3 in cdb0a09e1b outdated

0@@ -0,0 +1,23 @@
1+package=libpng
2+$(package)_version=1.6.34
3+$(package)_download_path=http://ftp.osuosl.org/pub/libpng/src/libpng16/

fanquake commented at 4:27 am on August 18, 2020:

I’d rather use https://downloads.sourceforge.net/project/libpng/libpng16/1.6.37/libpng-1.6.37.tar.xz as the URL. Was this was a mirror you were redirected to, or were you trying to avoid SourceForge? I can’t see this URL anywhere on http://www.libpng.org/pub/png/libpng.html.

in doc/dependencies.md:17 in cdb0a09e1b outdated

13@@ -14,7 +14,7 @@ These are the dependencies currently used by Bitcoin Core. You can find instruct
14 | GCC |  | [4.8+](https://gcc.gnu.org/) (C++11 support) |  |  |  |
15 | HarfBuzz-NG |  |  |  |  | [Yes](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/qt.mk) |
16 | libevent | [2.1.11-stable](https://github.com/libevent/libevent/releases) | [2.0.21](https://github.com/bitcoin/bitcoin/pull/18676) | No |  |  |
17-| libpng |  |  |  |  | [Yes](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/qt.mk) |
18+| libpng |  |  |  |  | No |

fanquake commented at 4:34 am on August 18, 2020:

Any reason you haven’t filled in Version used and CVEs here? CVE-2019-7317 applies to libpng prior to 1.6.37.

in depends/packages/libpng.mk:9 in cdb0a09e1b outdated

0@@ -0,0 +1,23 @@
1+package=libpng
2+$(package)_version=1.6.34
3+$(package)_download_path=http://ftp.osuosl.org/pub/libpng/src/libpng16/
4+$(package)_file_name=$(package)-$($(package)_version).tar.gz
5+$(package)_sha256_hash=574623a4901a9969080ab4a2df9437026c8a87150dfd5c235e28c94b212964a7
6+$(package)_dependencies=zlib
7+
8+define $(package)_set_vars
9+  $(package)_config_opts=--enable-static --disable-shared

fanquake commented at 4:39 am on August 18, 2020:

You can pass --disable-dependency-tracking here (as we do to all other packages that support it). Had a quick look at the configure options and it doesn’t seem like there’s anything obvious we should be disabling, like building documentation.

hebasto commented at 11:42 am on August 18, 2020: member

As https://bugreports.qt.io/browse/QTBUG-66388 seems resolved for Qt 5.12 could this change be skipped, and building PPC64 binaries postponed?

luke-jr commented at 12:44 pm on August 18, 2020: member

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

hebasto commented at 12:46 pm on August 18, 2020: member

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.

luke-jr force-pushed on Aug 18, 2020

luke-jr commented at 12:50 pm on August 18, 2020: member

Applied the proposed changes

in depends/packages/libpng.mk:5 in 963797019c outdated

0@@ -0,0 +1,24 @@
1+package=libpng
2+$(package)_version=1.6.37
3+$(package)_download_path=https://downloads.sourceforge.net/project/libpng/libpng16/$($(package)_version)/
4+$(package)_file_name=$(package)-$($(package)_version).tar.gz
5+$(package)_sha256_hash=c509d15ebdbfa355469828df2edcba15c5656761dd3037fcf28c206b5268a035

hebasto commented at 2:45 pm on August 18, 2020:

Switch to LZMA-compressed archive?

0$(package)_file_name=$(package)-$($(package)_version).tar.xz
1$(package)_sha256_hash=505e70834d35383537b6491e7ae8641f1a4bed1876dbfe361201fc80868d88ca

less by 50% :)

depends: Split libpng out of Qt 8d8d3f1396

luke-jr force-pushed on Aug 18, 2020

DrahtBot commented at 10:42 pm on August 18, 2020: member

Guix builds

File	commit 1bc8e8eae2dc4b47ef67afc6880ea48b8e84a086(master)	commit 351605f9d595f08d1830c2605fd812767cff9c5d(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`ba41f394f562c9ec...`	`a13caa8eb36ce3f7...`
*-aarch64-linux-gnu.tar.gz	`70d920e06497e77b...`	`bd67d720785d009b...`
*-arm-linux-gnueabihf-debug.tar.gz	`1af0f9da257d8cdb...`	`3dd5c901c2369bee...`
*-arm-linux-gnueabihf.tar.gz	`4017f134a463404c...`	`6a339e1a89544a3f...`
*-riscv64-linux-gnu-debug.tar.gz	`ac6e7216502ab750...`	`4829de0abd996453...`
*-riscv64-linux-gnu.tar.gz	`fded0f57f7a0fe6c...`	`43dce998646be93c...`
*-win-unsigned.tar.gz	`ee2b6e81325c9277...`	`2260533c9904fb12...`
*-win64-debug.zip	`c27b3c9b14d2aa2f...`	`39d87278d214e334...`
*-win64-setup-unsigned.exe	`8a09c5a88d0f5735...`	`9bb02b9a2be497da...`
*-win64.zip	`e7f5f963a6e490be...`	`0ffca41dbdda164a...`
*-x86_64-linux-gnu-debug.tar.gz	`bb2303a5a9e2d901...`	`c82da45e10d2dd9b...`
*-x86_64-linux-gnu.tar.gz	`6c0be417f285c32c...`	`c1b4021ccd0a81e3...`
*.tar.gz	`9e674ed0081c5f0b...`	`5d96d76e01553899...`
guix_build.log	`9d3c81b395c6f903...`	`0c318b8fd420afab...`
guix_build.log.diff		`bfd219662c47a00a...`

DrahtBot removed the label Needs Guix build on Aug 18, 2020

hebasto commented at 10:20 am on August 19, 2020: member

~~Cannot make gitian build for macOS. Sure it is not related to this PR though.~~ nm, was pointed at #19240

hebasto approved

hebasto commented at 11:22 am on August 19, 2020: member

ACK 8d8d3f1396736a3ae284f65eaa529984b9c3e97f, 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:

 04995b8f64c966d17d9aa8db328ad32e07fc1c9d411c611a236d4129eb3b40608  bitcoin-8d8d3f139673-aarch64-linux-gnu-debug.tar.gz
 1f9e26ef0e9482ce08f8b5c55d42db23d3c8f428222311901fcdbfa972d4c1972  bitcoin-8d8d3f139673-aarch64-linux-gnu.tar.gz
 236dcb911729493e70123a4d4a826ea2e1744e57fa34d43b2bb469764985363e5  bitcoin-8d8d3f139673-arm-linux-gnueabihf-debug.tar.gz
 30074ce82fff383930bc426ca17cb49d1dcb8a2f5080186eb9d0a9a3ecd39bc33  bitcoin-8d8d3f139673-arm-linux-gnueabihf.tar.gz
 4328e5756d14428a5abe357ccfe67ed998d3bf78c238e34d9e71b2d40eb8be7e4  bitcoin-8d8d3f139673-riscv64-linux-gnu-debug.tar.gz
 52a557d53a2b7efd88619318241460f7b9df0d25df072a9e746352c4095bc624b  bitcoin-8d8d3f139673-riscv64-linux-gnu.tar.gz
 6f5a099afb3b7bc2a90f1b3b32258242a89fc286c171546d305bc8f7eb3d3610b  bitcoin-8d8d3f139673-x86_64-linux-gnu-debug.tar.gz
 7660c86f13069dd7f189e1c5110fbeda53657a0a220b5ee6b76328ef0a283ef71  bitcoin-8d8d3f139673-x86_64-linux-gnu.tar.gz
 8
 9b1d19a481ea6cc8279d8cc184139992fd3469b6be1bcf86576752aaf11d987c8  bitcoin-8d8d3f139673-win-unsigned.tar.gz
1059127b06b566b243664ef5699a78c54841fc81b4122a046b1332b250dee081f2  bitcoin-8d8d3f139673-win64-debug.zip
117821e41feeaf713083132129eda25040d75e8d2c36229422854b0986efb5c2eb  bitcoin-8d8d3f139673-win64-setup-unsigned.exe
12b620958be279680e259ed1ca366f98f7a2880c32fdd42e9645efe44c8443e69d  bitcoin-8d8d3f139673-win64.zip
13
145a46200cfb7e527d5b75ba50e615bff13ec76aa7022fc32978ab8baa9c160d2c  bitcoin-8d8d3f139673-osx-unsigned.dmg
15ce3790234531e8013471d1b7bd3c57df3c6474c4ef61d0cbb3744ff13553ebc6  bitcoin-8d8d3f139673-osx-unsigned.tar.gz
161053e6776209260d58d695543bad261690c1b71b90d52e96588e68d17c972cf7  bitcoin-8d8d3f139673-osx64.tar.gz
17
1888224654414478c5602342e14ee8aedcbf31f7416d8e914b750314e062de756e  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)

DrahtBot commented at 8:09 pm on August 20, 2020: 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:

#19716 (build: Qt 5.15.x by fanquake)
#18077 (net: Add NAT-PMP port forwarding support by hebasto)

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.

DrahtBot commented at 2:50 pm on August 21, 2020: member

Gitian builds

File	commit e9b30126545d6ddd8772363e4079d1e4908ad117(master)	commit 8ec6ac313f70defb90307beed4ce57491b6cfc3d(master and this pull)
bitcoin-core-linux-0.21-res.yml	`cea1ba265603d02f...`	`8ac64d17ff0d0674...`
bitcoin-core-osx-0.21-res.yml	`77d7884f6a574134...`	`5d3bc3dbe6b71923...`
bitcoin-core-win-0.21-res.yml	`0b0f6bbbd9a5c0ef...`	`d5dad076973f6050...`
*-aarch64-linux-gnu-debug.tar.gz	`4ed42dd1dc70bea9...`	`00f7881590b78741...`
*-aarch64-linux-gnu.tar.gz	`8a7a6ef9cccbae35...`	`bfb79a65f26cdcc6...`
*-arm-linux-gnueabihf-debug.tar.gz	`e40e4858ee8bd4d1...`	`5ef1ed867e8a0103...`
*-arm-linux-gnueabihf.tar.gz	`7ddca7cad75ef53b...`	`bb058910cc21d14a...`
*-osx-unsigned.dmg	`79a2858ea685e8b0...`	`49da18d5abbaa8b1...`
*-osx64.tar.gz	`e77905a2f3abb599...`	`b6eb2ad69387d82b...`
*-riscv64-linux-gnu-debug.tar.gz	`4ccc4f940f998b62...`	`cba09b254ccdbdae...`
*-riscv64-linux-gnu.tar.gz	`561c5ed7ebaf0ea5...`	`cb3309316e40d79a...`
*-win64-debug.zip	`e0b30b67a2217a50...`	`178ba1536a48534f...`
*-win64-setup-unsigned.exe	`3e5025cde6a4d3d5...`	`d260c8dd4d5ca281...`
*-win64.zip	`caee7761af12305b...`	`a3834318722e48cd...`
*-x86_64-linux-gnu-debug.tar.gz	`4036b1cb7c0177b0...`	`f62c68633a6f2808...`
*-x86_64-linux-gnu.tar.gz	`e15879353155c44c...`	`649ec5092628d6e9...`
*.tar.gz	`e8cbb99f0f9f02a0...`	`53716e03277db323...`
linux-build.log	`43933168846d907b...`	`2b308ff5825cc184...`
osx-build.log	`f0d612ea7fbc1c07...`	`6590507eec49dd7a...`
win-build.log	`54a3cd2d3d8f9dd4...`	`c8455754e574fdc3...`
bitcoin-core-linux-0.21-res.yml.diff		`7ccaf493788ae0c3...`
bitcoin-core-osx-0.21-res.yml.diff		`8b2233b9f58e6c79...`
bitcoin-core-win-0.21-res.yml.diff		`69acc146c12feaaa...`
linux-build.log.diff		`51f5834a25c77243...`
osx-build.log.diff		`62125a018d822279...`
win-build.log.diff		`b2ef1f9733806e61...`

DrahtBot removed the label Needs gitian build on Aug 21, 2020

laanwj commented at 10:09 am on August 22, 2020: member

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.

theuni commented at 7:12 pm on September 4, 2020: member

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 referenced this in commit f02a10a101 on Sep 15, 2020

fanquake referenced this in commit 6875844061 on Sep 15, 2020

fanquake referenced this in commit f07fb5a55e on Sep 15, 2020

fanquake commented at 2:56 pm on September 15, 2020: member

I’ve posted an alternative to this PR in #19959. I think instead of splitting out libpng, we could just apply the upstream patch.

MarcoFalke referenced this in commit c7eb85d005 on Sep 22, 2020

fanquake commented at 11:41 pm on September 22, 2020: member

Closing this now that #19959 has been merged. I assume you’ll reopen #14066 now?

fanquake closed this on Sep 22, 2020

sidhujag referenced this in commit aede736079 on Sep 23, 2020

DrahtBot locked this on Feb 15, 2022

depends: Split libpng out of Qt #19751

Guix builds

Conflicts

Gitian builds