New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade lrcalc to 1.1.7 #16560
Comments
Attachment: lrcalc-1.1.7.tar.gz |
This comment has been minimized.
This comment has been minimized.
comment:1
Running tests now... |
comment:2
All (long) tests pass for me. |
Commit: |
comment:4
So the only difference between the two upstream tarballs is that one of them is properly autool=ed and the other is not? |
Reviewer: François Bissey |
comment:5
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
Replying to @kiwifb:
Nicolas would be the best one to answer that since he helped create it. Thanks for doing the review. |
comment:7
Replying to @tscrim:
As far as I remember, yes. And this one is an official release by Anders rather than a tarball produced by myself. |
comment:8
Fails on OSX
|
comment:9
Hum.... it seems it cannot cop with lrcalc_panic_frame being defined in mathlib/alloc.h
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
@ nthiery: in configure.ac, oops:
Not hurtful of course. |
Attachment: lrcalc-1.1.7-jump.patch.gz solve the lrcalc_panic_frame definition problem |
comment:11
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
I will test it on my Ubuntu machine later tonight (if no one beats me to it). |
comment:13
Well I have now checked it builds and installs in sage-on-gentoo on linux and OS X but haven't run tests. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:15
Git-a-fied and the spkg and sage checks passed for me. |
comment:16
Thanks for figuring out the OSX François (I couldn't do anything since I don't have access to one). |
Upstream: Reported upstream. No feedback yet. |
comment:17
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
So do you think we should hold off on merging this until we hear back? |
Changed branch from public/packages/lrcalc_1_1_7-16560 to |
Changed commit from |
comment:20
Replying to @tscrim:
Double dose of no. One for me and one for Volker :) |
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.
Upstream: Reported upstream. No feedback yet.
CC: @sagetrac-sage-combinat @nthiery
Component: packages: standard
Keywords: lrcalc
Author: Travis Scrimshaw
Branch:
b124959
Reviewer: François Bissey
Issue created by migration from https://trac.sagemath.org/ticket/16560
The text was updated successfully, but these errors were encountered: