spkg-configure.m4 for libpng

[embray]

Also should be done for #27168

Note libpng comes in a number of different ABI versions, usually indicated in the library filename as a suffix like "12" (for 1.2.x) up through "16" (for 1.6.x), where 16 is the most current and currently supported by Sage. Distros like Debian has separate packages for each libpng ABI version.

I think to keep it simple we should specifically check for and require libpng16. I don't know if all of Sage's dependencies support older libpngs.

configure tarball: https://github.com/sagemath/sage-prod/files/10659243/configure-318.tar.gz

CC: @kiwifb

Component: packages: standard

Author: Dima Pasechnik

Branch: adc901c

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/27186

[embray]

comment:1

That said, my Ubuntu build machine has a kinda old libpng12, and while I'm not saying we should support that, I'm curious to see if it will work...

[embray]

comment:2

Also, we should probably not use the system libpng at all on OSX (unless in homebrew or something, but I don't know if we explicitly support that yet...)

[dimpase]

comment:3

What's wrong with OSX's system libpng?

[embray]

comment:4

It's unusable. See the SPKG.txt for libpng.

[embray]

comment:5

Okay, at least on my Ubuntu 14.04 machine libpng 1.2 worked just fine actually. Did a full make ptestlong from scratch. Nothing complained. So maybe we can afford a bit more flexibility here.

[dimpase]

comment:6

Replying to @embray:

It's unusable. See the SPKG.txt for libpng.

I don't read it this way. I read it as "it won't conflict with the one built by Sage".
Well, maybe I am wrong - it's perhaps worth trying on OSX.

[embray]

comment:7

I think you are misreading. The point is that if you just blindly link -lpng on OSX without an alternative libpng on the linker path you'll get the libPng.dylib used by the system which is not compatible with anything we build that wants to link with libpng.

One workaround is to explicitly ask for one of the ABI-specific names like -lpng12 or -lpng16, at least on OSX. That way you might be able to pick up, say, a version from homebrew, but you won't accidentally link to the unusable OSX system lib.

[embray]

comment:8

Although I may also be misreading some too. ISTM the libPng.dylib on OSX only comes from the ImageIO framework, whatever that is, so I don't know if it on the linker path by default in the first place. The problem is just that we can't use the SONAME (or whatever the OSX equivalent is) of libpng when linking modules in our own packages because then it will conflict with anything that uses the ImageIO framework. This goes to show how little I understand about OSX.

[dimpase]

comment:9

Replying to @embray:

Although I may also be misreading some too. ISTM the libPng.dylib on OSX only comes from the ImageIO framework, whatever that is, so I don't know if it on the linker path by default in the first place. The problem is just that we can't use the SONAME (or whatever the OSX equivalent is) of libpng when linking modules in our own packages because then it will conflict with anything that uses the ImageIO framework. This goes to show how little I understand about OSX.

This still sounds as if using system's libPng would lead to various library conflicts (with other Sage-installed versions of stuff also available on the system), i.e. if we go far enough with our plan it would not matter...

Anyhow, I just checked that Homebrew also does not use system's libPng, but installs its own libpng16.dylib.

One way or another, I think we should not block the usage of Homebrew (or conda) libs, i.e. there should not be a rule saying "we're on OSX! install from source").

PS. Last time I checked (few months ago), it's possible to build and run Sage under Homebrew. Basically, it's an environment where lots of stuff lives in /usr/local...

[embray]

comment:10

Volker's OSX buildbot also has a libpng16 that lives in the /opt/X11 prefix, but I don't think that's a normal thing to have... OSX doesn't normally have an X installed, does it?

[dimpase]

comment:11

Replying to @embray:

Volker's OSX buildbot also has a libpng16 that lives in the /opt/X11 prefix, but I don't think that's a normal thing to have... OSX doesn't normally have an X installed, does it?

Not since 2010 or so, I guess :-)

[dimpase]

comment:12

the following works:

SAGE_SPKG_CONFIGURE([libpng], [
    dnl First try checking for libpng with pkg-config
    PKG_CHECK_MODULES([LIBPNG], [libpng16], [], [
        dnl Fallback to manually grubbing around for headers and libs
        AC_CHECK_HEADERS([libpng16/png.h], [sage_spkg_install_libpng=no; break], [sage_spkg_install_libpng=yes])
        AC_SEARCH_LIBS([png_get_io_ptr], [libpng16], [], [sage_spkg_install_libpng=yes])
    ])
])

[embray]

comment:14

I found that there is no reason to require libpng16 specifically (unless I'm wrong / there is some optional package I'm missing that requires a newer version)?

I was thinking we might do something pretty similar to this, but instead of just libpng16 we would check first for libpng16, and if not found try libpng14, then libpng12, and then just plain libpng (except on macOS where that conflicts with the system's incompatible "libPng").

[embray]

comment:15

If that overcomplicates things though, the above is fine. libpng just tends to be problematic, so I thought it would be good to be flexible in which versions we accept so long as there is no explicit need for 1.6.

[dimpase]

comment:16

in build/pkgs/tachyon/patches/Make-config.patch you see

+PNGLIB= -L$(SAGE_LOCAL)/lib -lpng16 -lz
-#PNGLIB= -L/usr/local/lib -lpng -lz

It suggests that libpng16 is needed.

[embray]

comment:17

I don't think so: This patch was made because the libpng package in Sage delete the normal libpng.so -> libpng16.so symlink that is installed, specifically so as to not conflict with the macOS "libPng", but in turn that means that if you compile tachyon in Sage you have to specify -lpng16, since -lpng does not exist in Sage's lib directory.

I don't think it has to be libpng16 specifically; that patch just specifies -lpng16 because that's what's in Sage. I was able to replace this with -lpng12 and it worked fine.

[embray]

comment:18

Removing most of the rest of my open tickets out of the 8.7 milestone, which should be closed.

[dimpase]

comment:19

I looked a bit into OSX libPng, and I see no trace of it on OSX 10.14.
So it's probably quite obsolete into in libpng's SPKG.

[dimpase]

Author: Dima Pasechnik

[dimpase]

Branch: u/dimpase/packages/png-config

[dimpase]

Commit: 590b383

[dimpase]

New commits:

590b383

spkg-configure for libpng

[dimpase]

comment:33

no, why? neither is a dependency of the other.

[dimpase]

comment:34

but there is the bloody mess with configure package versions I am complaining about, sure.

[dimpase]

comment:35

once you start depending on generated stuff, it's like crack ;-)

[embray]

comment:36

Well, this branch contains the diff:

diff --git a/build/pkgs/configure/package-version.txt b/build/pkgs/configure/package-version.txt
index 4dab36b..dda3451 100644
--- a/build/pkgs/configure/package-version.txt
+++ b/build/pkgs/configure/package-version.txt
@@ -1 +1 @@
-317
+318

which to make sense I thought would need #27265, which supplies configure-317.

[dimpase]

comment:37

well, one has no control over version numbers of configure spkg. for different branches they may clash, be unrelated, etc.

[embray]

comment:38

For that reason, if nothing else, maybe we should do like I suggested elsewhere and combine several of these together into a single ticket (once we're satisfied that they work individually).

[dimpase]

comment:39

sure, I'm all for it - but we need more of these actually positively reviewed ;-)

[sagetrac-git]

Changed commit from 1e8b1a2 to adc901c

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a6e554f	spkg-configure for libpng
adc901c	don't check unneeded requirements for system libpng

[dimpase]

comment:41

removed configure bump and rebased over 8.8.beta5 for the ease of reviewing.

[kiwifb]

Reviewer: François Bissey

[kiwifb]

comment:42

Part of series of ticket.

[dimpase]

comment:43

Please merge together with #27825

[orlitzky]

comment:44

I think the zlib <-> libpng stuff here could still be improved. First, trivia; this should be quoted to avoid syntax errors if the variable has a space in it:

if test x$sage_spkg_install_zlib = xyes; then

Now, the only reason we're checking for inflateValidate in zlib is for the benefit of libpng, correct? Because the only place inflateValidate is used in our copy of libpng is the following, in pngrutil.c:

#if ZLIB_VERNUM >= 0x1290 && \
   defined(PNG_SET_OPTION_SUPPORTED) && defined(PNG_IGNORE_ADLER32)
      if (((png_ptr->options >> PNG_IGNORE_ADLER32) & 3) == PNG_OPTION_ON)
         /* Turn off validation of the ADLER32 checksum in IDAT chunks */
         ret = inflateValidate(&png_ptr->zstream, 0);
#endif

It's wrapped in a zlib version check, so... does anything bad happen if we simply ignore the matter? That is, drop the inflateValidate check from the zlib macro, and eliminate the libpng <-> zlib weirdness as a bonus?

[dimpase]

comment:45

Replying to @orlitzky:

I think the zlib <-> libpng stuff here could still be improved. First, trivia; this should be quoted to avoid syntax errors if the variable has a space in it:

if test x$sage_spkg_install_zlib = xyes; then

sage_spkg_install_zlib is an internal, taking a controlled set of values.
(yes, no, '', I suppose)

Now, the only reason we're checking for inflateValidate in zlib is for the benefit of libpng, correct? Because the only place inflateValidate is used in our copy of libpng is the following, in pngrutil.c:

#if ZLIB_VERNUM >= 0x1290 && \
   defined(PNG_SET_OPTION_SUPPORTED) && defined(PNG_IGNORE_ADLER32)
      if (((png_ptr->options >> PNG_IGNORE_ADLER32) & 3) == PNG_OPTION_ON)
         /* Turn off validation of the ADLER32 checksum in IDAT chunks */
         ret = inflateValidate(&png_ptr->zstream, 0);
#endif

It's wrapped in a zlib version check, so... does anything bad happen if we simply ignore the matter? That is, drop the inflateValidate check from the zlib macro, and eliminate the libpng <-> zlib weirdness as a bonus?

We only check for inflateValidate in system's zlib if we cannot use the system's libpng. In this case we need it, as Sage's libpng requires it. This allows us to get away with using older versions of system's libpng/zlib, which are perfectly fine for our purposes.

PS. I think it's a good example of how vendoring of everything leads to a relentless upgrading spiral (cause a vendored version gets broken on one particular system, or due to conflicts with system libraries that happens for one or another reason...)

[orlitzky]

comment:46

Replying to @dimpase:

We only check for inflateValidate in system's zlib if we cannot use the system's libpng. In this case we need it, as Sage's libpng requires it.

But are you sure? This code is taken from sage's libpng:

#if ZLIB_VERNUM >= 0x1290 && \
   defined(PNG_SET_OPTION_SUPPORTED) && defined(PNG_IGNORE_ADLER32)
      if (((png_ptr->options >> PNG_IGNORE_ADLER32) & 3) == PNG_OPTION_ON)
         /* Turn off validation of the ADLER32 checksum in IDAT chunks */
         ret = inflateValidate(&png_ptr->zstream, 0);
#endif

and that's the only occurrence of "inflateValidate" in the libpng source code. Since that whole block will be ignored for old zlib, I don't think we actually need inflateValidate for our libpng.

(inflateValidate was introduced in zlib commit 9852c209 on 2016-09-20, and ZLIB_VERNUM was set to 0x1290 in commit 2fa463bacf on 2016-12-31, so the check above in libpng should work)

[dimpase]

comment:47

Oh, I see what you're saying now, thanks.

Well, assuming there is one and only zlib, so that ZLIB_VERNUM correctly identifies the presence of inflateValidate, and assuming that a future libpng will not drop this version test for some reason, this test can be dropped.

[vbraun]

Changed branch from u/dimpase/packages/png-config to adc901c

[sagetrac-tmonteil]

Changed commit from adc901c to none

[sagetrac-tmonteil]

comment:49

Let me point that there is an issue with inflateValidate in #27319.

[saraedum]

comment:50

It appears to me that this does not work for tachyon. In tachyon's Make-config.patch, we do the following:

-USEPNG=
-PNGINC=
-PNGLIB=
+USEPNG= -DUSEPNG
+PNGINC= -I$(SAGE_LOCAL)/include
+PNGLIB= -L$(SAGE_LOCAL)/lib -lpng -lz

This won't work if libpng's headers have not been installed to $SAGE_LOCAL. See #28745 comment:33.

[saraedum]

comment:51

It appears that tachyon is ignoring the CFLAGS that were used when ./configure ran.

[dimpase]

comment:52

what is “not working”? it certainly is possible to use system libpng everywhere in sage.
tachyon had to be patched a bit more, iirc, to allow this.

---

All I did I changed -lpng16 to -lpng in a6e554f8755
to accommodate variations in versions. Note that indeed if libpng is not in the default linker path, i.e. a specific -L argument must be supplied, then one might need to patch more...

But otherwise it works, e.g. I have

$ ldd local/bin/tachyon 
	linux-vdso.so.1 (0x00007ffc721f4000)
	libpng16.so.16 => /usr/lib64/libpng16.so.16 (0x00007f0c0a62e000)
	libz.so.1 => /lib64/libz.so.1 (0x00007f0c0a610000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f0c0a4d3000)
	libmvec.so.1 => /lib64/libmvec.so.1 (0x00007f0c0a4a7000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f0c0a484000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f0c0a2b4000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f0c0a716000)

on Gentoo

[dimpase]

comment:53

Replying to @saraedum:

It appears that tachyon is ignoring the CFLAGS that were used when ./configure ran.

right. It's quite a mess indeed.