Opened 3 years ago
Closed 3 years ago
#27825 closed enhancement (fixed)
spkg-configure.m4 for libgd
Reported by: | dimpase | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.9 |
Component: | build: configure | Keywords: | |
Cc: | mjo, jdemeyer, fbissey | Merged in: | |
Authors: | Dima Pasechnik | Reviewers: | François Bissey, Erik Bray |
Report Upstream: | N/A | Work issues: | |
Branch: | 8c97d9b (Commits, GitHub, GitLab) | Commit: | 8c97d9ba22cf72c2ca71f59e24bcb56aa4d26f18 |
Dependencies: | Stopgaps: |
Description (last modified by )
use pkg-config to get it.
configure tarball is here: http://users.ox.ac.uk/~coml0531/sage/configure-2bd1bda5f85b8f8e4a7fc7499f8529795099160a.tar.gz
Change History (44)
comment:1 follow-up: ↓ 3 Changed 3 years ago by
comment:2 Changed 3 years ago by
- Cc jdemeyer added
(Wall-of-text alert. There's a tl;dr at the end, but I've dumped my reasoning for review so that we don't accidentally break some case I haven't thought of.)
For posterity, we discussed the iconv dependency on the mailing list.
The libgd package has an automagic dependency on iconv, meaning that it has optional support for iconv that is enabled automatically if a suitable iconv is present at build time. At that point, iconv is no longer optional, because libgd will be compiled to call some iconv functions, and will potentially be linked to libiconv.
Moreover, there are a few ways that "libiconv" can be implemented. On modern linux systems, the calls are all part of glibc, which is why you don't see libgd being explicitly linked against iconv on those systems. However, on other operating systems, GNU libiconv is indeed installed as a separate package, and you will find libgd linked against it on those systems.
Now, I don't believe that SageMath uses any of the iconv support in libgd. The only SageMath file to call a GD function is matrix/matrix_mod2_dense.pyx
(someone confirm this?), and it doesn't pass any strings to the API. The only place iconv is used in libgd is in src/gdkanji.c
, and it's used for manipulate string encodings. So, SageMath shouldn't need it.
As a result, I don't think iconv should be a dependency of the SageMath libgd package, but there are some conflicting goals here that I'll catalogue:
- We'd like the libgd built by sage to be consistent across platforms. That is, it should either have iconv support or not, regardless of the OS.
- We'd like the detected system libgd to be as consistent as possible with the one that sage builds; however...
- We don't want to force the system libgd to be built with a specific configuration with regards to iconv, because that will prevent the system package from being used in a lot of cases where it otherwise could.
There's no perfect solution for all three, and in my opinion the third is the most important, which forces us to choose between (1) and (2). The two associated plans of action are,
- Leave the iconv dependency in libgd, and accept a system libgd with no such support.
- Drop the iconv dependency in libgd, and take whatever happens.
The second option means that the libgd built by sage won't be consistent, but it does bring the sage package to parity with the system package check. And, it's simpler. So finally,
tl;dr I think the practical solution is to
- Remove iconv from libgd's dependencies
- Remove iconv from libgd's SPKG.txt
- Write spkg-configure.m4 without mention of iconv (like Dima just did).
comment:3 in reply to: ↑ 1 Changed 3 years ago by
comment:4 Changed 3 years ago by
yes, sure, I didn't spell out all the dependencies yet.
comment:5 follow-up: ↓ 6 Changed 3 years ago by
- Branch set to u/dimpase/packages/libgd-config
- Commit set to 06356729e88dd7e102e945a00412e99d7c4d85b6
- Dependencies set to #27168
- Description modified (diff)
- Status changed from new to needs_review
No need to explicitly check for libpng being from the system, as this is done by the freetype macro run.
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 3 years ago by
Replying to dimpase:
No need to explicitly check for libpng being from the system, as this is done by the freetype macro run.
We should still check it ourselves, for the same reason we need the redundant libpng in libgd's dependencies
: if freetype ever drops the libpng dependency, someone would be within his rights to remove that check from the freetype macro, and would have no idea that it's going to break libgd.
That breakage will manifest on some weird system a year later, and it will take us a lot more time to track it down then than it will to play it safe now.
comment:7 Changed 3 years ago by
- Commit changed from 06356729e88dd7e102e945a00412e99d7c4d85b6 to 33a144179f216d62e0eaaf6263f7b2162da3cd39
Branch pushed to git repo; I updated commit sha1. New commits:
33a1441 | check for libpng explicitly
|
comment:8 in reply to: ↑ 6 Changed 3 years ago by
Replying to mjo:
Replying to dimpase:
No need to explicitly check for libpng being from the system, as this is done by the freetype macro run.
We should still check it ourselves, for the same reason we need the redundant libpng in libgd's
dependencies
: if freetype ever drops the libpng dependency, someone would be within his rights to remove that check from the freetype macro, and would have no idea that it's going to break libgd.
OK, fixed.
comment:9 Changed 3 years ago by
- Cc fbissey added
comment:10 Changed 3 years ago by
Hum... Feels circular at first sight (libpng <-> zlib). Also libgd actually picks many other formats- it is one of its point. Is libpng really the only one we care about? jpeg or anything else?
comment:11 follow-up: ↓ 12 Changed 3 years ago by
this is what we build in Sage, just look at its libgd/dependencies.
comment:12 in reply to: ↑ 11 Changed 3 years ago by
- Reviewers set to François Bissey
- Status changed from needs_review to positive_review
Replying to dimpase:
this is what we build in Sage, just look at its libgd/dependencies.
Fair enough. The chain of various checks between libpng, zlib and freetype is hurting my head. I am glad we can just abstract it and let autoconf deal with it until it complains. If it can digest it and produce a working configure script that's already a strong test.
comment:13 Changed 3 years ago by
Thanks, but please review its dependencies too!
comment:14 Changed 3 years ago by
I see, it is part of the same bundle.
comment:15 Changed 3 years ago by
To release manager - this might need a configure bump.
comment:16 Changed 3 years ago by
- Commit changed from 33a144179f216d62e0eaaf6263f7b2162da3cd39 to 2a9d203b0f380b1a23709ec924300f9a9d7f7dea
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
2a9d203 | configure bump
|
comment:17 Changed 3 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
comment:18 Changed 3 years ago by
- Commit changed from 2a9d203b0f380b1a23709ec924300f9a9d7f7dea to b08f2f990e091b25026bf8a608c09b03aa0d8617
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
8d4157b | spkg-configure for ncurses, request version >= 5.7
|
7b78256 | readline try on osx
|
2b3a5c7 | new readline 8.0 and checks in its spkg-configure
|
228bba0 | Revert "new readline 8.0 and checks in its spkg-configure"
|
79e37a0 | test for rl_bind_keyseq to detect old or fake readlines
|
6828f7a | get termcap library name from the readline's configure
|
8376eb2 | Merge remote-tracking branch 'trac/u/dimpase/packages/ncurses_readline-config' into plusreadline
|
395a185 | Trac #27814: add configure check for system "rw" library.
|
b08f2f9 | Merge remote-tracking branch 'trac/u/mjo/ticket/27814' into plusreadline
|
comment:19 Changed 3 years ago by
- Dependencies changed from #27168 to #27168, #27814, #27277
merged all the (potentially) configure-bumping positively reviewed tickets from #27330 here.
comment:20 Changed 3 years ago by
- Status changed from needs_review to positive_review
comment:21 Changed 3 years ago by
- Status changed from positive_review to needs_review
comment:22 Changed 3 years ago by
Please--please give me a chance to review these.
comment:23 Changed 3 years ago by
Ok, on my current Cygwin (which is a slightly old version by several months) the detection of my system's libpng, libfreetype, and libgd all worked (the latter after I installed the libgd-devel package of course).
rw not so much because (unsurprisingly) there is not a Cygwin package for rw. Perhaps that's something I can do; up to this point I've actually never submitted a brand new Cygwin package, but I know more-or-less how it's done and rw should be very easy to package.
comment:24 Changed 3 years ago by
Just realized I also got this in my configure output:
configure: === checking whether to install the libgd SPKG === checking Installing freetype? ... ./configure: line 11221: test: xno=: unary operator expected
will look into it...
comment:25 Changed 3 years ago by
could it be just missing space in if test...
?
comment:26 Changed 3 years ago by
That's what it looks like. I wonder if I just accidentally deleted the space while looking at the file. No, it was already missing.
comment:27 Changed 3 years ago by
Compiles OK on Cygwin (again, with all packages relevant to this ticket taken from the system, with the exception of rw). Still want to do a bit of functional testing but I don't imagine any major problems.
comment:28 Changed 3 years ago by
- Commit changed from b08f2f990e091b25026bf8a608c09b03aa0d8617 to 4558bd8d3c9a966e5b796d42af70ea4661a9678c
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
b5ed6fd | readline try on osx
|
2be9ab3 | new readline 8.0 and checks in its spkg-configure
|
253cd3d | Revert "new readline 8.0 and checks in its spkg-configure"
|
baac646 | test for rl_bind_keyseq to detect old or fake readlines
|
09f2ce8 | get termcap library name from the readline's configure
|
69a9191 | a straightforward version with pkg-config only
|
9a24e83 | add SAGE_FREETYPE_PREFIX for libgd
|
f89ea02 | spkg-configure for libgd
|
272cb70 | remove dependence on iconv, deemed unneeded on #27825
|
4558bd8 | check for libpng explicitly
|
comment:29 Changed 3 years ago by
- Dependencies changed from #27168, #27814, #27277 to #27168, #27277
rebased over 8.8.beta6
comment:30 Changed 3 years ago by
- Commit changed from 4558bd8d3c9a966e5b796d42af70ea4661a9678c to a5aef02d9a6e3ad5bf432d8eef9d015e28eb4914
Branch pushed to git repo; I updated commit sha1. New commits:
acf9cc6 | For consistency's sake, set SAGE_FREETYPE_PREFIX to empty when using the system lib
|
3809866 | As we are updating libgd, go ahead and add an spkg-legacy-uninstall for removing old versions
|
a5aef02 | Update these messages to match stylistically with the similar messages
|
comment:31 Changed 3 years ago by
- Commit changed from a5aef02d9a6e3ad5bf432d8eef9d015e28eb4914 to d4cfc22da17a3a2a327e61ece1e35db3483b2414
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
fb034b5 | get termcap library name from the readline's configure
|
aab8e76 | a straightforward version with pkg-config only
|
676c860 | add SAGE_FREETYPE_PREFIX for libgd
|
183a595 | spkg-configure for libgd
|
ec09e25 | remove dependence on iconv, deemed unneeded on #27825
|
249aaa4 | check for libpng explicitly
|
330f256 | For consistency's sake, set SAGE_FREETYPE_PREFIX to empty when using the system lib
|
8ba295f | As we are updating libgd, go ahead and add an spkg-legacy-uninstall for removing old versions
|
e798188 | Update these messages to match stylistically with the similar messages
|
d4cfc22 | bump up configure version
|
comment:32 Changed 3 years ago by
- Description modified (diff)
rebased over 8.8.beta7, bumped up configure version
comment:33 Changed 3 years ago by
The module sage.matrix.matrix_mod2_dense
turns out to be the only one that actually uses libgd. And as I noted in #27901 the need for it is actually highly dubious. For now I don't have the bandwidth to argue with it, but it seems likely to me that it's a dependency we can drop at some point.
In any case, that module tends to depend on more libraries than most others, in part due to the use of libgd, but not solely. Also, the libgd on Cygwin is built with all of its dependencies. With this an other related fixes on Cygwin I get:
$ ldd local/lib/python2.7/site-packages/sage/matrix/matrix_mod2_dense.dll ntdll.dll => /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll (0x7ffddbdf0000) KERNEL32.DLL => /cygdrive/c/WINDOWS/System32/KERNEL32.DLL (0x7ffddbbb0000) KERNELBASE.dll => /cygdrive/c/WINDOWS/System32/KERNELBASE.dll (0x7ffdd8240000) cygm4ri-0-0-20140914.dll => /home/embray/src/sagemath/sage-cygwin/local/bin/cygm4ri-0-0-20140914.dll (0x3ccae0000) cyggd-3.dll => /usr/bin/cyggd-3.dll (0x3f60e0000) cyggmp-10.dll => /usr/bin/cyggmp-10.dll (0x3f5a30000) cygwin1.dll => /usr/bin/cygwin1.dll (0x180040000) cygpng16-16.dll => /usr/bin/cygpng16-16.dll (0x3ebfb0000) cygfontconfig-1.dll => /usr/bin/cygfontconfig-1.dll (0x3f64e0000) cygfreetype-6.dll => /usr/bin/cygfreetype-6.dll (0x3f62f0000) cygimagequant-0.dll => /usr/bin/cygimagequant-0.dll (0x3f2f40000) cygjpeg-8.dll => /usr/bin/cygjpeg-8.dll (0x3f2790000) libpython2.7.dll => /home/embray/src/sagemath/sage-cygwin/local/bin/libpython2.7.dll (0x3c20d0000) cygtiff-6.dll => /usr/bin/cygtiff-6.dll (0x3ea7a0000) cygwebp-7.dll => /usr/bin/cygwebp-7.dll (0x3ea350000) cygiconv-2.dll => /home/embray/src/sagemath/sage-cygwin/local/bin/cygiconv-2.dll (0x3cce10000) cygz.dll => /usr/bin/cygz.dll (0x3e8dc0000) cygexpat-1.dll => /usr/bin/cygexpat-1.dll (0x3f68e0000) cyggcc_s-seh-1.dll => /usr/bin/cyggcc_s-seh-1.dll (0x3f4640000) cyggomp-1.dll => /usr/bin/cyggomp-1.dll (0x3f5730000) cygjbig-2.dll => /usr/bin/cygjbig-2.dll (0x3f2810000) cygbz2-1.dll => /usr/bin/cygbz2-1.dll (0x3f7050000) cyglzma-5.dll => /usr/bin/cyglzma-5.dll (0x3ed760000) cygXpm-4.dll => /usr/bin/cygXpm-4.dll (0x3f7340000) cygX11-6.dll => /usr/bin/cygX11-6.dll (0x3f7560000) cygxcb-1.dll => /usr/bin/cygxcb-1.dll (0x3ea100000) cygXau-6.dll => /usr/bin/cygXau-6.dll (0x3f7540000) cygXdmcp-6.dll => /usr/bin/cygXdmcp-6.dll (0x3f7490000)
So the only libraries coming from Sage and not from the system are:
- iconv (high priority to fix this one IMO)
- python
- m4ri
Pretty good progress I'd say!
comment:34 Changed 3 years ago by
As I have now have iconv's spkg-configure.m4, on #27823, we could go ahead and add the dependency on it for libgd.
by right, however, it only needs testing if system's libgd is linked to libiconv, and I don't quite know how to check this (there is a deprecated gdlib-config script one can call, though)
comment:35 Changed 3 years ago by
- Commit changed from d4cfc22da17a3a2a327e61ece1e35db3483b2414 to a957fde5f41cd900f098b288c89970041059b865
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
dba6830 | test for rl_bind_keyseq to detect old or fake readlines
|
fd1a7f1 | get termcap library name from the readline's configure
|
e87f27e | a straightforward version with pkg-config only
|
6df1719 | add SAGE_FREETYPE_PREFIX for libgd
|
d19543d | spkg-configure for libgd
|
f4b4cac | remove dependence on iconv, deemed unneeded on #27825
|
8a3693a | check for libpng explicitly
|
ddeb8c1 | For consistency's sake, set SAGE_FREETYPE_PREFIX to empty when using the system lib
|
d47c654 | As we are updating libgd, go ahead and add an spkg-legacy-uninstall for removing old versions
|
a957fde | Update these messages to match stylistically with the similar messages
|
comment:36 Changed 3 years ago by
rebased over 8.8.rc0; no configure bump done
comment:37 Changed 3 years ago by
- Status changed from needs_review to positive_review
I still have some nitpicks about the AC_MSG_CHECKING
message formatting being inconsistent in some cases. But I'd rather just get this done for now, and apply my nitpicks in the next round of configure updates.
comment:38 Changed 3 years ago by
- Milestone changed from sage-8.8 to sage-8.9
This should obviously be postponed until the next release (hopefully early on).
comment:39 Changed 3 years ago by
- Commit changed from a957fde5f41cd900f098b288c89970041059b865 to e84c459eb431d2740b5932c43afa09e2e1428291
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
e84c459 | (re)added missing space in if
|
comment:40 Changed 3 years ago by
- Reviewers changed from François Bissey to François Bissey, Erik Bray
- Status changed from needs_review to positive_review
I guess this has fallen through rebasing cracks...
comment:41 Changed 3 years ago by
- Commit changed from e84c459eb431d2740b5932c43afa09e2e1428291 to 8c97d9ba22cf72c2ca71f59e24bcb56aa4d26f18
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. Last 10 new commits:
57c6e5c | a straightforward version with pkg-config only
|
d07d326 | add SAGE_FREETYPE_PREFIX for libgd
|
2b9d77c | spkg-configure for libgd
|
113830d | remove dependence on iconv, deemed unneeded on #27825
|
da9af50 | check for libpng explicitly
|
e3a4f75 | For consistency's sake, set SAGE_FREETYPE_PREFIX to empty when using the system lib
|
6c88682 | As we are updating libgd, go ahead and add an spkg-legacy-uninstall for removing old versions
|
4cbd9a6 | Update these messages to match stylistically with the similar messages
|
2bd1bda | (re)added missing space in if
|
8c97d9b | configure bump
|
comment:42 Changed 3 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
rebased over latest beta
comment:43 Changed 3 years ago by
- Dependencies #27168, #27277 deleted
removed "wont fix" deps from the description
comment:44 Changed 3 years ago by
- Branch changed from u/dimpase/packages/libgd-config to 8c97d9ba22cf72c2ca71f59e24bcb56aa4d26f18
- Resolution set to fixed
- Status changed from positive_review to closed
I'm trying
to see whether this produces a working Sage (on my system libgd is not linked to iconv, according to ldd).