sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.39k stars 473 forks source link

Update GLPK to 4.55 #12703

Closed 83660e46-0051-498b-a8c1-f7a7bd232b5a closed 9 years ago

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 12 years ago

Old versions (at least till 4.47) fail to build when link-time optimization (LTO, gcc -flto ...) is enabled.

While configure shows an error message, it continues normally and doesn't exit with an error. Only later during the build, a shell syntax error caused by libtool makes the build finally fail.

New versions have been re-autotoolized with more recent autotools.

New versions also unconditionally use their own zlib, this is circumvented by patching in the spirit of what Red Hat does.

Finally we let GLPK build shared lib on Cygwin.

Upstream tarball:

Upstream: Reported upstream. Developers acknowledge bug.

CC: @nathanncohen @simon-king-jena @tscrim

Component: packages: standard

Keywords: link time optimization nm glpsol spkg upgrade update

Author: Leif Leonhardy, Jean-Pierre Flori

Branch: 827eaa3

Reviewer: Nathann Cohen, Volker Braun

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

jpflori commented 9 years ago
comment:50

And this is nm version:

% nm --version                                                
GNU nm (GNU Binutils for Debian) 2.24.90.20141023
Copyright (C) 2014 Free Software Foundation, Inc.
Ce logiciel est libre; vous pouvez le redistribuer selon les termes de la
version 3 de la licence GNU General Public License ou (à votre discrétion
jpflori commented 9 years ago
comment:51

Ok, it seems to be a similar issue as in:

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:52

Ah, so it's the French one that doesn't work! B)

(Not sure what "cannot run sed ..." is supposed to mean.)

jpflori commented 9 years ago
comment:53

Yeah, that's boring but out of our hands. I'll just post my git branch for 4.55 including Cygwin shared lib generation and use of system zlib here tomorrow.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:54

FWIW, with vanilla GLPK 4.55, CFLAGS=-flto works for me with GCC 4.7.4, but fails with GCC 4.9.1 (both with binutils 2.22).

There've been major changes to LTO over time IIRC, so while the "new" errors are annoying, they're probably not too surprising.

But it's a minor issue anyway, and apparently meanwhile modifying CFLAGS is sufficient to get around the failure, as opposed to modifying the sources which previously was necessary.

Will your "spkg" be based on my previous one? (Unfortunately my home directory is no longer accessible via HTTP, so you'd have to use scp instead, say.)

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 9 years ago
comment:55

Replying to @nexttime:

FWIW, with vanilla GLPK 4.55, CFLAGS=-flto works for me with GCC 4.7.4, but fails with GCC 4.9.1 (both with binutils 2.22).

Just for the record: With GCC 4.8.3, -flto is sufficient as well.

There've been major changes to LTO over time IIRC, so while the "new" errors are annoying, they're probably not too surprising.

jpflori commented 9 years ago

Branch: u/jpflori/ticket/12703

jpflori commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,34 +1,12 @@
-Even the latest upstream version (4.47) fails to build when link-time optimization (LTO, `gcc -flto ...`) is enabled.
+Old versions (at least till 4.47) fail to build when link-time optimization (LTO, `gcc -flto ...`) is enabled.

 While `configure` shows an error message, it continues normally and doesn't exit with an error.  Only later during the build, a shell syntax error caused by `libtool` makes the build finally fail.

-The solution is to re-autotoolize the package with more recent autotools.
+New versions have been re-autotoolized with more recent autotools.

-I have an spkg with updated autogenerated files which also upgrades GLPK within Sage to the latest stable version, 4.47.
+New versions also unconditionally use their own zlib, this is circumvented by patching in the spirit of what Red Hat does.

----
+Finally we let GLPK build shared lib on Cygwin.

-**New spkg:** [http://boxen.math.washington.edu/home/leif/Sage/spkgs/glpk-4.47.p1.spkg](http://boxen.math.washington.edu/home/leif/Sage/spkgs/glpk-4.47.p1.spkg)
-
-**md5sum:** `090ee5d398582db531b27e49deab6749  glpk-4.47.p1.spkg`
-
-
-### glpk-4.47.p1 (Leif Leonhardy, March 22nd 2012)
-* #12703: Make GLPK work with LTO (`gcc -flto ...`).
-  Export CPPFLAGS, since `sage-env` currently *doesn't*.
-
-
-### glpk-4.47.p0 (Leif Leonhardy, March 19th 2012)
-* #12703: Make GLPK work with LTO (`gcc -flto ...`).
-  This just involves updating the lib- and autotools files,
-  but thereby upgrading to the latest version (4.47) shouldn't
-  hurt either.
-  Note that the `src/` directory now isn't vanilla, since upstream
-  hasn't yet fixed the bug (i.e., updated these files).
-  The "real" source files in contrast of course *are*.
-  See "Special Update/Build Instructions" above on how to update
-  the files of a fresh upstream tarball if necessary.
-* Cleaned up `spkg-install` and `spkg-check` (and this file ;-)
-* Running the test suite now uses `$MAKE` instead of `make` as well.
-* `CPPFLAGS` and `LDFLAGS` no longer get overwritten in `spkg-install`.
-
+Upstream tarball:
+* http://ftp.gnu.org/gnu/glpk/glpk-4.55.tar.gz
jpflori commented 9 years ago

New commits:

57682f2GLPK update.
758b53aUpdate GLPK to 4.55. Keep vanilla upstream tarball.
jpflori commented 9 years ago

Changed author from Leif Leonhardy to Leif Leonhardy, Jean-Pierre Flori

jpflori commented 9 years ago

Commit: 758b53a

kiwifb commented 9 years ago
comment:57

I cannot comment on the build part but the commit addresses a good number of doctest failures I have seen in sage-on-gentoo with glpk 4.54. Only one bit really need to be changed:

--- a/src/sage/numerical/mip.pyx
+++ b/src/sage/numerical/mip.pyx
@@ -2387,12 +2387,12 @@ cdef class MixedIntegerLinearProgram(SageObject):
             sage: b = p.get_backend()
             sage: b.solver_parameter("simplex_or_intopt", "simplex_only")
             sage: b.solver_parameter("verbosity_simplex", "GLP_MSG_ALL")
-            sage: p.solve()  # tol 0.00001
+            sage: p.solve()  # rel tol 3e-2
             GLPK Simplex Optimizer, v4.44
             2 rows, 2 columns, 4 non-zeros
             *     0: obj =   7.000000000e+00  infeas =  0.000e+00 (0)
             *     2: obj =   9.400000000e+00  infeas =  0.000e+00 (0)
-            OPTIMAL SOLUTION FOUND
+            OPTIMAL LP SOLUTION FOUND
             9.4
         """
         return self._backend

You still have "GLPK Simplex Optimizer, v4.44" so it seems you have forgotten that bit. I doubt it would be caught by the tolerance. My own request would be for the version to be tested as "4....".

jdemeyer commented 9 years ago
comment:58

Replying to @kiwifb:

I doubt it would be caught by the tolerance.

It works: the relative tolerance of 3e-2 is sufficient to consider 4.44 and 4.55 equal.

My own request would be for the version to be tested as "4....".

Hiding floats under ... while testing with tolerance is not supported.

jdemeyer commented 9 years ago
comment:59

The statements

* As of version 4.55, zlib is included in the GLPK sources, and *that*
version is unconditionally compiled into the GLPK library, so there's
no "external" dependency on it

and

* zlib.patch: don't build the included zlib library.

seem to contradict eachother.

jdemeyer commented 9 years ago
comment:60

Also, CFALGS surely is a typo?

jdemeyer commented 9 years ago
comment:61

If you add

# Exit on error
set -e

then remove all other error checking :-)

kiwifb commented 9 years ago
comment:62

Replying to @jdemeyer:

Replying to @kiwifb:

I doubt it would be caught by the tolerance.

It works: the relative tolerance of 3e-2 is sufficient to consider 4.44 and 4.55 equal.

I think I misunderstood the tolerance. I always thought it was absolute tolerance $\Delta$x but you are saying it is relative as in $\Delta$x/x, then yes it makes it pass but it probably still should be changed.

My own request would be for the version to be tested as "4....".

Hiding floats under ... while testing with tolerance is not supported.

Now I remember about that.

jdemeyer commented 9 years ago
comment:63

I doubt that

# The following is an easy way to prevent autotools to regenerate its files.
touch *

actually works as advertised. You're assuming that touch * will never cause configure.ac to have a newer timestamp than configure.

jdemeyer commented 9 years ago
comment:64

With the philosophy that packages should be as close to upstream as possible, should we really add the patch the disable compiling the included zlib?

jpflori commented 9 years ago
comment:65

Replying to @jdemeyer:

I doubt that

# The following is an easy way to prevent autotools to regenerate its files.
touch *

actually works as advertised. You're assuming that touch * will never cause configure.ac to have a newer timestamp than configure.

It works for me. It could indeed fail on system where touch * will de very slow and the timestamp very precise. Trying to touch the right files by hand is a nightmare, there are just too many dependencies. I've tried reading the magical overly complicated Makefile generated by autotools but always seemed to miss something.

jpflori commented 9 years ago
comment:66

Replying to @jdemeyer:

With the philosophy that packages should be as close to upstream as possible, should we really add the patch the disable compiling the included zlib?

I think we should not compile the included zlib like we should not compile the various versions of GMP included in a bunch of packages.

jdemeyer commented 9 years ago
comment:67

What does the "Reported upstream. Developers acknowledge bug." refer to and where is the upstream report?

jpflori commented 9 years ago
comment:68

Replying to @jdemeyer:

What does the "Reported upstream. Developers acknowledge bug." refer to and where is the upstream report?

I linked some thread from glpk list here. That should be the report. The upstream fix is that the autotools stuff was regenerated since 4.47.

jpflori commented 9 years ago
comment:69

Replying to @jpflori:

Replying to @jdemeyer:

With the philosophy that packages should be as close to upstream as possible, should we really add the patch the disable compiling the included zlib?

I think we should not compile the included zlib like we should not compile the various versions of GMP included in a bunch of packages.

Also note that we ship a vanilla tarball.

jpflori commented 9 years ago
comment:70

Replying to @jdemeyer:

The statements

* As of version 4.55, zlib is included in the GLPK sources, and *that*
version is unconditionally compiled into the GLPK library, so there's
no "external" dependency on it

and

* zlib.patch: don't build the included zlib library.

seem to contradict eachother.

Indeed, I'll change the "unconditionally" by "there is no configure flag to prevent it from being" and remove the thing about dependency.

kiwifb commented 9 years ago
comment:71

I think if you are going to modify configure and co. you need to generate a new tarball for sage. Patching configure directly is not really sustainable and touching may need to be done with more care. I am quite surprised you weren't bitten by it so far considering that configure.ac comes after configure in most ordering.

jpflori commented 9 years ago
comment:72

Replying to @kiwifb:

I think if you are going to modify configure and co. you need to generate a new tarball for sage. Patching configure directly is not really sustainable and touching may need to be done with more care. I am quite surprised you weren't bitten by it so far considering that configure.ac comes after configure in most ordering.

That's why I touch *. Ensuring that configure.ac was patch before configure and Makefiles.am before Makefiles.in and then playing with m4 files timestamp just drove me crazy.

jdemeyer commented 9 years ago
comment:73

Replying to @jpflori:

Ensuring that configure.ac was patch before configure and Makefiles.am before Makefiles.in and then playing with m4 files timestamp just drove me crazy.

I cannot really imagine that it's that bad: you only need to touch a few files...

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 758b53a to 4afb953

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

4afb953A few fixes for GLPK update to 4.55.
jdemeyer commented 9 years ago
comment:75

In the "dependencies" part", you can just write

* zlib

(there is already a comment about zlib in the "Special Update/Build Instructions")

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

b3bf953Refrain autotools from regenerating its files when configuring GLPK.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 4afb953 to b3bf953

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

2776a68zlib is a dependency of GLPK as we don't use the upstream shipped version.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from b3bf953 to 2776a68

jpflori commented 9 years ago
comment:79

FYI, the cygwin shared lib stuff should go upstream in one form or another. See:

jpflori commented 9 years ago
comment:80

There is something wrong on Cygwin with the shared lib stuff....

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 2776a68 to 468a7d8

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

0554899Merge remote-tracking branch 'trac/develop' into ticket/12703
468a7d8Fix GLPK tweak for Cygwin.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 468a7d8 to 8b77569

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

8b77569Fix GLPK tweak for Cygwin.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

584cbd6Fix stupid typo in previous commit for GLPK on Cygwin.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 8b77569 to 584cbd6

jpflori commented 9 years ago
comment:84

Ok, this is now fixed and confirmed to work on Cygwin and ppc64. Sorry about the forced push, I included some spurious autotools stuff in the previous commit.

vbraun commented 9 years ago
comment:86

The -0.0 looks like numerical noise, its probably better to use # abs tol 1e-15 or so. Rest looks good to me.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

c333b09Merge remote-tracking branch 'trac/develop' into ticket/12703
9ecce27Modify GLPK test involving numerical noise.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 584cbd6 to 9ecce27

jpflori commented 9 years ago
comment:88

Tests modified.

vbraun commented 9 years ago

Changed reviewer from Nathann Cohen to Nathann Cohen, Volker Braun

vbraun commented 9 years ago
comment:90
sage -t --long --warn-long 40.1 src/sage/numerical/mip.pyx
**********************************************************************
File "src/sage/numerical/mip.pyx", line 2097, in sage.numerical.mip.MixedIntegerLinearProgram.number_of_variables.solve
Failed example:
    round(x[1],6)
Expected:
    0.0
Got:
    -0.0
**********************************************************************
1 item had failures:
   1 of  24 in sage.numerical.mip.MixedIntegerLinearProgram.number_of_variables.solve
    [493 tests, 1 failure, 0.88 s]
----------------------------------------------------------------------
sage -t --long --warn-long 40.1 src/sage/numerical/mip.pyx  # 1 doctest failed
----------------------------------------------------------------------