Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#27186 closed enhancement (fixed)

spkg-configure.m4 for libpng

Reported by: embray Owned by:
Priority: major Milestone: sage-8.8
Component: packages: standard Keywords: spkg-configure
Cc: fbissey Merged in:
Authors: Dima Pasechnik Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: adc901c (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by dimpase)

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

Attachments (1)

configure-318.tar.gz (106.7 KB) - added by dimpase 3 years ago.
updated configure spkg

Download all attachments as: .zip

Change History (54)

comment:1 Changed 3 years ago by 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...

comment:2 Changed 3 years ago by 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...)

comment:3 Changed 3 years ago by dimpase

What's wrong with OSX's system libpng?

comment:4 follow-up: Changed 3 years ago by embray

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

comment:5 Changed 3 years ago by 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.

comment:6 in reply to: ↑ 4 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by 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.

comment:8 follow-up: Changed 3 years ago by 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.

Last edited 3 years ago by embray (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 3 years ago by 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...

comment:10 follow-up: Changed 3 years ago by 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?

comment:11 in reply to: ↑ 10 Changed 3 years ago by 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 :-)

comment:12 Changed 3 years ago by 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])
    ])
])

comment:13 Changed 3 years ago by dimpase

  • Keywords spkg-configure added

comment:14 Changed 3 years ago by 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").

comment:15 Changed 3 years ago by 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.

comment:16 Changed 3 years ago by 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.

Last edited 3 years ago by dimpase (previous) (diff)

comment:17 Changed 3 years ago by 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.

comment:18 Changed 3 years ago by embray

  • Milestone changed from sage-8.7 to sage-pending

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

comment:19 Changed 3 years ago by 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.

comment:20 Changed 3 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/packages/png-config
  • Commit set to 590b3833197f436d1e6fec95916d32527e373ec6
  • Milestone changed from sage-pending to sage-8.8

New commits:

590b383spkg-configure for libpng

comment:21 Changed 3 years ago by 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.

comment:22 Changed 3 years ago by 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...

comment:23 Changed 3 years ago by git

  • Commit changed from 590b3833197f436d1e6fec95916d32527e373ec6 to cce4eef79a3e6f40f45d385ea754ad50f9d2de88

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

cce4eefspkg-configure for libpng

comment:24 Changed 3 years ago by dimpase

  • Status changed from new to needs_review

comment:25 Changed 3 years ago by git

  • Commit changed from cce4eef79a3e6f40f45d385ea754ad50f9d2de88 to 4e9fca1dd163c19116521bffce526f57162b35a9

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

4e9fca1spkg-configure for libpng

comment:26 Changed 3 years ago by dimpase

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

comment:27 Changed 3 years ago by dimpase

  • Status changed from needs_review to needs_info

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

comment:28 Changed 3 years ago by git

  • Commit changed from 4e9fca1dd163c19116521bffce526f57162b35a9 to 2f5aaa77a5e880069733d2dbd4db9b3b696f6e48

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

d20b81bspkg-configure for libpng
2f5aaa7don't check unneeded requirements for system libpng

comment:29 Changed 3 years ago by dimpase

  • Status changed from needs_info to needs_review

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

comment:30 Changed 3 years ago by git

  • Commit changed from 2f5aaa77a5e880069733d2dbd4db9b3b696f6e48 to 1e8b1a23a0316eb3d34e7c7a80fc6b95f054d716

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

f4d8970spkg-configure for libpng
f5fa38edon't check unneeded requirements for system libpng
1e8b1a2bumped up configure spkg

Changed 3 years ago by dimpase

updated configure spkg

comment:31 Changed 3 years ago by dimpase

  • Cc fbissey added; dimpase removed
  • Description modified (diff)

comment:32 Changed 3 years ago by embray

I guess should have #27265 as a dependency?

comment:33 Changed 3 years ago by dimpase

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

comment:34 Changed 3 years ago by dimpase

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

comment:35 Changed 3 years ago by dimpase

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

comment:36 Changed 3 years ago by embray


Well, this branch contains the diff:

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

comment:37 Changed 3 years ago by dimpase

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

comment:38 Changed 3 years ago by 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).

comment:39 Changed 3 years ago by dimpase

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

comment:40 Changed 3 years ago by git

  • Commit changed from 1e8b1a23a0316eb3d34e7c7a80fc6b95f054d716 to adc901cdcb4ae4850a9dcc4a009add6c569903fc

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

a6e554fspkg-configure for libpng
adc901cdon't check unneeded requirements for system libpng

comment:41 Changed 3 years ago by dimpase

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

comment:42 Changed 3 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Part of series of ticket.

comment:43 Changed 3 years ago by dimpase

Please merge together with #27825

comment:44 follow-up: Changed 3 years ago by 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?

comment:45 in reply to: ↑ 44 ; follow-up: Changed 3 years ago by 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...)

Last edited 3 years ago by dimpase (previous) (diff)

comment:46 in reply to: ↑ 45 Changed 3 years ago by 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)

comment:47 Changed 3 years ago by 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.

Last edited 3 years ago by dimpase (previous) (diff)

comment:48 Changed 3 years ago by vbraun

  • Branch changed from u/dimpase/packages/png-config to adc901cdcb4ae4850a9dcc4a009add6c569903fc
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:49 Changed 3 years ago by tmonteil

  • Commit adc901cdcb4ae4850a9dcc4a009add6c569903fc deleted

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

comment:50 Changed 3 years ago by 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.

comment:51 follow-up: Changed 3 years ago by saraedum

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

comment:52 Changed 3 years ago by 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.

Version 0, edited 3 years ago by dimpase (next)

comment:53 in reply to: ↑ 51 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.