Opened 6 months ago

Closed 10 days ago

Last modified 7 days ago

#27168 closed defect (duplicate)

spkg-configure.m4 for freetype

Reported by: dimpase Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build: configure Keywords: spkg-configure freetype
Cc: embray, mkoeppe, mjo Merged in:
Authors: Dima Pasechnik Reviewers: François Bissey, Erik Bray
Report Upstream: N/A Work issues:
Branch: public/packages/freetype-config (Commits) Commit: 8b2cf94e4d041d0bbe4d27bf98af5c7dbbed1c59
Dependencies: #27186 Stopgaps:

Description (last modified by slelievre)

It gets ugly with library conflicts, so more libraries available on the system should be used instead of ones shipped by Sage, e.g. freetype is a good example. Freetype may be used by R packages, and they in turn might depend on other system libraries we don't even know about, cf. e.g. #27163.

Ticket closed as duplicate after the corresponding changes were integrated in #27825.

Change History (44)

comment:1 Changed 6 months ago by embray

  • Summary changed from spkg-configure.m4 for more spkg libraries, e.g. freetype to spkg-configure.m4 for freetype

I think we already have a generic ticket about this, so let's have this one just focus specifically on freetype.

comment:2 Changed 6 months ago by embray

Perhaps of interest, from cairo's own configure.ac: https://cgit.freedesktop.org/cairo/tree/configure.ac#n489

We can probably reuse much of this.

comment:3 Changed 6 months ago by dimpase

I actually do not know how to deal with freetype's deps: bzip2, libpng (and zlib - not explicitly listed, but a dep of libpng).

Naturally, if we have to build any of these, we cannot use a system's freetype.

Which brings up the need for a proper dependencies resolver, and raises the question whether we are doing all this right.

IMHO a better plan would be to spin off some, and then all, non-python deps of Sage into a separate (auto-tools) project/package, which builds and installs them into SAGE_LOCAL (just the autotools --prefix parameter). This would allow this to be gradual, once the initial setup, with just a handful already spkg-configure.m4-ed packages spun off. The advantage is that the dependency resolution would be a natural top-down thing.

comment:4 follow-up: Changed 6 months ago by embray

I understand what you're saying and have thought about this problem as well, albeit without a completely clear solution. On the one hand I think it should be possible to use a system package regardless of its dependencies. On the other hand it is asking for trouble doing something like, as in your example, building a bz2 for Sage, but then using a libfreetype from the system which was compiled against its system libbz2. This may "just work" but mostly only by luck.

This was less of a problem when this mechanism was used only for a handful of packages that didn't live somewhere deep in the package dependency graph (the old configure.ac checked for these packages in more or less arbitrary order, and that is still the case unfortunately.

I don't agree with your idea of bundling a bunch of packages together but I'm not sure I understand it either. I think there are a couple things to do here:

Using the freetype/bz2 example again: if we add a spkg-configure.m4 for one package, we should also do so for all its build dependencies. That way, assuming they are detected properly, it will properly use the system packages for the entire dependency tree rooted at freetype.

However, I should also add some logic to SAGE_SPKG_COLLECT (likely with an external helper script) to actually load the package dependency graph and do these checks in a reasonable such that we don't even try to use a package from the system unless we've already detected all its dependencies from the system.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 months ago by dimpase

Replying to embray:

I understand what you're saying and have thought about this problem as well, albeit without a completely clear solution. On the one hand I think it should be possible to use a system package regardless of its dependencies. On the other hand it is asking for trouble doing something like, as in your example, building a bz2 for Sage, but then using a libfreetype from the system which was compiled against its system libbz2. This may "just work" but mostly only by luck.

This was less of a problem when this mechanism was used only for a handful of packages that didn't live somewhere deep in the package dependency graph (the old configure.ac checked for these packages in more or less arbitrary order, and that is still the case unfortunately.

I don't agree with your idea of bundling a bunch of packages together but I'm not sure I understand it either.

Well, perhaps bundling is a wrong word, but the idea is that once you removed a lot of stuff from Sage, you need means to build few missing pieces in a coherent way. The complexity of doing this inside Sage is too much, in particular due to inflexible and a bit broken dependency resolution we have.

I think there are a couple things to do here:

Using the freetype/bz2 example again: if we add a spkg-configure.m4 for one package, we should also do so for all its build dependencies. That way, assuming they are detected properly, it will properly use the system packages for the entire dependency tree rooted at freetype.

I don't see why this always be the case. E.g. our current libgd is pretty old, and it does not need dependencies as new as provided by Sage (e.g. bz2). So the host system might have its own libgd, matching our version requirements, and spkg-configure.m4 would tell Sage to stay with the system's libgd. Yet, a newer libgd dependency, bz2, required by Sage for some reason, gets built by Sage. Oops, now we have system's libgd that might get mis-linked to Sage's bz2, or, worse, stuff gets broken during loading at runtime (or, even worse, gets subtly broken and result in wrong results). This is really playing with fire.

Often (freetype is a good example) Sage dependencies get updated just because something breaks on version N of BlaFoo OS, without much regard to whether this might break Sage on version N-1 of BlaFoo OS (often, but by no means always, it is meant to continue working...).

comment:6 Changed 6 months ago by dimpase

  • Description modified (diff)
  • Summary changed from spkg-configure.m4 for freetype to spkg-configure.m4 for freetype and its dependencies

Thinking more about it, I should take a part of my comments above back - I have not quite understood the whole scheme of things. It now seems to me that as soon as an spkg blah gets an spkg-configure.m4, all of its dependencies must have get it too, and then autoconf would do its magic just fine.

That is, for freetype this means that also libpng and bzip2 must also get spkg-configure.m4's. Thus the title change.

comment:7 follow-up: Changed 6 months ago by dimpase

  • Authors set to Dima Pasechnik

Here is 1st try for bzip2, appears working:

SAGE_SPKG_CONFIGURE([bzip2], [
    AC_SEARCH_LIBS([BZ2_bzCompress], [bz2], [], [sage_spkg_install_bzip2=yes])
    AC_CHECK_HEADER(bzlib.h, [], [sage_spkg_install_bzip2=yes])
    AC_CHECK_PROG(bzip2, [break], [sage_spkg_install_bzip2=yes])
])

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

Replying to dimpase:

Replying to embray:

Using the freetype/bz2 example again: if we add a spkg-configure.m4 for one package, we should also do so for all its build dependencies. That way, assuming they are detected properly, it will properly use the system packages for the entire dependency tree rooted at freetype.

I don't see why this always be the case. E.g. our current libgd is pretty old, and it does not need dependencies as new as provided by Sage (e.g. bz2). So the host system might have its own libgd, matching our version requirements, and spkg-configure.m4 would tell Sage to stay with the system's libgd. Yet, a newer libgd dependency, bz2, required by Sage for some reason, gets built by Sage. Oops, now we have system's libgd that might get mis-linked to Sage's bz2, or, worse, stuff gets broken during loading at runtime (or, even worse, gets subtly broken and result in wrong results). This is really playing with fire.

I agree with this ofc. If for some reason Sage (or really some other dependency of Sage) specifically needs some recent-enough libbz2 (for example) then we really need to check for that version on the system (either by actual version number, though better by feature checks if possible). If not found, then we have to build our own copy. And my point is that in that case we then need to also, at least by default, build all other dependencies that depend on that libbz2, regardless whether they can otherwise be found on the system.

comment:9 in reply to: ↑ 7 Changed 6 months ago by embray

Replying to dimpase:

Here is 1st try for bzip2, appears working:

SAGE_SPKG_CONFIGURE([bzip2], [
    AC_SEARCH_LIBS([BZ2_bzCompress], [bz2], [], [sage_spkg_install_bzip2=yes])
    AC_CHECK_HEADER(bzlib.h, [], [sage_spkg_install_bzip2=yes])
    AC_CHECK_PROG(bzip2, [break], [sage_spkg_install_bzip2=yes])
])

That looks good, though I think we need to switch the order of AC_CHECK_HEADER and AC_SEARCH_LIBS. I've found that the result of AC_CHECK_HEADER can be used by AC_SEARCH_LIBS so that it can find the correct headers when building test programs. I'm not entirely sure how that mechanism works, but I did find that to be the case with libffi for example...

comment:10 Changed 6 months ago by dimpase

OK. Do you think we need to check the version of bzip2 (it's been 1.0.6 for 5+ years already)?

comment:11 Changed 6 months ago by embray

I'll make a specific ticket for adding an spkg-configure.m4 for bzip2 and add it as a dependency of this ticket.

I'm testing this out right now. My general feeling about version checks is to avoid it until and unless we know a specific reason we need it.

comment:12 Changed 6 months ago by dimpase

Here the reason would be to avoid nasty surprises for people installing Sage in weird places.

Actually, while I was at it

$ bzip2 --version 2>&1 | sed -n -e 's/bzip2.* Version *\([[0-9]]*\.[[0-9]]*\.[[0-9]]*\).*/\1/p'

outputs 1.0.6, as needed. (then one can steal the rest of version-checking from spkg-configure.m4 of patch spkg, say, to check the version).

comment:13 follow-up: Changed 6 months ago by embray

I'm not sure what nasty surprises you have in mind. I don't think many people use this package directly.

comment:14 Changed 6 months ago by embray

  • Dependencies set to #27182

Forgot to add here: #27182

comment:15 Changed 6 months ago by embray

  • Dependencies changed from #27182 to #27182 #27186

comment:16 in reply to: ↑ 13 Changed 6 months ago by dimpase

Replying to embray:

I'm not sure what nasty surprises you have in mind. I don't think many people use this package directly.

Somebody starts installing Sage on an HPC cluster with typical semi-obsolete 10 years-old Linux version, and it fails as it picks up system's bzlib2 from previous century...

comment:17 Changed 6 months ago by embray

I mean, that's fine. If it doesn't pick it up then it doesn't, and installs Sage's instead.

comment:18 Changed 5 months ago by dimpase

  • Keywords spkg-configure added

comment:19 Changed 4 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:20 Changed 3 months ago by dimpase

  • Branch set to u/dimpase/packages/freetype-config
  • Commit set to d09cdd18e73bc6900a537db04f93fc3187c92d12

New commits:

d20b81bspkg-configure for libpng
2f5aaa7don't check unneeded requirements for system libpng
d09cdd1a straightforward version with pkg-config only

comment:21 Changed 3 months ago by dimpase

libgd configure needs --with-freetype=yes if one goes for system's freetype, instead of --with-freetype="$SAGE_LOCAL".

comment:22 Changed 3 months ago by git

  • Commit changed from d09cdd18e73bc6900a537db04f93fc3187c92d12 to a0422aec4197a21cf088a75f6cf168571bb6992b

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

a0422aeadd SAGE_FREETYPE_PREFIX for libgd

comment:23 Changed 3 months ago by dimpase

  • Status changed from new to needs_review

comment:24 Changed 3 months ago by dimpase

  • Dependencies changed from #27182 #27186 to #27186
  • Status changed from needs_review to needs_work
  • Summary changed from spkg-configure.m4 for freetype and its dependencies to spkg-configure.m4 for freetype

comment:25 Changed 3 months ago by dimpase

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

comment:26 Changed 3 months ago by git

  • Commit changed from a0422aec4197a21cf088a75f6cf168571bb6992b to 3e1c041f69fc11fec9ff1fa645ddb0c38006b5c7

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
f689d8da straightforward version with pkg-config only
3e1c041add SAGE_FREETYPE_PREFIX for libgd

comment:27 Changed 2 months ago by git

  • Commit changed from 3e1c041f69fc11fec9ff1fa645ddb0c38006b5c7 to 9eeaa56f37f863635edd9edc02a9bfa98bc47517

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
61daadda straightforward version with pkg-config only
9eeaa56add SAGE_FREETYPE_PREFIX for libgd

comment:28 Changed 2 months ago by dimpase

  • Cc mkoeppe added

can we get this through? (I deliberately not do a configure bump here, to lessen the rebasing hell...)

comment:29 Changed 2 months ago by dimpase

  • Cc mjo added

comment:30 Changed 2 months ago by fbissey

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

LGTM

comment:31 Changed 2 months ago by dimpase

Thanks! And how about its dependency, #27186 ?

comment:32 Changed 2 months ago by dimpase

Please merge together with #27825

comment:33 Changed 2 months ago by embray

  • Component changed from packages: standard to build: configure
  • Keywords freetype added
  • Status changed from positive_review to needs_review

Please never set these to positive review until I've had a chance to OK them on Cygwin.

comment:34 Changed 2 months ago by embray

I don't like this:

+    if test x$sage_spkg_install_freetype = xyes; then
+      AC_SUBST(SAGE_FREETYPE_PREFIX, ['$SAGE_LOCAL'])
+    else
+      AC_SUBST(SAGE_FREETYPE_PREFIX, [yes])
+    fi

I think if we need this variable at all (which it seems we do currently for building libgd) it should either be an actual filesystem path or empty for consistency's sake. If some random package (like again, libgd) requires you to pass --with-freetype=yes that should be handled in its spkg-install, e.g. like

if [ -n "$SAGE_FREETYPE_PREFIX" ]; then
    CONFIGURE_LIBGD="$CONFIGURE_LIBGD --with-freetype=$SAGE_FREETYPE_PREFIX"
else
    CONFIGURE_LIBGD="$CONFIGURE_LIBGD --with-freetype=yes"
fi

or something along those lines.

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

comment:35 Changed 2 months ago by dimpase

Yep, I agree that it's inconsistent with way such an inconsistency is dealt with in Flint, I'll fix this.

comment:36 Changed 2 months ago by embray

I also noticed some inconsistencies in the formatting of some of the messages, as compared to the stuff in #27822 since I change the messages in those ticket to be more consistent with the style of other autoconf messages (e.g. using mostly lower-case, like "no" instead of "No."). This is just trivial nitpick though. I can fix those up if you want.

comment:37 Changed 2 months ago by dimpase

Please do the fixes (and do not forget about mpfr/mpc/ntl bunch, #27822 and #27265, which is currently not moving forward, too).

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

comment:38 Changed 8 weeks ago by embray

  • Branch changed from u/dimpase/packages/freetype-config to public/packages/freetype-config
  • Commit changed from 9eeaa56f37f863635edd9edc02a9bfa98bc47517 to 8b2cf94e4d041d0bbe4d27bf98af5c7dbbed1c59
  • Reviewers changed from François Bissey to François Bissey, Erik Bray

Here are some minor updates that incorporate my review comments from earlier in this ticket, and a few other minor tweaks that seemed worth including. Please review.


New commits:

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

comment:39 Changed 8 weeks ago by dimpase

this ought to be rebased over 8.8.beta6...

comment:40 Changed 7 weeks ago by dimpase

  • Milestone changed from sage-8.8 to sage-duplicate/invalid/wontfix
  • Status changed from needs_review to positive_review

OK, this looks good, I've cherry-picked these into u/dimpase/packages/libgd-config. Let's continue on #27825.

comment:41 Changed 7 weeks ago by dimpase

ideally, we should be adding an extra parameter to SAGE_SPKG_CONFIGURE to list dependencies that must be verified to come from the system, instead of this monotonous copypastes for these checks.

(It could even be the 2nd parameter, and we then mass-edit all the spkg-configure.m4 to make use of it)

comment:42 Changed 6 weeks ago by embray

It doesn't necessarily have to be an argument to SAGE_SPKG_CONFIGURE, although that could be nice. It could also be a macro similar to AC_REQUIRE that you would put at the top. I agree it should be made less repetitive either way.

comment:43 Changed 10 days ago by chapoton

  • Resolution set to duplicate
  • Status changed from positive_review to closed

comment:44 Changed 7 days ago by slelievre

  • Description modified (diff)
Note: See TracTickets for help on using tickets.