#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: |
Description (last modified by )
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)
Change History (54)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
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
What's wrong with OSX's system libpng?
comment:4 follow-up: ↓ 6 Changed 3 years ago by
It's unusable. See the SPKG.txt for libpng.
comment:5 Changed 3 years ago by
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
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
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: ↓ 9 Changed 3 years ago by
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.
comment:9 in reply to: ↑ 8 Changed 3 years ago by
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: ↓ 11 Changed 3 years ago by
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
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
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
- Keywords spkg-configure added
comment:14 Changed 3 years ago by
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
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
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.
comment:17 Changed 3 years ago by
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
- 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
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
- Branch set to u/dimpase/packages/png-config
- Commit set to 590b3833197f436d1e6fec95916d32527e373ec6
- Milestone changed from sage-pending to sage-8.8
New commits:
590b383 | spkg-configure for libpng
|
comment:21 Changed 3 years ago by
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
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
- Commit changed from 590b3833197f436d1e6fec95916d32527e373ec6 to cce4eef79a3e6f40f45d385ea754ad50f9d2de88
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cce4eef | spkg-configure for libpng
|
comment:24 Changed 3 years ago by
- Status changed from new to needs_review
comment:25 Changed 3 years ago by
- Commit changed from cce4eef79a3e6f40f45d385ea754ad50f9d2de88 to 4e9fca1dd163c19116521bffce526f57162b35a9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4e9fca1 | spkg-configure for libpng
|
comment:26 Changed 3 years ago by
we also need to check that zlib, the dependency, comes from the system.
comment:27 Changed 3 years ago by
- 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
- Commit changed from 4e9fca1dd163c19116521bffce526f57162b35a9 to 2f5aaa77a5e880069733d2dbd4db9b3b696f6e48
comment:29 Changed 3 years ago by
- 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
- Commit changed from 2f5aaa77a5e880069733d2dbd4db9b3b696f6e48 to 1e8b1a23a0316eb3d34e7c7a80fc6b95f054d716
comment:31 Changed 3 years ago by
- Cc fbissey added; dimpase removed
- Description modified (diff)
comment:32 Changed 3 years ago by
I guess should have #27265 as a dependency?
comment:33 Changed 3 years ago by
no, why? neither is a dependency of the other.
comment:34 Changed 3 years ago by
but there is the bloody mess with configure package versions I am complaining about, sure.
comment:35 Changed 3 years ago by
once you start depending on generated stuff, it's like crack ;-)
comment:36 Changed 3 years ago by
Well, this branch contains the diff:
-
build/pkgs/configure/package-version.txt
diff --git a/build/pkgs/configure/package-version.txt b/build/pkgs/configure/package-version.txt index 4dab36b..dda3451 100644
a b 1 31 71 318
which to make sense I thought would need #27265, which supplies configure-317.
comment:37 Changed 3 years ago by
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
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
sure, I'm all for it - but we need more of these actually positively reviewed ;-)
comment:40 Changed 3 years ago by
- Commit changed from 1e8b1a23a0316eb3d34e7c7a80fc6b95f054d716 to adc901cdcb4ae4850a9dcc4a009add6c569903fc
comment:41 Changed 3 years ago by
removed configure bump and rebased over 8.8.beta5 for the ease of reviewing.
comment:42 Changed 3 years ago by
- 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
Please merge together with #27825
comment:44 follow-up: ↓ 45 Changed 3 years ago by
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: ↓ 46 Changed 3 years ago by
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 placeinflateValidate
is used in our copy of libpng is the following, inpngrutil.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); #endifIt'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...)
comment:46 in reply to: ↑ 45 Changed 3 years ago by
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
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.
comment:48 Changed 3 years ago by
- 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
- Commit adc901cdcb4ae4850a9dcc4a009add6c569903fc deleted
Let me point that there is an issue with inflateValidate
in #27319.
comment:50 Changed 3 years ago by
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: ↓ 53 Changed 3 years ago by
It appears that tachyon is ignoring the CFLAGS that were used when ./configure ran.
comment:52 Changed 3 years ago by
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.
comment:53 in reply to: ↑ 51 Changed 3 years ago by
Replying to saraedum:
It appears that tachyon is ignoring the CFLAGS that were used when ./configure ran.
right. It's quite a mess indeed.
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...