#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, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
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)
Change History (22)
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Branch set to public/packages/lrcalc_1_1_7-16560
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
All (long) tests pass for me.
comment:3 Changed 8 years ago by
- Commit set to 7134ee179bfa1126cde66f8fc4e1e30db5e89a4a
comment:4 follow-up: ↓ 6 Changed 8 years ago by
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 8 years ago by
- 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: ↓ 7 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
- 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.
comment:9 Changed 8 years ago by
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 8 years ago by
@ nthiery: in configure.ac, oops:
AC_PREREQ([2.67]) AC_INIT([lrcalc],[1.1.6],[asbuch at math rutgers edu])
Not hurtful of course.
comment:11 Changed 8 years ago by
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 8 years ago by
I will test it on my Ubuntu machine later tonight (if no one beats me to it).
comment:13 Changed 8 years ago by
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 8 years ago by
- Commit changed from 7134ee179bfa1126cde66f8fc4e1e30db5e89a4a to b124959527d51806a30b2e44d3038547e04f2a90
Branch pushed to git repo; I updated commit sha1. New commits:
b124959 | Added patch file to lrcalc.
|
comment:15 Changed 8 years ago by
- Status changed from needs_work to positive_review
Git-a-fied and the spkg and sage checks passed for me.
comment:16 Changed 8 years ago by
Thanks for figuring out the OSX François (I couldn't do anything since I don't have access to one).
comment:17 Changed 8 years ago by
- 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: ↓ 20 Changed 8 years ago by
So do you think we should hold off on merging this until we hear back?
comment:19 Changed 8 years ago by
- 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 8 years ago by
- 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 :)
Running tests now...