Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spkg-configure.m4 for freetype #27168

Closed
dimpase opened this issue Jan 29, 2019 · 53 comments
Closed

spkg-configure.m4 for freetype #27168

dimpase opened this issue Jan 29, 2019 · 53 comments

Comments

@dimpase
Copy link
Member

dimpase commented Jan 29, 2019

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.

Depends on #27186

CC: @embray @mkoeppe @orlitzky

Component: build: configure

Keywords: spkg-configure freetype

Author: Dima Pasechnik

Branch/Commit: public/packages/freetype-config @ 8b2cf94

Reviewer: François Bissey, Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/27168

@dimpase dimpase added this to the sage-8.7 milestone Jan 29, 2019
@embray
Copy link
Contributor

embray commented Jan 29, 2019

comment:1

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

@embray embray changed the title spkg-configure.m4 for more spkg libraries, e.g. freetype spkg-configure.m4 for freetype Jan 29, 2019
@embray
Copy link
Contributor

embray commented Jan 29, 2019

comment:2

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.

@dimpase
Copy link
Member Author

dimpase commented Jan 30, 2019

comment:3

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.

@embray
Copy link
Contributor

embray commented Jan 30, 2019

comment:4

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.

@dimpase
Copy link
Member Author

dimpase commented Jan 30, 2019

comment:5

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...).

@dimpase
Copy link
Member Author

dimpase commented Jan 30, 2019

comment:6

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.

@dimpase

This comment has been minimized.

@dimpase dimpase changed the title spkg-configure.m4 for freetype spkg-configure.m4 for freetype and its dependencies Jan 30, 2019
@dimpase
Copy link
Member Author

dimpase commented Jan 31, 2019

comment:7

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])
])

@dimpase
Copy link
Member Author

dimpase commented Jan 31, 2019

Author: Dima Pasechnik

@embray
Copy link
Contributor

embray commented Jan 31, 2019

comment:8

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.

@embray
Copy link
Contributor

embray commented Jan 31, 2019

comment:9

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...

@dimpase
Copy link
Member Author

dimpase commented Jan 31, 2019

comment:10

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

@embray
Copy link
Contributor

embray commented Jan 31, 2019

comment:11

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.

@dimpase
Copy link
Member Author

dimpase commented Jan 31, 2019

comment:12

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).

@embray
Copy link
Contributor

embray commented Jan 31, 2019

comment:13

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

@embray
Copy link
Contributor

embray commented Jan 31, 2019

comment:14

Forgot to add here: #27182

@embray
Copy link
Contributor

embray commented Jan 31, 2019

Dependencies: #27182

@embray
Copy link
Contributor

embray commented Jan 31, 2019

Changed dependencies from #27182 to #27182 #27186

@dimpase
Copy link
Member Author

dimpase commented Jan 31, 2019

comment:16

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...

@embray
Copy link
Contributor

embray commented Jan 31, 2019

comment:17

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

@embray
Copy link
Contributor

embray commented Mar 25, 2019

comment:19

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

@embray embray modified the milestones: sage-8.7, sage-8.8 Mar 25, 2019
@dimpase
Copy link
Member Author

dimpase commented May 15, 2019

comment:31

Thanks! And how about its dependency, #27186 ?

@dimpase
Copy link
Member Author

dimpase commented May 15, 2019

comment:32

Please merge together with #27825

@embray
Copy link
Contributor

embray commented May 17, 2019

Changed keywords from none to spkg-configure freetype

@embray
Copy link
Contributor

embray commented May 17, 2019

comment:33

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

@embray
Copy link
Contributor

embray commented May 17, 2019

comment:34

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.

@dimpase
Copy link
Member Author

dimpase commented May 17, 2019

comment:35

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

@embray
Copy link
Contributor

embray commented May 17, 2019

comment:36

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.

@dimpase
Copy link
Member Author

dimpase commented May 17, 2019

comment:37

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

@embray
Copy link
Contributor

embray commented May 23, 2019

comment:38

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

@embray
Copy link
Contributor

embray commented May 23, 2019

Changed commit from 9eeaa56 to 8b2cf94

@embray
Copy link
Contributor

embray commented May 23, 2019

Changed reviewer from François Bissey to François Bissey, Erik Bray

@embray
Copy link
Contributor

embray commented May 23, 2019

@dimpase
Copy link
Member Author

dimpase commented May 23, 2019

comment:39

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

@dimpase
Copy link
Member Author

dimpase commented May 24, 2019

comment:40

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

@dimpase dimpase removed this from the sage-8.8 milestone May 24, 2019
@dimpase
Copy link
Member Author

dimpase commented May 25, 2019

comment:41

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)

@embray
Copy link
Contributor

embray commented May 31, 2019

comment:42

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.

@slel

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants