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://trac.sagemath.org/raw-attachment/ticket/27186/configure-318.tar.gz

[embray]

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]

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]

What's wrong with OSX's system libpng?

[embray]

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

[embray]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

[dimpase]

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]

New commits:

​590b383

spkg-configure for libpng

[embray]

I need to re-review this, but I'm okay with just throwing my hands up and saying to hell with OSX. If there is still an issue we'll find out, and now that we have --without-system-libpng at least there's a workaround in place.

[dimpase]

I think Apple's library won't be known to pkg-config, and then AC_SEARCH_LIBS would filter it out. Anyhow it's probably only some post-EOL OSX versions that might potentially be problematic...

[git]

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

​cce4eef

spkg-configure for libpng

[git]

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

​4e9fca1

spkg-configure for libpng

[dimpase]

we also need to check that zlib, the dependency, comes from the system.

[dimpase]

but zlib does not need to satisfy the requirements for Sages's libpng.

[git]

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

​d20b81b	spkg-configure for libpng
​2f5aaa7	don't check unneeded requirements for system libpng

[dimpase]

now we do not try to check for unneeded new functionality in zlib for older libpng's

[git]

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

​f4d8970	spkg-configure for libpng
​f5fa38e	don't check unneeded requirements for system libpng
​1e8b1a2	bumped up configure spkg

[dimpase]

updated configure spkg

[embray]

I guess should have #27265 as a dependency?

[dimpase]

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

[dimpase]

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

[dimpase]

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

[embray]

Well, this branch contains the diff:

build/pkgs/configure/package-version.txt

@@ -1 @@
-317
+318

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

[dimpase]

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

[embray]

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]

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

[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]

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

[fbissey]

Part of series of ticket.

[dimpase]

Please merge together with #27825

[mjo]

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]

Replying to mjo:

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

[mjo]

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]

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.

[tmonteil]

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

[saraedum]

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 ​https://trac.sagemath.org/ticket/28745#comment:33.

[saraedum]

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

[dimpase]

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.

[dimpase]

Replying to saraedum:

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

right. It's quite a mess indeed.