Opened 5 months ago

Closed 4 months 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) Commit: 8c97d9ba22cf72c2ca71f59e24bcb56aa4d26f18
Dependencies: Stopgaps:

Change History (44)

comment:1 follow-up: Changed 5 months ago by dimpase

I'm trying

SAGE_SPKG_CONFIGURE([libgd], [
    AC_REQUIRE([SAGE_SPKG_CONFIGURE_FREETYPE])
    AC_MSG_CHECKING([Installing freetype? ])
    if test x$sage_spkg_install_freetype= xyes; then
      AC_MSG_RESULT([Yes. Install libgd as well.])
      sage_spkg_install_libgd=yes
    else
      AC_MSG_RESULT([No.])
      PKG_CHECK_MODULES([LIBGD], [gdlib >= 2.1], [], [sage_spkg_install_libgd=yes])
    fi
])

to see whether this produces a working Sage (on my system libgd is not linked to iconv, according to ldd).

comment:2 Changed 5 months ago by mjo

  • 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:

  1. 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.
  2. We'd like the detected system libgd to be as consistent as possible with the one that sage builds; however...
  3. 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,

  1. Leave the iconv dependency in libgd, and accept a system libgd with no such support.
  2. 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 5 months ago by mjo

Replying to dimpase:

    AC_MSG_CHECKING([Installing freetype? ])

libpng too?

comment:4 Changed 5 months ago by dimpase

yes, sure, I didn't spell out all the dependencies yet.

comment:5 follow-up: Changed 5 months ago by dimpase

  • Authors set to Dima Pasechnik
  • 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.

Last edited 5 months ago by dimpase (previous) (diff)

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 months ago by 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.

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 5 months ago by git

  • Commit changed from 06356729e88dd7e102e945a00412e99d7c4d85b6 to 33a144179f216d62e0eaaf6263f7b2162da3cd39

Branch pushed to git repo; I updated commit sha1. New commits:

33a1441check for libpng explicitly

comment:8 in reply to: ↑ 6 Changed 5 months ago by dimpase

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 5 months ago by dimpase

  • Cc fbissey added

comment:10 Changed 5 months ago by fbissey

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: Changed 5 months ago by dimpase

this is what we build in Sage, just look at its libgd/dependencies.

comment:12 in reply to: ↑ 11 Changed 5 months ago by fbissey

  • 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 5 months ago by dimpase

Thanks, but please review its dependencies too!

comment:14 Changed 5 months ago by fbissey

I see, it is part of the same bundle.

comment:15 Changed 5 months ago by dimpase

To release manager - this might need a configure bump.

comment:16 Changed 5 months ago by git

  • 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:

2a9d203configure bump

comment:17 Changed 5 months ago by dimpase

  • Description modified (diff)
  • Status changed from needs_review to positive_review

comment:18 Changed 5 months ago by git

  • 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:

8d4157bspkg-configure for ncurses, request version >= 5.7
7b78256readline try on osx
2b3a5c7new readline 8.0 and checks in its spkg-configure
228bba0Revert "new readline 8.0 and checks in its spkg-configure"
79e37a0test for rl_bind_keyseq to detect old or fake readlines
6828f7aget termcap library name from the readline's configure
8376eb2Merge remote-tracking branch 'trac/u/dimpase/packages/ncurses_readline-config' into plusreadline
395a185Trac #27814: add configure check for system "rw" library.
b08f2f9Merge remote-tracking branch 'trac/u/mjo/ticket/27814' into plusreadline

comment:19 Changed 5 months ago by dimpase

  • Dependencies changed from #27168 to #27168, #27814, #27277

merged all the (potentially) configure-bumping positively reviewed tickets from #27330 here.

comment:20 Changed 5 months ago by dimpase

  • Status changed from needs_review to positive_review

comment:21 Changed 5 months ago by embray

  • Status changed from positive_review to needs_review

comment:22 Changed 5 months ago by embray

Please--please give me a chance to review these.

comment:23 Changed 5 months ago by embray

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 5 months ago by embray

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 5 months ago by dimpase

could it be just missing space in if test... ?

comment:26 Changed 5 months ago by embray

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.

Last edited 5 months ago by embray (previous) (diff)

comment:27 Changed 5 months ago by embray

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 5 months ago by git

  • Commit changed from b08f2f990e091b25026bf8a608c09b03aa0d8617 to 4558bd8d3c9a966e5b796d42af70ea4661a9678c

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

b5ed6fdreadline try on osx
2be9ab3new readline 8.0 and checks in its spkg-configure
253cd3dRevert "new readline 8.0 and checks in its spkg-configure"
baac646test for rl_bind_keyseq to detect old or fake readlines
09f2ce8get termcap library name from the readline's configure
69a9191a straightforward version with pkg-config only
9a24e83add SAGE_FREETYPE_PREFIX for libgd
f89ea02spkg-configure for libgd
272cb70remove dependence on iconv, deemed unneeded on #27825
4558bd8check for libpng explicitly

comment:29 Changed 5 months ago by dimpase

  • Dependencies changed from #27168, #27814, #27277 to #27168, #27277

rebased over 8.8.beta6

comment:30 Changed 5 months ago by git

  • Commit changed from 4558bd8d3c9a966e5b796d42af70ea4661a9678c to a5aef02d9a6e3ad5bf432d8eef9d015e28eb4914

Branch pushed to git repo; I updated commit sha1. New commits:

acf9cc6For consistency's sake, set SAGE_FREETYPE_PREFIX to empty when using the system lib
3809866As we are updating libgd, go ahead and add an spkg-legacy-uninstall for removing old versions
a5aef02Update these messages to match stylistically with the similar messages

comment:31 Changed 5 months ago by git

  • Commit changed from a5aef02d9a6e3ad5bf432d8eef9d015e28eb4914 to d4cfc22da17a3a2a327e61ece1e35db3483b2414

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

fb034b5get termcap library name from the readline's configure
aab8e76a straightforward version with pkg-config only
676c860add SAGE_FREETYPE_PREFIX for libgd
183a595spkg-configure for libgd
ec09e25remove dependence on iconv, deemed unneeded on #27825
249aaa4check for libpng explicitly
330f256For consistency's sake, set SAGE_FREETYPE_PREFIX to empty when using the system lib
8ba295fAs we are updating libgd, go ahead and add an spkg-legacy-uninstall for removing old versions
e798188Update these messages to match stylistically with the similar messages
d4cfc22bump up configure version

comment:32 Changed 5 months ago by dimpase

  • Description modified (diff)

rebased over 8.8.beta7, bumped up configure version

comment:33 Changed 4 months ago by embray

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 4 months ago by dimpase

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 4 months ago by git

  • Commit changed from d4cfc22da17a3a2a327e61ece1e35db3483b2414 to a957fde5f41cd900f098b288c89970041059b865

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

dba6830test for rl_bind_keyseq to detect old or fake readlines
fd1a7f1get termcap library name from the readline's configure
e87f27ea straightforward version with pkg-config only
6df1719add SAGE_FREETYPE_PREFIX for libgd
d19543dspkg-configure for libgd
f4b4cacremove dependence on iconv, deemed unneeded on #27825
8a3693acheck for libpng explicitly
ddeb8c1For consistency's sake, set SAGE_FREETYPE_PREFIX to empty when using the system lib
d47c654As we are updating libgd, go ahead and add an spkg-legacy-uninstall for removing old versions
a957fdeUpdate these messages to match stylistically with the similar messages

comment:36 Changed 4 months ago by dimpase

rebased over 8.8.rc0; no configure bump done

comment:37 Changed 4 months ago by embray

  • 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 4 months ago by embray

  • 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 4 months ago by git

  • 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 4 months ago by dimpase

  • 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 4 months ago by git

  • 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:

57c6e5ca straightforward version with pkg-config only
d07d326add SAGE_FREETYPE_PREFIX for libgd
2b9d77cspkg-configure for libgd
113830dremove dependence on iconv, deemed unneeded on #27825
da9af50check for libpng explicitly
e3a4f75For consistency's sake, set SAGE_FREETYPE_PREFIX to empty when using the system lib
6c88682As we are updating libgd, go ahead and add an spkg-legacy-uninstall for removing old versions
4cbd9a6Update these messages to match stylistically with the similar messages
2bd1bda(re)added missing space in if
8c97d9bconfigure bump

comment:42 Changed 4 months ago by dimpase

  • Description modified (diff)
  • Status changed from needs_review to positive_review

rebased over latest beta

comment:43 Changed 4 months ago by dimpase

  • Dependencies #27168, #27277 deleted

removed "wont fix" deps from the description

comment:44 Changed 4 months ago by vbraun

  • Branch changed from u/dimpase/packages/libgd-config to 8c97d9ba22cf72c2ca71f59e24bcb56aa4d26f18
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.