Opened 8 years ago

Closed 8 years ago

#12840 closed enhancement (fixed)

update M4RI to newest upstream release

Reported by: malb Owned by: tbd
Priority: major Milestone: sage-5.3
Component: packages: standard Keywords:
Cc: Merged in: sage-5.3.beta0
Authors: Martin Albrecht, John Palmieri Reviewers: Simon King, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: to be merged with #12841, #13109 Stopgaps:

Attachments (3)

trac_12840_m4ri_new_version.patch (6.0 KB) - added by malb 8 years ago.
trac_12840_m4ri_png.patch (764 bytes) - added by malb 8 years ago.
trac_12840-rebased-on-13109.patch (1.4 KB) - added by jhpalmieri 8 years ago.

Download all attachments as: .zip

Change History (66)

comment:1 Changed 8 years ago by malb

  • Description modified (diff)

comment:2 Changed 8 years ago by malb

  • Description modified (diff)

comment:3 Changed 8 years ago by malb

  • Description modified (diff)

comment:4 Changed 8 years ago by malb

  • Dependencies set to #12841

comment:5 Changed 8 years ago by malb

  • Status changed from new to needs_review

comment:6 Changed 8 years ago by malb

Last edited 8 years ago by malb (previous) (diff)

Changed 8 years ago by malb

comment:7 Changed 8 years ago by malb

Updated the patch because I forgot to raise a ZeroDivisionError in __invert__. Fixed now.

comment:8 Changed 8 years ago by SimonKing

After applying the patch and the spkg and rebuilding the polybori spkg, sage -br failed with

gcc -pthread -shared -L/mnt/local/king/SAGE/stable/sage-5.0.beta13/local/lib build/temp.linux-x86_64-2.7/sage/matrix/matrix_integer_dense.o -L/mnt/local/king/SAGE/stable/sage-5.0.beta13/local/lib -L/mnt/local/king/SAGE/stable/sage-5.0.beta13/local/lib -lcsage -liml -lpari -lm -lgmp -lcblas -latlas -lstdc++ -lntl -lpython2.7 -o build/lib.linux-x86_64-2.7/sage/matrix/matrix_integer_dense.so
error: command 'gcc' failed with exit status 1
Error installing modified sage library code.

No error message, I am afraid.

comment:9 Changed 8 years ago by SimonKing

PS: That was with sage-5.0.beta13 plus a lot of patches, but none of them changing matrices.

comment:10 Changed 8 years ago by SimonKing

And repeating sage -br, I finally got a proper error:

In file included from /mnt/local/king/SAGE/stable/sage-5.0.beta13/local/include/m4rie/m4rie.h:56,
                 from sage/matrix/matrix_mod2e_dense.cpp:240:
/mnt/local/king/SAGE/stable/sage-5.0.beta13/local/include/m4rie/permutation.h:30:30: error: m4ri/permutation.h: Datei oder Verzeichnis nicht gefunden
error: command 'gcc' failed with exit status 1
Error installing modified sage library code.

comment:11 Changed 8 years ago by SimonKing

PS: permutation.h seems not to be present in the sources.

comment:12 Changed 8 years ago by SimonKing

PS: The same error occurs when trying to rebuild libm4rie spkg: M4RIE expects permutation.h

comment:13 Changed 8 years ago by malb

Yes, #12841 is a dependency of this ticket.

comment:14 Changed 8 years ago by SimonKing

Ouch. I missed #12841. Sorry.

comment:15 Changed 8 years ago by SimonKing

Am I right that both spkgs have to be installed, but #12840 goes before #12841?

comment:16 follow-up: Changed 8 years ago by SimonKing

Yippie! Applying the patches from #12840 and #12841, installing the new m4ri spkg from here, then installing the new m4rie spkg from #12841 (why is it on a different ticket? Can one live without the other?), re-installing polybori and finally sage -br, Sage starts!

comment:17 in reply to: ↑ 16 Changed 8 years ago by malb

Replying to SimonKing:

Yippie! Applying the patches from #12840 and #12841, installing the new m4ri spkg from here, then installing the new m4rie spkg from #12841 (why is it on a different ticket? Can one live without the other?),

No, you're right, they are co-dependent.

comment:18 follow-up: Changed 8 years ago by SimonKing

First of all: make ptestlong works with the two new spkgs (even though I have a few other patches applied...).

Here is a small timing.

Vanilla Sage-5.0.rc0:

sage: K = GF(2)
sage: %time A = random_matrix(K,20000,20000)
CPU times: user 0.29 s, sys: 0.02 s, total: 0.31 s
Wall time: 0.31 s
sage: %time B = random_matrix(K,20000,20000)
CPU times: user 0.23 s, sys: 0.03 s, total: 0.26 s
Wall time: 0.27 s
sage: %time (A*B).rank()
CPU times: user 11.70 s, sys: 0.05 s, total: 11.75 s
Wall time: 11.81 s
19937

Sage-5.0.beta13 with the new spkgs (and some other patches that shouldn't affect the timing for matrix multiplication):

sage: K = GF(2)
sage: %time A = random_matrix(K,20000,20000)
CPU times: user 0.30 s, sys: 0.03 s, total: 0.33 s
Wall time: 0.33 s
sage: %time B = random_matrix(K,20000,20000)
CPU times: user 0.32 s, sys: 0.01 s, total: 0.33 s
Wall time: 0.33 s
sage: %time (A*B).rank()
CPU times: user 11.70 s, sys: 0.10 s, total: 11.80 s
Wall time: 11.83 s
19937

Well, at least there is no slow-down...

Do you claim that the new version of M4RI is faster? You do claim a speed-up for M4RIE, on #12841.

comment:19 in reply to: ↑ 18 Changed 8 years ago by malb

Replying to SimonKing:

Do you claim that the new version of M4RI is faster? You do claim a speed-up for M4RIE, on #12841.

No, M4RI shouldn't have changed (at least the stuff, only bugfixes & refactoring.

That said, there's a new function for inverting upper triangular matrices which is much faster than the generic method, but it's not exposed yet. I need to implement the same for lower triangular matrices, then we can speed up inversions in general.

Furthermore, M4RI ships its own PNG reader/writer now which is much faster than what we have in Sage and uses much less memory. Again, it's not used yet. This patch just gets the new version in.

comment:20 Changed 8 years ago by SimonKing

  • Reviewers set to Simon King
  • Status changed from needs_review to positive_review

Since make ptestlong passes and the hg repository in the package is fine and SPKG.txt is up to date, I think I can give a positive review. However, I only tested on one platform. So, if the release manager believes that this is not enough, then please speak up!

comment:21 Changed 8 years ago by jdemeyer

  • Dependencies changed from #12841 to to be merged wth #12841

comment:22 Changed 8 years ago by jdemeyer

  • Dependencies changed from to be merged wth #12841 to to be merged with #12841

comment:23 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This fails to install on the Skynet machine "eno" (Fedora 16 x86_64):

/bin/sh ./libtool --tag=CC   --mode=link gcc -std=gnu99 -mmmx -msse -msse2 -msse3   -g -fPIC -Wall -pedantic -O2 -release 0.0.20111203 -no
-undefined  -o libm4ri.la -rpath /home/buildbot/build/sage/eno-1/eno_full/build/sage-5.1.beta1/local/lib brilliantrussian.lo misc.lo mzd.l
o graycode.lo strassen.lo mzp.lo triangular.lo triangular_russian.lo ple.lo ple_russian.lo solve.lo echelonform.lo mmc.lo debug_dump.lo io
.lo -lm -lpng
libtool: link: gcc -shared  -fPIC -DPIC  .libs/brilliantrussian.o .libs/misc.o .libs/mzd.o .libs/graycode.o .libs/strassen.o .libs/mzp.o .
libs/triangular.o .libs/triangular_russian.o .libs/ple.o .libs/ple_russian.o .libs/solve.o .libs/echelonform.o .libs/mmc.o .libs/debug_dum
p.o .libs/io.o   -lm -lpng  -mmmx -msse -msse2 -msse3 -O2   -Wl,-soname -Wl,libm4ri-0.0.20111203.so -o .libs/libm4ri-0.0.20111203.so
/usr/bin/ld: cannot find -lpng
collect2: ld returned 1 exit status
make[1]: *** [libm4ri.la] Error 1
make[1]: Leaving directory `/home/buildbot/build/sage/eno-1/eno_full/build/sage-5.1.beta1/spkg/build/libm4ri-20120415/src'
make: *** [all] Error 2
Error building libm4ri
$ ls -l local/lib/libpng*
36k -rw-r--r-- 1 buildbot sage 828k May  8 06:16 local/lib/libpng12.a
4.1k -rwxr-xr-x 1 buildbot sage 1.1k May  8 06:16 local/lib/libpng12.la*
   0 lrwxrwxrwx 1 buildbot sage   18 May  8 06:16 local/lib/libpng12.so -> libpng12.so.0.35.0*
   0 lrwxrwxrwx 1 buildbot sage   18 May  8 06:16 local/lib/libpng12.so.0 -> libpng12.so.0.35.0*
467k -rwxr-xr-x 1 buildbot sage 460k May  8 06:16 local/lib/libpng12.so.0.35.0*

$ ls -l /lib*/libpng* /usr/lib*/libpng*
ls: cannot access /lib*/libpng*: No such file or directory
   0 lrwxrwxrwx 1 root root   16 May  2 11:53 /usr/lib64/libpng.so.3 -> libpng.so.3.49.0*
173k -rwxr-xr-x 1 root root 172k Apr  7 14:33 /usr/lib64/libpng.so.3.49.0*
   0 lrwxrwxrwx 1 root root   18 May  2 11:53 /usr/lib64/libpng12.so.0 -> libpng12.so.0.49.0*
164k -rwxr-xr-x 1 root root 162k Apr  7 14:33 /usr/lib64/libpng12.so.0.49.0*

Note that there is no file /usr/lib64/libpng.so, so obviously there cannot be proper PNG support on this machine. I consider this to be an upstream (not Sage) error.

$ grep -i png spkg/logs/libm4ri-20120415.log
checking whether to build with PNG support... yes
checking for PNG... no
checking for PNG... yes
/bin/sh ./libtool --tag=CC   --mode=link gcc -std=gnu99 -mmmx -msse -msse2 -msse3   -g -fPIC -Wall -pedantic -O2 -release 0.0.20111203 -no-undefined  -o libm4ri.la -rpath /home/buildbot/build/sage/eno-1/eno_full/build/sage-5.1.beta1/local/lib brilliantrussian.lo misc.lo mzd.lo graycode.lo strassen.lo mzp.lo triangular.lo triangular_russian.lo ple.lo ple_russian.lo solve.lo echelonform.lo mmc.lo debug_dump.lo io.lo -lm -lpng
libtool: link: gcc -shared  -fPIC -DPIC  .libs/brilliantrussian.o .libs/misc.o .libs/mzd.o .libs/graycode.o .libs/strassen.o .libs/mzp.o .libs/triangular.o .libs/triangular_russian.o .libs/ple.o .libs/ple_russian.o .libs/solve.o .libs/echelonform.o .libs/mmc.o .libs/debug_dump.o .libs/io.o   -lm -lpng  -mmmx -msse -msse2 -msse3 -O2   -Wl,-soname -Wl,libm4ri-0.0.20111203.so -o .libs/libm4ri-0.0.20111203.so
/usr/bin/ld: cannot find -lpng

comment:24 follow-up: Changed 8 years ago by jdemeyer

Why are you using PKG_CHECK_MODULES() to check for PNG support in configure.ac? Why not AC_CHECK_LIB() or AC_SEARCH_LIBS()?

comment:25 Changed 8 years ago by jdemeyer

Exactly the same problem with -lpng happens on Skynet "cleo" (RHEL 5.3 ia64)

comment:26 in reply to: ↑ 24 Changed 8 years ago by malb

Replying to jdemeyer:

Why are you using PKG_CHECK_MODULES() to check for PNG support in configure.ac? Why not AC_CHECK_LIB() or AC_SEARCH_LIBS()?

We are doing both. We try pkg-config first and if that fails we fall back to AC_CHECK_LIB. In your log above, PKG_CHECK_MODULES() fails to find libpng and AC_CHECK_LIB() finds it.

comment:27 Changed 8 years ago by malb

Note that stand-alone M4RI builds fine on eno.

checking pkg-config is at least version 0.9.0... yes
checking for PNG... no
checking for PNG... no
checking for PNG... no
checking for PNG... no
checking for png_create_write_struct in -lpng... no

So I don't know why it fails when built inside Sage. In any case, Sage ships libpng so we should adapt deps to make M4RI dependent on libpng. I'll provide a patch to that effect.

Changed 8 years ago by malb

comment:28 follow-up: Changed 8 years ago by jdemeyer

Replying to malb:

In your log above, PKG_CHECK_MODULES() fails to find libpng and AC_CHECK_LIB() finds it.

Actually, it's not quite like that. PKG_CHECK_MODULES() found libpng12 but you still try to link with -lpng instead of -lpng12 (the first failure comes from the libpng14 check). AC_CHECK_LIB() isn't called (but it would fail).

comment:29 Changed 8 years ago by malb

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

comment:30 Changed 8 years ago by jdemeyer

I recommend against using PKG_CHECK_MODULES() and would use only AC_SEARCH_LIBS().

comment:31 follow-up: Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The patch you attached is probably good, but that's just part of the problem.

comment:32 in reply to: ↑ 31 ; follow-up: Changed 8 years ago by malb

Replying to jdemeyer:

The patch you attached is probably good, but that's just part of the problem.

Can you elaborate on that? I don't understand.

comment:33 in reply to: ↑ 32 ; follow-up: Changed 8 years ago by jdemeyer

Replying to malb:

Can you elaborate on that? I don't understand.

Well, I tried to explain in http://trac.sagemath.org/sage_trac/ticket/12840#comment:28. The fact that

PKG_CHECK_MODULES([PNG], [libpng12])

succeeds, does not imply that the library file libpng.so exists. I don't see what you're trying to accomplish with those PKG_CHECK_MODULES() checks.

comment:34 Changed 8 years ago by jdemeyer

I can try to write a patch if you like.

comment:35 in reply to: ↑ 33 Changed 8 years ago by malb

Replying to jdemeyer:

Replying to malb:

Can you elaborate on that? I don't understand.

PKG_CHECK_MODULES([PNG], [libpng12])

succeeds, does not imply that the library file libpng.so exists.

Ah, right. It would need -lpng12. Yep, that's a bug. I'll fix that next week-ish.

I don't see what you're trying to accomplish with those PKG_CHECK_MODULES() checks.

I am trying to use pkg-config for what it's intended. Making linking easier.

comment:36 follow-up: Changed 8 years ago by jdemeyer

Maybe you're using it for what it's intended, but not how it's intended.

comment:37 in reply to: ↑ 28 Changed 8 years ago by malb

Replying to jdemeyer:

Replying to malb:

In your log above, PKG_CHECK_MODULES() fails to find libpng and AC_CHECK_LIB() finds it.

Actually, it's not quite like that. PKG_CHECK_MODULES() found libpng12 but you still try to link with -lpng instead of -lpng12 (the first failure comes from the libpng14 check). AC_CHECK_LIB() isn't called (but it would fail).

For some reason I missed this comment earlier, sorry. That makes sense & is an upstream bug which I intend to fix.

comment:38 in reply to: ↑ 36 ; follow-up: Changed 8 years ago by malb

Replying to jdemeyer:

Maybe you're using it for what it's intended, but not how it's intended.

This is a remarkably unhelpful comment.

comment:39 in reply to: ↑ 38 Changed 8 years ago by jdemeyer

Replying to malb:

This is a remarkably unhelpful comment.

I know, but I have never done anything with pkgconfig, so I can't help more.

comment:40 Changed 8 years ago by malb

  • Description modified (diff)

I've updated the SPKG with a new upstream release which should fix the png issue.

http://sage.math.washington.edu/home/malb/spkgs/libm4ri-20120522.spkg

comment:41 Changed 8 years ago by malb

  • Status changed from needs_work to needs_review

comment:42 follow-up: Changed 8 years ago by malb

Could someone please review this. I am looking into updating LinBox and updating M4RIE is a requirement for that (long story involving Givaro).

Last edited 8 years ago by malb (previous) (diff)

comment:43 in reply to: ↑ 42 Changed 8 years ago by SimonKing

Replying to malb:

Could someone please review this. I am looking into updating LinBox and updating M4RIE is a requirement for that (long story involving Givaro).

I could test the code on one or two linuxes. But I couldn't test whether the png problems on eno (as observed by Jeroen) are fixed. Jeroen, could you finish the review?

comment:44 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:45 Changed 8 years ago by malb

  • Description modified (diff)

comment:46 follow-up: Changed 8 years ago by jdemeyer

The previous version installed fine on eno within Sage. It seems the libpng check is totally gone now.

comment:47 in reply to: ↑ 46 Changed 8 years ago by malb

Replying to jdemeyer:

The previous version installed fine on eno within Sage.

It seems the libpng check is totally gone now.

That's strange, it doesn't check?

I had to update SPKG because I didn't add "-I$SAGE_LOCAL/include" before but we need it now because of the PNG dependency. The newest SPKG installs fine on geom.math which doesn't have a system libpng-dev.

comment:48 Changed 8 years ago by vbraun

I've tried this on Fedora 17 (which doesn't have libpng12 either, its up to libpng15 now) and configure works as intended:

checking whether to build with PNG support... yes
checking for pkg-config... /usr/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
checking for PNG... yes

Jeroen, you might have overlooked this part in the log since it comes after the lengthy "checking for cache sizes...".

I'm building on eno right now and if it works I'll set this ticket to positive review.

comment:49 follow-up: Changed 8 years ago by malb

So Fedora has libpng.so then? I am asking because I am not checking for png15 explicitly.

if test "x${want_png}" = "xyes" ; then
   PKG_CHECK_MODULES([PNG], [libpng],
      [have_libpng="yes"; LIBPNG_LIBADD=`pkg-config --libs libpng`],
      [have_libpng="no"])
   if ! test "x${have_libpng}" = "xyes" ; then
      AC_CHECK_LIB([png],
         [png_create_write_struct],
         [have_libpng="yes"; LIBPNG_LIBADD="-lpng"],
         [AC_CHECK_LIB([png14],
            [png_create_write_struct],
            [have_libpng="yes"; LIBPNG_LIBADD="-lpng14"],
            [AC_CHECK_LIB([png12],
               [png_create_write_struct],
               [have_libpng="yes"; LIBPNG_LIBADD="-lpng12"],
               [AC_CHECK_LIB([png10],
                  [png_create_write_struct],
                  [have_libpng="yes"; LIBPNG_LIBADD="-lpng10"],
                  [have_libpng="no"])
               ])
            ])
        ])
   fi
   if test "x${have_libpng}" = "xno" ; then
      AC_MSG_WARN([Can not find a usuable PNG library. Make sure that CPPFLAGS and LDFLAGS are correctly set.])
   fi
fi

comment:50 in reply to: ↑ 49 Changed 8 years ago by malb

Replying to malb:

So Fedora has libpng.so then? I am asking because I am not checking for png15 explicitly. 

Argh, I am stupid. It would find libpng12 as included in Sage. Sorry bout the noise.

comment:51 Changed 8 years ago by vbraun

  • Reviewers changed from Simon King to Simon King, Volker Braun
  • Status changed from needs_review to positive_review

Yes the PKG_CHECK_MODULES test wins in Sage, which is the way it should be. pkg-config is the way to go imho. Though if libpng were to use standard libtool versioning we would live in a better world, too ;-)

In any case, built and tested fine on eno. Positive review.

comment:52 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

Changed 8 years ago by jhpalmieri

comment:53 Changed 8 years ago by jhpalmieri

  • Dependencies changed from to be merged with #12841 to to be merged with #12841, #13109
  • Description modified (diff)
  • Status changed from positive_review to needs_work

The deprecations here should use the format from #13109.

comment:54 Changed 8 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

comment:55 Changed 8 years ago by vbraun

The deprecations are not doctested.

comment:56 Changed 8 years ago by vbraun

  • Authors changed from Martin Albrecht to Martin Albrecht, John Palmieri
  • Status changed from needs_review to positive_review

Hmm deprecations (and any other warnings) don't work in cdef methods. I guess we can't doctest them, for now.

comment:57 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.2 to sage-pending

comment:58 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.3

comment:59 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This needs to be rebased to sage-5.2.rc1.

comment:60 Changed 8 years ago by jhpalmieri

Can you clarify the rebasing issues? I have no problems applying the patches from here and from #12841 on top of sage-5.2.rc1.

comment:61 Changed 8 years ago by vbraun

I'm with John here, I've been carrying the mari/e/givaro/linbox patches in my queue for a while now and there was no breakage in sage-5.2.rc1 as far as I can tell.

comment:62 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to positive_review

My mistake, I only applied the second Sage library patch and that didn't work.

comment:63 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.3.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.