Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16560 closed enhancement (fixed)

Upgrade lrcalc to 1.1.7

Reported by: tscrim Owned by: sage-combinat
Priority: major Milestone: sage-6.3
Component: packages: standard Keywords: lrcalc
Cc: sage-combinat, nthiery Merged in:
Authors: Travis Scrimshaw Reviewers: François Bissey
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: b124959 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by tscrim)

Using the upstream based off of here: http://www.math.rutgers.edu/~asbuch/lrcalc/ but use the one in the attachments as the root directory is lrcalc-sage-1.1.7.

This should (help) fix #14625.

Attachments (2)

lrcalc-1.1.7.tar.gz (1.2 MB) - added by tscrim 5 years ago.
lrcalc-1.1.7-jump.patch (4.4 KB) - added by fbissey 5 years ago.
solve the lrcalc_panic_frame definition problem

Download all attachments as: .zip

Change History (22)

Changed 5 years ago by tscrim

comment:1 Changed 5 years ago by tscrim

  • Branch set to public/packages/lrcalc_1_1_7-16560
  • Description modified (diff)
  • Status changed from new to needs_review

Running tests now...

comment:2 Changed 5 years ago by tscrim

All (long) tests pass for me.

comment:3 Changed 5 years ago by git

  • Commit set to 7134ee179bfa1126cde66f8fc4e1e30db5e89a4a

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

190501cChanged version number.
69d3573Update SPKG.txt and checksums.ini.
7134ee1Changed checksum.

comment:4 follow-up: Changed 5 years ago by fbissey

So the only difference between the two upstream tarballs is that one of them is properly autool=ed and the other is not?

comment:5 Changed 5 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

This version has otherwise been in use in sage-on-gentoo since December without any reported issue so I am putting that positive review.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by tscrim

Replying to fbissey:

So the only difference between the two upstream tarballs is that one of them is properly autool=ed and the other is not?

Nicolas would be the best one to answer that since he helped create it.

Thanks for doing the review.

comment:7 in reply to: ↑ 6 Changed 5 years ago by nthiery

Replying to tscrim:

Replying to fbissey:

So the only difference between the two upstream tarballs is that one of them is properly autool=ed and the other is not?

Nicolas would be the best one to answer that since he helped create it.

As far as I remember, yes. And this one is an official release by Anders rather than a tarball produced by myself.

comment:8 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Fails on OSX

libtool: link: gcc -dynamiclib -Wl,-undefined -Wl,dynamic_lookup -o .libs/liblrcalc.1.dylib   -Wl,-force_load,mathlib/.libs/libmath.a -Wl,-force_load,lrcoef/.libs/libsymfcn.a -Wl,-force_load,schubert/.libs/libschub.a   -O2   -install_name  /Users/buildslave-sage/slave/sage_git/build/local/lib/liblrcalc.1.dylib -compatibility_version 2 -current_version 2.0 -Wl,-single_module
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    mathlib/.libs/libmath.a(hashtab.o)
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    mathlib/.libs/libmath.a(list.o)
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    mathlib/.libs/libmath.a(set.o)
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    mathlib/.libs/libmath.a(vectarg.o)
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    mathlib/.libs/libmath.a(vector.o)
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    lrcoef/.libs/libsymfcn.a(symfcn.o)
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    lrcoef/.libs/libsymfcn.a(maple.o)
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    schubert/.libs/libschub.a(schublib.o)
duplicate symbol _lrcalc_panic_frame in:
    mathlib/.libs/libmath.a(alloc.o)
    schubert/.libs/libschub.a(lincomb.o)
ld: 9 duplicate symbols for architecture x86_64
collect2: error: ld returned 1 exit status
make[5]: *** [liblrcalc.la] Error 1
make[5]: Target `all-am' not remade because of errors.
make[4]: *** [all-recursive] Error 1
make[3]: *** [all] Error 2
Error building lrcalc.

Full log: http://build.sagemath.org/sage/builders/%20%20fast%20Volker%20MiniMac%20%28OSX%2010.9%20x86_64%29%20incremental/builds/279/steps/compile/logs/stdio

comment:9 Changed 5 years ago by fbissey

Hum.... it seems it cannot cop with lrcalc_panic_frame being defined in mathlib/alloc.h

#include <setjmp.h>

/*  Programs using the lrcalc library should set lrcalc_panic_frame
 *  with setjmp(lrcalc_panic_frame).  The lrcalc library will call
 *  longjmp(lrcalc_panic_frame, 1) if an "out of memory" event occurs.
 */
jmp_buf lrcalc_panic_frame;

All the files for which the linker is complaining about are inheriting the declaration through multiple include statement but are not using it. I guess the proper course of action, from a correctness point of view, would be to create a specific header for it and include it only where needed. Looking for other options....

comment:10 Changed 5 years ago by fbissey

@ nthiery: in configure.ac, oops:

AC_PREREQ([2.67])
AC_INIT([lrcalc],[1.1.6],[asbuch at math rutgers edu])

Not hurtful of course.

Changed 5 years ago by fbissey

solve the lrcalc_panic_frame definition problem

comment:11 Changed 5 years ago by fbissey

OK the attached patch solve the problem on OS X, it should work on linux too but I haven't tested it there so far. It is a bit invasive and should be submitted upstream for consideration.

comment:12 Changed 5 years ago by tscrim

I will test it on my Ubuntu machine later tonight (if no one beats me to it).

comment:13 Changed 5 years ago by fbissey

Well I have now checked it builds and installs in sage-on-gentoo on linux and OS X but haven't run tests.

comment:14 Changed 5 years ago by git

  • Commit changed from 7134ee179bfa1126cde66f8fc4e1e30db5e89a4a to b124959527d51806a30b2e44d3038547e04f2a90

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

b124959Added patch file to lrcalc.

comment:15 Changed 5 years ago by tscrim

  • Status changed from needs_work to positive_review

Git-a-fied and the spkg and sage checks passed for me.

comment:16 Changed 5 years ago by tscrim

Thanks for figuring out the OSX François (I couldn't do anything since I don't have access to one).

comment:17 Changed 5 years ago by fbissey

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

Well it is all a bit sloppy in my opinion. You have this variable that I put in separate header that needs to be global in the programs using it. Problems is it was put everywhere including the library parts. The gnu linker used in linux is infintely tolerant to some stuff and possibly has a way of dealing with it. But in this case I think the OS X linker is correct in saying "please fix this up".

I also sent an email upstream along with the patch.

comment:18 follow-up: Changed 5 years ago by tscrim

So do you think we should hold off on merging this until we hear back?

comment:19 Changed 5 years ago by vbraun

  • Branch changed from public/packages/lrcalc_1_1_7-16560 to b124959527d51806a30b2e44d3038547e04f2a90
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:20 in reply to: ↑ 18 Changed 5 years ago by fbissey

  • Commit b124959527d51806a30b2e44d3038547e04f2a90 deleted

Replying to tscrim:

So do you think we should hold off on merging this until we hear back?

Double dose of no. One for me and one for Volker :)

Note: See TracTickets for help on using tickets.