Opened 15 years ago

Closed 9 years ago

Last modified 9 years ago

#748 closed defect (fixed)

update iml to the 1.0.3 release + our patches

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

Status badges

Attachments (1)

iml-1.0.3.p0.diff (16.2 KB) - added by Jeroen Demeyer 9 years ago.

Download all attachments as: .zip

Change History (52)

comment:1 Changed 15 years ago by William Stein

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 15 years ago by William Stein

The 1.0.1.p8 version is in sage-2.8.5

comment:3 Changed 15 years ago by William Stein

Milestone: sage-2.8.6sage-2.9

comment:4 Changed 15 years ago by Michael Abshoff

Summary: iml autohellupdate iml to the 1.0.2 release + our patches

comment:5 Changed 14 years ago by Michael Abshoff

Owner: changed from William Stein to Michael Abshoff
Status: newassigned

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 14 years ago by Craig Citro

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

comment:7 Changed 13 years ago by Jason Grout

Status: newneeds_info_new

comment:8 Changed 11 years ago by Leif Leonhardy

Keywords: spkg upgrade added
Milestone: sage-4.7.2sage-duplicate/invalid/wontfix
Report Upstream: N/A
Resolution: duplicate
Status: needs_infoclosed

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

comment:9 Changed 11 years ago by Leif Leonhardy

Reviewers: Leif Leonhardy

comment:10 Changed 11 years ago by David Kirkby

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 11 years ago by Keshav 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 11 years ago by Keshav Kini

Keywords: iml added
Resolution: duplicate
Reviewers: Leif Leonhardy
Status: closednew

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 11 years ago by François Bissey

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 11 years ago by Keshav 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 11 years ago by Keshav 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 11 years ago by Keshav Kini

Summary: [waiting on upstream] update iml to the 1.0.3 release + our patchesupdate 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 11 years ago by Keshav 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 11 years ago by François Bissey

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 11 years ago by François Bissey

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 11 years ago by Keshav Kini

Keywords: sd40.5 added

comment:21 Changed 10 years ago by Jean-Pierre Flori

Cc: Jean-Pierre Flori added

comment:22 Changed 10 years ago by Keshav 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 10 years ago by François Bissey

What is happening to you on OS X?

comment:24 Changed 10 years ago by Keshav 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 10 years ago by Keshav 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 10 years ago by François Bissey

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 10 years ago by Jean-Pierre Flori

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 10 years ago by François Bissey

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 10 years ago by Keshav 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 10 years ago by François Bissey

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 10 years ago by François Bissey

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 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:33 Changed 9 years ago by Jeroen Demeyer

Dependencies: #14699
Description: modified (diff)

comment:34 Changed 9 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Description: modified (diff)
Milestone: sage-duplicate/invalid/wontfixsage-5.11

comment:35 Changed 9 years ago by Jeroen Demeyer

Description: modified (diff)
Status: newneeds_review

comment:36 Changed 9 years ago by Jeroen Demeyer

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

comment:37 Changed 9 years ago by Leif Leonhardy

I still get

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

comment:38 in reply to:  37 Changed 9 years ago by Jeroen Demeyer

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 9 years ago by Jean-Pierre Flori

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 Changed 9 years ago by Jean-Pierre Flori

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 ; Changed 9 years ago by Jeroen Demeyer

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 ; Changed 9 years ago by Jean-Pierre Flori

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.

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

Version 0, edited 9 years ago by Jean-Pierre Flori (next)

comment:43 Changed 9 years ago by Jean-Pierre Flori

Reviewers: Jean-Pierre Flori
Status: needs_reviewneeds_work
Work issues: remove tinyatlas stuff, rename sage*.patch

comment:44 in reply to:  42 Changed 9 years ago by Jeroen Demeyer

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 9 years ago by Jeroen Demeyer

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

Working on new spkg...

comment:46 Changed 9 years ago by Jeroen Demeyer

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 9 years ago by Jeroen Demeyer

Attachment: iml-1.0.3.p0.diff added

comment:47 Changed 9 years ago by Jeroen Demeyer

Status: needs_workneeds_review
Work issues: remove tinyatlas stuff, rename sage*.patch

comment:48 Changed 9 years ago by Jean-Pierre Flori

Status: needs_reviewpositive_review

comment:49 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.11.beta2
Resolution: fixed
Status: positive_reviewclosed

comment:50 Changed 9 years ago by Leif Leonhardy

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

comment:51 in reply to:  50 Changed 9 years ago by Leif Leonhardy

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.