Opened 12 years ago

Closed 6 years ago

Last modified 5 years ago

#748 closed defect (fixed)

update iml to the 1.0.3 release + our patches

Reported by: was Owned by: mabshoff
Priority: major Milestone: sage-5.11
Component: packages: standard Keywords: spkg upgrade iml sd40.5
Cc: jpflori Merged in: sage-5.11.beta2
Authors: Jeroen Demeyer Reviewers: Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14699 Stopgaps:

Attachments (1)

iml-1.0.3.p0.diff (16.2 KB) - added by jdemeyer 6 years ago.

Download all attachments as: .zip

Change History (52)

comment:1 Changed 12 years ago by was

I've reverted the iml package that will be distributed with Sage to version 1.0.1.p6 (from sage-2.8.4) until this gets resolved.

comment:2 Changed 12 years ago by was

The 1.0.1.p8 version is in sage-2.8.5

comment:3 Changed 12 years ago by was

  • Milestone changed from sage-2.8.6 to sage-2.9

comment:4 Changed 12 years ago by mabshoff

  • Summary changed from iml autohell to update iml to the 1.0.2 release + our patches

comment:5 Changed 11 years ago by mabshoff

  • Owner changed from was to mabshoff
  • Status changed from new to assigned

A couple commets:

  • Arne merged the nullspace patch and a couple of the other fixes. Some of the other fixes no longer apply
  • spkg-install does odd things, i.e fall back to non-ATLAS which is no longer needed
  • The SPKG needs some general cleanup
  • on OSX without creating 'repl' the build on the vanilla sources fails

Arne is putting together some 1.0.3 release which we can then test in Sage

Cheers,

Michael

comment:6 Changed 11 years ago by craigcitro

  • Summary changed from update iml to the 1.0.2 release + our patches to [waiting on upstream] update iml to the 1.0.3 release + our patches

comment:7 Changed 10 years ago by jason

  • Status changed from new to needs_info_new

comment:8 Changed 8 years ago by leif

  • Keywords spkg upgrade added
  • Milestone changed from sage-4.7.2 to sage-duplicate/invalid/wontfix
  • Report Upstream set to N/A
  • Resolution set to duplicate
  • Status changed from needs_info to closed

I'm closing this as a duplicate of #9568.

comment:9 Changed 8 years ago by leif

  • Reviewers set to Leif Leonhardy

comment:10 Changed 7 years ago by drkirkby

Is it not more sensible for the duplicated ticket to be closed, rather than the original one? That's the way gcc handle it. If you report something that's a duplicate of an older ticket, the newer ticket gets closed - not the older one.

Dave

comment:11 Changed 7 years ago by kini

I guess it depends on which ticket has the most useful comments / patches and the most relevant people CC'd to it. At least #13027 has an SPKG attached. #9568 and this ticket certainly have more useful comments than #13027, though.

comment:12 Changed 7 years ago by kini

  • Keywords iml added
  • Resolution duplicate deleted
  • Reviewers Leif Leonhardy deleted
  • Status changed from closed to new

I have no objection to reopening this ticket, anyway. In any case, once someone actually solves the problem, all three will be closed.

comment:13 Changed 7 years ago by fbissey

I have a patch for repl problem in sage-on-gentoo but all the autotool chain needs to be regenerated after you apply it. https://github.com/cschwan/sage-on-gentoo/blob/master/sci-libs/iml/files/iml-1.0.3-repl_removal.patch

comment:14 Changed 7 years ago by kini

Thanks, that will help. We cannot run autotools at build time, only at packaging time, since we don't have autotools as a dependency of Sage, nor do we package it in Sage. So rather than including your patch I guess I should apply it, run autotools, and then save *that* diff as a patch to apply at build time.

Before seeing your diff I had solved the repl problem like this:

  • repl/Makefile.am

    diff --git a/repl/Makefile.am b/repl/Makefile.am
    index 4cf001b..7be2e86 100644
    a b  
    11noinst_LTLIBRARIES      = librepl.la
    2 librepl_la_SOURCES   =
     2librepl_la_SOURCES   = dummy.c
    33librepl_la_LIBADD    = @LTLIBOBJS@
    44
  • repl/Makefile.in

    diff --git a/repl/Makefile.in b/repl/Makefile.in
    index 5c65bfa..ba963ec 100644
    a b CONFIG_HEADER = $(top_builddir)/config.h 
    5151CONFIG_CLEAN_FILES =
    5252LTLIBRARIES = $(noinst_LTLIBRARIES)
    5353librepl_la_DEPENDENCIES = @LTLIBOBJS@
    54 am_librepl_la_OBJECTS =
     54am_librepl_la_OBJECTS = dummy.lo
    5555librepl_la_OBJECTS = $(am_librepl_la_OBJECTS)
    5656DEFAULT_INCLUDES = -I. -I$(srcdir) -I$(top_builddir)
    5757depcomp = $(SHELL) $(top_srcdir)/config/depcomp
    sharedstatedir = @sharedstatedir@ 
    172172sysconfdir = @sysconfdir@
    173173target_alias = @target_alias@
    174174noinst_LTLIBRARIES = librepl.la
    175 librepl_la_SOURCES =
     175librepl_la_SOURCES = dummy.c
    176176librepl_la_LIBADD = @LTLIBOBJS@
    177177all: all-am
    178178
    distclean-compile: 
    226226        -rm -f *.tab.c
    227227
    228228@AMDEP_TRUE@@am__include@ @am__quote@$(DEPDIR)/realloc.Plo@am__quote@
     229@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/dummy.Plo@am__quote@
    229230
    230231.c.o:
    231232@am__fastdepCC_TRUE@    if $(COMPILE) -MT $@ -MD -MP -MF "$(DEPDIR)/$*.Tpo" -c -o $@ $<; \
    clean-am: clean-generic clean-libtool clean-noinstLTLIBRARIES \ 
    367368        mostlyclean-am
    368369
    369370distclean: distclean-am
    370         -rm -rf $(DEPDIR)
     371        -rm -rf $(DEPDIR) ./$(DEPDIR)
    371372        -rm -f Makefile
    372373distclean-am: clean-am distclean-compile distclean-generic \
    373374        distclean-libtool distclean-tags
    install-man: 
    393394installcheck-am:
    394395
    395396maintainer-clean: maintainer-clean-am
    396         -rm -rf $(DEPDIR)
     397        -rm -rf $(DEPDIR) ./$(DEPDIR)
    397398        -rm -f Makefile
    398399maintainer-clean-am: distclean-am maintainer-clean-generic
    399400
  • new file repl/dummy.c

    diff --git a/repl/dummy.c b/repl/dummy.c
    new file mode 100644
    index 0000000..39e7442
    - +  
     1void dummy () { return; }
  • src/Makefile.in

    diff --git a/src/Makefile.in b/src/Makefile.in
    index e9ca293..a1f5a81 100644
    a b libiml_la_CFLAGS = $(AM_CFLAGS) 
    223223libiml_la_LIBADD = $(EXTERNLIB) \
    224224                   $(top_builddir)/repl/librepl.la
    225225
     226libiml_la_LDFLAGS = -version-info 1:0:1
    226227all: all-am
    227228
    228229.SUFFIXES:

The first hunk is manually edited, as is the hunk adding the file repl/dummy.c; the rest are created by running automake in the src directory.

There are a couple of things that worry me, such as the -version-info 1:0:1 near the end of the diff. Does that mean that autotools are misunderstanding the version of IML as 1.0.1 instead of 1.0.3? Why does a similar line not already exist in that file? As far as I can tell, I made sure I was using the same versions of autotools that the authors of IML were - autoconf 2.59, automake 1.9.6, and libtool 1.5.22.

Anyway, I'll try again with your patch instead of this dummy function and see what's different. This time I'll also try the whole autotools suite by running autoreconf -i (as recommended to me by Jeroen) - is this what you recommend too, fbissey?

comment:15 Changed 7 years ago by kini

Oh, I should mention that the patch I pasted in the previous comment actually does work to get rid of the build failure caused in repl/. Now the problem seems to be something with dynamic linking. To copy from #13027:

creating test-largeentry
dyld: lazy symbol binding failed: Symbol not found: _cblas_dgemm
  Referenced from: /Users/wstein/build/sage-5.0/spkg/build/iml-1.0.3.p0/src/src/.libs/libiml.0.dylib
  Expected in: flat namespace

dyld: Symbol not found: _cblas_dgemm
  Referenced from: /Users/wstein/build/sage-5.0/spkg/build/iml-1.0.3.p0/src/src/.libs/libiml.0.dylib
  Expected in: flat namespace

/bin/sh: line 1: 74987 Trace/BPT trap          ${dir}$tst
FAIL: test-smallentry
dyld: lazy symbol binding failed: Symbol not found: _cblas_dgemm
  Referenced from: /Users/wstein/build/sage-5.0/spkg/build/iml-1.0.3.p0/src/src/.libs/libiml.0.dylib
  Expected in: flat namespace

dyld: Symbol not found: _cblas_dgemm
  Referenced from: /Users/wstein/build/sage-5.0/spkg/build/iml-1.0.3.p0/src/src/.libs/libiml.0.dylib
  Expected in: flat namespace

/bin/sh: line 1: 75006 Trace/BPT trap          ${dir}$tst
FAIL: test-largeentry
===================
2 of 2 tests failed
===================
make[2]: *** [check-TESTS] Error 1
make[1]: *** [check-am] Error 2
make: *** [check-recursive] Error 1
Error testing IML

As you can see, this is during the testing phase, so building completes successfully with the patch I pasted in the previous comment.

comment:16 Changed 7 years ago by kini

  • Summary changed from [waiting on upstream] update iml to the 1.0.3 release + our patches to update iml to the 1.0.3 release + our patches

If after five years we haven't heard anything from upstream, by the way, I don't think we should any longer "wait on upstream", so I'm changing the title of the ticket...

comment:17 Changed 7 years ago by kini

I spoke too soon - Arne Storjohann just contacted me to tell me that he has corrected the links on the IML webpage for older versions of IML, so now I have been able to produce a diff between clean upstream 1.0.1 and the version in the current standard spkg iml-1.0.1.p14.spkg. I will attach it after posting this comment, for reference.

comment:18 Changed 7 years ago by fbissey

I don't think I ever ran the test with the sage-on-gento ebuild on OS X. I will try that.What OS X version were you testing?

comment:19 Changed 7 years ago by fbissey

Reread the whole thing again. Yes autoreconf -i sounds good. Just for reference while upstream claims you need atlas cblas it doesn't use any atlas specific bits but the configure script is set to find atlas and nothing else.

When we regenerate configure we usually don't ship it as a patch, the diff is usually too big. Historically in the case of cddlib Id on't think we ship pristine source in that case but the patch we used and the recipe to regenerate configure and friends. cddlib is a spkg you could look at for reference.

Tests pass on sage-on-gentoo on OS X 10.5.8 by the way.

comment:20 Changed 7 years ago by kini

  • Keywords sd40.5 added

comment:21 Changed 7 years ago by jpflori

  • Cc jpflori added

comment:22 Changed 7 years ago by kini

Hi jpflori! I was working on this a couple months ago, and as I recall I was running into a similar autotools dilemma to the one you were discussing on sage-devel recently. I never did get it to build on OS X...

Here's an unfinished SPKG if you want to take a look at what I did so far (not much): http://boxen.math.washington.edu/home/keshav/files/iml-1.0.3.p0.unfinished.spkg

comment:23 Changed 7 years ago by fbissey

What is happening to you on OS X?

comment:24 Changed 7 years ago by kini

As I recall, tests were failing because the built binaries weren't linking to the system's BLAS properly, or something. See earlier comments on this ticket and on #13027.

comment:25 Changed 7 years ago by kini

(Sorry, I see that I said I didn't get it to build on OS X - that's incorrect, I got it to build but not to work.)

comment:26 Changed 7 years ago by fbissey

Yes of course I should really have tried to be more helpful there. I'll look at what you have done so far. Using atlas on OS X may help here.

comment:27 Changed 7 years ago by jpflori

I've got no access to OS X based computers, so if the problem is mainly about that, I'll have trouble helping...

I mainly want to add -no-undefined to LDFLAGS on Cygwin (not the proper solution, if it works which has to be checked... we should only add it to the lib..._la_LDFLAGS in some Makefile.am, but that's looking for trouble, and adding it to LDFLAGS should be ok, even though it will be used all the time, even when it's useless, but a priori not harmful).

comment:28 Changed 7 years ago by fbissey

OK there are two options: 1 - we wait for sage to default to ATLAS-3.10 on OS X and use that 2 - rewrite the configure.ac to provide any cblas we want and regenerate configure. We are going to regenerate configure in the process anyway.

comment:29 Changed 7 years ago by kini

Do you have some nonzero expectation of Sage defaulting to building and using ATLAS on OS X rather than using the BLAS that's built into the operating system? When I was working on this I assumed waiting for ATLAS on OS X was a non-option, but maybe you know something I don't.

I only saw the second option remaining, and that's actually why I sort of gave up on this - I didn't want to start writing autotools scripts if possible, after seeing David Kirkby's dire warnings on sage-devel to the effect that in order to write autotools scripts you have to carefully read the autotools manual from cover to cover first...

I suppose a third option would be to ask the iml authors themselves to support generic BLAS.

comment:30 Changed 7 years ago by fbissey

You could say I know enough. And we have already done the work on gentoo. The gentoo solution itself cannot be adopted as it rely on blas/lapack installing pkgconfig files. But I know enough. The easiest thing is just to remove all traces of Atlas autodetection and provide the necessary stuff by compile flags. More sophisticated is to include a "--with-cblas" statement that would be basically do the same thing.

I am otherwise positive that #10508 is opening the possibility of ATLAS on OS X - but all spkg using blas/lapack need to be adjusted for it to be useful.

Patching the detection can be done orthogonally to getting atlas, the autodetection is a tricky business in this case.

comment:31 Changed 7 years ago by fbissey

I think I can spend a little bit of time sorting stuff on this in the next few days. I will try to future proof it for #10508 so that there is no need to change the spkg when it is changed to positive review.

comment:32 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:33 Changed 6 years ago by jdemeyer

  • Dependencies set to #14699
  • Description modified (diff)

comment:34 Changed 6 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-5.11

comment:35 Changed 6 years ago by jdemeyer

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

comment:36 Changed 6 years ago by jdemeyer

not yet doctested but at least the spkg and its testsuite work on Linux and OS X.

comment:37 follow-up: Changed 6 years ago by leif

I still get

./configure: line 18624: -O2: command not found

comment:38 in reply to: ↑ 37 Changed 6 years ago by jdemeyer

Replying to leif:

I still get

./configure: line 18624: -O2: command not found

Fixed in new spkg.

Doctests on boxen.math pass.

comment:39 Changed 6 years ago by jpflori

I already checked the spkg looks sane and installs fine. Sage still pass its (not long) tests.

I just have to check the changes in the patches and I'll give it positive review.

comment:40 follow-up: Changed 6 years ago by jpflori

Why merge tinyatlas.patch into sage1.patch?

On top of that I don't really get the use of the tinyatlas patch. The function it defines (in a header file) is used nor in IML nor in Sage. I'm testing Sage without it (no problems for IML).

So maybe renaming sage1.patch to memory.patch and discarding tinyatlas.patch would be enough. And lets move the sage3_memleak.patch to memleak.patch while at it.

comment:41 in reply to: ↑ 40 ; follow-up: Changed 6 years ago by jdemeyer

Replying to jpflori:

Why merge tinyatlas.patch into sage1.patch?

Because those patches seem related: sage1.patch adds some include for tinyatlas.h.

On top of that I don't really get the use of the tinyatlas patch. The function it defines (in a header file) is used nor in IML nor in Sage.

That's certainly not true. The files RNSop.c and certsolve.c in the src/src directory in IML use catlas_daxpby().

My guess is that's a function which normally comes from ATLAS, but which is needed on systems where ATLAS is not installed.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 6 years ago by jpflori

Replying to jdemeyer:

Replying to jpflori:

Why merge tinyatlas.patch into sage1.patch?

Because those patches seem related: sage1.patch adds some include for tinyatlas.h.

Ok, i thought the include was in tinyatlas.h as well, so I did not check.

On top of that I don't really get the use of the tinyatlas patch. The function it defines (in a header file) is used nor in IML nor in Sage.

That's certainly not true. The files RNSop.c and certsolve.c in the src/src directory in IML use catlas_daxpby().

It seems to be true with IML 1.0.3 but not with 1.0.1 indeed. (I mean the daxpby function was needed in 1.0.1 but is not anymore in 1.0.3.)

My guess is that's a function which normally comes from ATLAS, but which is needed on systems where ATLAS is not installed.

Last edited 6 years ago by jpflori (previous) (diff)

comment:43 Changed 6 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to needs_work
  • Work issues set to remove tinyatlas stuff, rename sage*.patch

comment:44 in reply to: ↑ 42 Changed 6 years ago by jdemeyer

Replying to jpflori:

It seems to be true with IML 1.0.3 but not with 1.0.1.

OK, right, I checked the old sources.

comment:45 Changed 6 years ago by jdemeyer

I confirm that catlas_daxpby() is indeed nowhere used outside of the ATLAS sources.

Working on new spkg...

comment:46 Changed 6 years ago by jdemeyer

Applying the following changes:

  • SPKG.txt

    diff --git a/SPKG.txt b/SPKG.txt
    a b  
    3232
    3333=== Patches ===
    3434
    35  * rename_lift.patch: Change lift to iml_lift in padiclift.* and
    36    nonsingsolve.*, since otherwise  on OSX you'll have horrible weird
    37    conflict problems.
     35 * blas_headers.patch: Add BLAS header files from GSL, needed in case
     36   ATLAS has not been installed.
     37 * build.patch: Made build scripts that work.
    3838 * configure_default_cflags.patch: get rid of the following error
    3939   during configure:
    4040     ./configure: line 18624: -O3: command not found
    41  * Modified some of the examples, and made build scripts that work.
     41 * examples.patch: Modified some of the examples.
     42 * memleak.patch: use mpz_set_ui instead of mpz_init_set_ui on mpz
     43   which is already allocated.
     44 * remove_repl.patch: Do not build/install src/repl at all, since it
     45   does nothing anyway and creating empty archives fails on OS X.
    4246
    4347== Changelog ==
    4448
    4549=== iml-1.0.3.p0 (Jeroen Demeyer, 12 June 2013) ===
    4650 * #748: Upgrade to latest upstream version, rebase patches.
    4751 * Remove rename_lift.patch and sage2.patch, which were upstreamed.
    48  * Merged tinyatlas.patch into sage1.patch.
    49  * Apply sage3_memleak.patch in all 3 places with similar code.
     52 * Removed tinyatlas.patch and #include "tinyatlas.h"
     53 * Removed sage1.patch
     54 * Apply sage3_memleak.patch in all 3 places with similar code, rename
     55   to memleak.patch
    5056 * Use -O3 optimization level by default.
    5157 * Add configure_default_cflags.patch.
    5258
  • deleted file patches/sage1.patch

    diff --git a/patches/sage3_memleak.patch b/patches/memleak.patch
    rename from patches/sage3_memleak.patch
    rename to patches/memleak.patch
    diff --git a/patches/sage1.patch b/patches/sage1.patch
    deleted file mode 100644
    + -  
    1 diff -ruN b/src/RNSop.c a/src/RNSop.c
    2 --- b/src/RNSop.c      2006-11-23 22:25:23.000000000 +0100
    3 +++ a/src/RNSop.c      2013-06-10 23:05:18.872404179 +0200
    4 @@ -46,6 +46,7 @@
    5  
    6  
    7  #include "RNSop.h"
    8 +#include "tinyatlas.h"
    9  
    10  /*
    11   *
    12 diff -ruN b/src/memalloc.c a/src/memalloc.c
    13 --- b/src/memalloc.c   2006-11-23 22:25:23.000000000 +0100
    14 +++ a/src/memalloc.c   2013-06-10 23:05:18.872404179 +0200
    15 @@ -48,13 +48,16 @@
    16  
    17  #include "error.h"
    18  #include "common.h"
    19 +#include "stdio.h"
    20  
    21  void *
    22  xmalloc (size_t num)
    23  {
    24    void * new = malloc(num);
    25 -  if (!new)
    26 -    iml_fatal ("Memory exhausted");
    27 +  if (!new) {
    28 +    printf("%ul\n", num);
    29 +    iml_fatal ("Memory exhausted in xmalloc");
    30 +  }
    31    return new;
    32  }
    33  
    34 @@ -65,8 +68,10 @@
    35    if (!p)
    36      return xmalloc(num);
    37    new = realloc(p, num);
    38 -  if (!new)
    39 -    iml_fatal("Memory exhausted");
    40 +  if (!new) {
    41 +    printf("%ul\n", num);
    42 +    iml_fatal("Memory exhausted in xrealloc");
    43 +  }
    44    return new;
    45  }
    46  
    47 @@ -76,8 +81,10 @@
    48  {
    49  #if HAVE_CALLOC
    50    void * new = calloc(num, size);
    51 -  if (!new)
    52 -    iml_fatal("Memory exhausted");
    53 +  if (!new) {
    54 +    printf("%ul\n", num);
    55 +    iml_fatal("Memory exhausted in xcalloc");
    56 +  }
    57  #else
    58    void * new = xmalloc(num*size);
    59    bzero(new, num*size);
    60 diff -ruN iml-1.0.1-sage/src/tinyatlas.h src/src/tinyatlas.h
    61 --- iml-1.0.1-sage/src/tinyatlas.h     1970-01-01 01:00:00.000000000 +0100
    62 +++ src/src/tinyatlas.h        2007-03-01 04:11:42.000000000 +0100
    63 @@ -0,0 +1,17 @@
    64 +/*
    65 +Compute Y = alpha * X + beta * Y
    66 +
    67 +where
    68 +   N = degree of each vector
    69 +   incX = X stride
    70 +   incY = Y stride
    71 +*/
    72 +
    73 +void catlas_daxpby(const int N, const double alpha, const double *X,
    74 +const int incX, const double beta, double *Y, const int incY)
    75 +{
    76 +  int i;
    77 +  for(i=0; i < N; i++) {
    78 +    Y[i*incY] = alpha * X[i*incX] + beta * Y[i*incY];
    79 +  }
    80 +}

Changed 6 years ago by jdemeyer

comment:47 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review
  • Work issues remove tinyatlas stuff, rename sage*.patch deleted

comment:48 Changed 6 years ago by jpflori

  • Status changed from needs_review to positive_review

comment:49 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.11.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:50 follow-up: Changed 5 years ago by leif

Sign of life of IML upstream on #14648 !!1!111!

comment:51 in reply to: ↑ 50 Changed 5 years ago by leif

Replying to leif:

Sign of life of IML upstream on #14648 !!1!111!

http://trac.sagemath.org/ticket/14648#comment:51 ff. that is.

Note: See TracTickets for help on using tickets.