Opened 11 years ago
Last modified 11 years ago
#11321 closed defect
Make lcalc compatible with the new PARI — at Version 47
Reported by: | jdemeyer | Owned by: | jdemeyer |
---|---|---|---|
Priority: | major | Milestone: | sage-4.8 |
Component: | packages: standard | Keywords: | lcalc spkg |
Cc: | fbissey, rishi | Merged in: | |
Authors: | Jeroen Demeyer, Leif Leonhardy | Reviewers: | Volker Braun, Leif Leonhardy |
Report Upstream: | Reported upstream. Developers acknowledge bug. | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #11130 | Stopgaps: |
Description (last modified by )
PARI version 2.4.4 (see #11130, now aiming at 2.5.0) no longer has an allocatemoremem()
function, which lcalc
uses. There is a function allocatemem()
which can be used instead.
The second reviewer patch (not in the spkg) removes the requirement for a PARI >= 2.4.4 spkg, i.e. with that applied it will build with our current ones as well.
New spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-1.23.p9.spkg (this will only build after installing the PARI spkg from #11130).
Reviewers should look at lcalc-1.23.p6-p9.diff to see the difference with the lcalc spkg currently in Sage.
Change History (51)
comment:1 follow-up: ↓ 3 Changed 11 years ago by
- Reviewers set to Volker Braun
comment:2 Changed 11 years ago by
- Cc fbissey added
comment:3 in reply to: ↑ 1 Changed 11 years ago by
Replying to vbraun:
Looks good to me. Are you making a full lcalc spkg at one point?
Yes, when I have time.
comment:4 Changed 11 years ago by
- Description modified (diff)
comment:5 Changed 11 years ago by
- Dependencies set to #11130
comment:6 Changed 11 years ago by
- Report Upstream changed from N/A to Reported upstream. Little or no feedback.
comment:7 Changed 11 years ago by
- Status changed from new to needs_review
New spkg ready for review, needs to be built with the new PARI spkg.
comment:8 Changed 11 years ago by
- Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. Developers acknowledge bug.
On 2011-05-12 18:56, Michael Rubinstein wrote:
Thanks for the patch.
I should point out that I now have a google code page for lcalc:
http://code.google.com/p/l-calc/
(lcalc was taken, so I went with l-calc).
The page has my beta newer version of lcalc, L-1.3 However, Rishikesh's cython wrapper is still being updated for that, so it won't work within sage with the current wrapper.
I should point out that Lcommandline_elliptic.cc has become part of the underlying class library and moved to Lelliptic.cc in version L-1.3
Mike
comment:9 Changed 11 years ago by
- Status changed from needs_review to needs_work
comment:10 Changed 11 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:11 Changed 11 years ago by
- Description modified (diff)
comment:12 Changed 11 years ago by
- Description modified (diff)
The patch to the Makefile still uses -lmpir
instead of -lgmp
. (We always build MPIR with GMP compatibility, as all other packages refer to GMP rather than MPIR.) And the receipt for all
is still funny, but that's upstream...
I'd quote $patch
to be on the safe side. Also, the patches are / have to be applied with -p1
(from lcalc-*/src/
) rather than -p0
, which I don't mind, but Jeroen wanted to unify this across spkgs IIRC.
comment:13 Changed 11 years ago by
- Reviewers changed from Volker Braun to Volker Braun, Leif Leonhardy
spkg-install:
- The comment and a message regarding
SAGE_DEBUG
is not consistent. (SAGE_DEBUG
has to be set toyes
to enable debugging support, while anything else disables it, though not deleting any user-specified flags of course.) I'd always add-g
anyway, since this doesn't have any measureable performance impact, only the object files will grow slightly. $CC
must not be quoted in the compiler test (if we support flags withinCC
rather than compiler paths containing spaces). Perhaps a matter of taste, but this should be consistent in Sage, and e.g.distutils
uses the former variant, not to mention the way we use$MAKE
. Same applies to$CXX
and in principle$SAGE_FORTRAN
, while the Fortran compiler test is useless there anyway.- Funny that Dave uses
test -e
for the Fortran library... - I wonder if error messages etc. should be redirected to
stderr
as we started to do in a couple of scripts. The disadvantage of doing so is that both streams occasionally get out of sync due to buffering, leading to potentially confusing output. - Copy sage specific modifications should now of course read Apply Sage ....
$UNAME
should also be quoted (twice);case ... in ...
would anyway be better.- I'm not sure if Cygwin needs both a
.dll
and a.so
file (cf. #11547). Currently the.dll
is copied to an.so
in$SAGE_LOCAL/lib/
. Unless it is statically linked, the stand-alonelcalc
might also require the shared library in$SAGE_LOCAL/bin/
.
SPKG.txt
:
- The Upstream contact section should be moved up.
- Special Build Instructions should read ... Update/Build? ....
- The Changelog heading is missing.
- The Dependencies section is completely missing.
Some files from src/include/
can also be deleted (and I doubt we have to copy all of them to $SAGE_LOCAL/include/lcalc/
; L*.h
and mpfr_mul_d.h
-- included by Lgmpfrxx.h
-- should suffice):
~/Sage/spkgs/lcalc-1.23.p7$ ls src/include/
cmdline.h Lfind_zeros.h
getopt.h Lgamma.h
Lcommandline_elliptic.h Lglobals.h
Lcommandline_globals.h Lgmpfrxx.h
Lcommandline.h Lgram.h
Lcommandline_misc.h L.h
Lcommandline_numbertheory.h Lint_complex.h
Lcommandline_twist.h Lmisc.h
Lcommandline_values_zeros.h Lnumberzeros.h
Lcommon.h Lnumeric.h
Lcommon_ld.h Lprint.h
Lcomplex.h Lriemannsiegel_blfi.h
Ldirichlet_series.h Lriemannsiegel.h
Ldokchitser.h Lvalue.h
Lexplicit_formula.h Lvalue.h.bak
Lexplicit_formula.h.swap.crap mpfr_mul_d.h
I wonder if it's worth to run Lcalc's tests from an spkg-check
file.
I'd really appreciate having the changes to spkgs to be "finally" reviewed committed, especially if files get deleted, as is the case here. Also, IMHO commit messages are subject to review as well, no matter if some merge script would add a generic one in case it is missing.
comment:14 follow-up: ↓ 15 Changed 11 years ago by
- Description modified (diff)
New spkg, same location. This cases care of most of leif's comments. I believe that the remainder of leif's comments should not prevent a positive review for this ticket.
About committing the changes: I find it very annoying to have to commit the changes everytime I make a change. If you are happy with the spkg except for the committing of the changes, I can still commit the changes at that point.
comment:15 in reply to: ↑ 14 ; follow-ups: ↓ 16 ↓ 17 Changed 11 years ago by
Replying to jdemeyer:
About committing the changes: I find it very annoying to have to commit the changes everytime I make a change.
You don't have to, unless you publish them. ;-)
And you can always keep an uncommitted copy. I don't think it's much work to commit the changes if you have to create (perhaps upload) and announce a new spkg anyway.
If you are happy with the spkg except for the committing of the changes, I can still commit the changes at that point.
Well, not committing the changes means I have to commit your changes in your name (with some random commit message) and repackage the spkg
- in order to really test all changes (as files get deleted upon commit),
- if I want to make some reviewer patch or a (follow-up) spkg based on yours.
The first point is of course more crucial, since not only lazy people might just install and test the spkg as is, such that potential errors will only show up much later. So not committing the changes is IMHO simply bad practice unless the spkg is really and explicitly said to be in some alpha state, most probably not ready and not intended to get merged as is (despite needing review / testing by others), which might be ok in rare cases.
There's a reason the term commit is used rather than just e.g. save changes, the former implying some confidence or conviction by the author (or committer).
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 19 Changed 11 years ago by
Replying to leif:
Well, not committing the changes means I have to commit your changes in your name (with some random commit message) and repackage the spkg in order to really test all changes (as files get deleted upon commit),
This is simply false. Committing does not delete the files. Committing only changes files inside .hg
.
comment:17 in reply to: ↑ 15 ; follow-up: ↓ 20 Changed 11 years ago by
Replying to leif:
Well, not committing the changes means I have to commit your changes in your name (with some random commit message) and repackage the spkg if I want to make some reviewer patch or a (follow-up) spkg based on yours.
When doing this, committing the changes is probably the least of your worries. Basing a new spkg on a not-yet-positively-reviewed skpg is not a very good idea anyway (see #11605 vs #11130).
comment:18 follow-ups: ↓ 24 ↓ 27 Changed 11 years ago by
From first glance:
- (GNU)
patch
should be added to the dependencies section. - This is still in:
(As mentioned, I also would add
# If SAGE_DEBUG is set either unset (the default), or set to 'yes' ... echo >&2 "Code will be built with debugging information present. Set 'SAGE_DEBUG' to 'no' if you don't want that." # Actually anything othe than 'yes' will cause # no debugging information to be added.
-g
by default.) - There's neither a
patches/README.txt
nor a Patches section inSPKG.txt
. success 'install'
should perhaps besuccess 'copying header files'
.
The rest looks ok.
I've btw. successfully tested the previous p7 with PARI 2.5.0.p0 and Sage 4.7.1.rc0 (Ubuntu 10.04 x86_64, GCC 4.5.1), both with our current MPIR and the newer MPIR 2.1.3 from #8664.
comment:19 in reply to: ↑ 16 ; follow-up: ↓ 21 Changed 11 years ago by
Replying to jdemeyer:
Replying to leif:
Well, not committing the changes means I have to commit your changes in your name (with some random commit message) and repackage the spkg in order to really test all changes (as files get deleted upon commit),
This is simply false. Committing does not delete the files. Committing only changes files inside
.hg
.
hg remove foo && hg commit
removes foo
also from the working directory (unless you've already deleted it).
comment:20 in reply to: ↑ 17 Changed 11 years ago by
Replying to jdemeyer:
Replying to leif:
Well, not committing the changes means I have to commit your changes in your name (with some random commit message) and repackage the spkg if I want to make some reviewer patch or a (follow-up) spkg based on yours.
When doing this, committing the changes is probably the least of your worries.
For a reviewer patch it is.
Basing a new spkg on a not-yet-positively-reviewed skpg is not a very good idea anyway (see #11605 vs #11130).
#11604 was based on #11605 in the hope of (or giving it a chance) getting it into Sage 4.7.1, such that this bug gets fixed in reasonable time (in an official release). #11605 was likely to get merged into 4.7.1 because it is a critical bug, so not basing #11604 on the former would have simply been stupid.
#11130 has the milestone set to Sage 4.7.2 and currently (officially) still needs work. (Basing a new spkg on that of #11130 would not have been necessary since the new PARI version fixes the bug anyway. And even if it didn't, you claim above it would have been a bad idea to base a new one on a not yet positively reviewed, as is the case with PARI 2.5.0.p0, more precisely, it even "needs work" as I mentioned above.)
comment:21 in reply to: ↑ 19 Changed 11 years ago by
Replying to leif:
Replying to jdemeyer:
This is simply false. Committing does not delete the files. Committing only changes files inside
.hg
.
hg remove foo && hg commit
removesfoo
also from the working directory (unless you've already deleted it).
P.S.: If you don't want the file in the working directory to get deleted, use hg forget ...
, which just removes it from the (current version in the) repository.
comment:22 Changed 11 years ago by
Ok, you're right. hg remove foo
removes foo
from the working directory immediately. (And if you recreate it and commit, that won't delete the new one either, which might be a different pitfall, though Mercurial will report it as an unknown file afterwards.)
Anyway, I believe hg status
should simply report nothing, as one might miss the ?
among lots of other files marked M
, A
or R
.
comment:23 Changed 11 years ago by
P.P.S.: src/include/Lvalue.h.bak
is still present in the p8.
comment:24 in reply to: ↑ 18 ; follow-up: ↓ 25 Changed 11 years ago by
- Status changed from needs_review to needs_work
Replying to leif:
- This is still in:
(As mentioned, I also would add# If SAGE_DEBUG is set either unset (the default), or set to 'yes' ... echo >&2 "Code will be built with debugging information present. Set 'SAGE_DEBUG' to 'no' if you don't want that." # Actually anything othe than 'yes' will cause # no debugging information to be added.-g
by default.)
I do not plan to change this here. This is a functional change which is unrelated to the topic of this ticket so such a change does not belong here.
comment:25 in reply to: ↑ 24 Changed 11 years ago by
Replying to jdemeyer:
Replying to leif:
- This is still in:
# If SAGE_DEBUG is set either unset (the default), or set to 'yes' ... echo >&2 "Code will be built with debugging information present. Set 'SAGE_DEBUG' to 'no' if you don't want that." # Actually anything othe than 'yes' will cause # no debugging information to be added.
(As mentioned, I also would add
-g
by default.)I do not plan to change this here. This is a functional change which is unrelated to the topic of this ticket so such a change does not belong here.
Just remove the "is set either unset (the default), or", add "or unset it" to "Set 'SAGE_DEBUG' to 'no' ...", and s/othe/other/
.
I don't mind not adding -g
by default here (though it really makes sense, and other packages do this). Of course one can always put -g
into the "global" CFLAGS
, but you usually need debugging information when it's "too late". Getting better tracebacks from less experienced users is the main reason. One can always strip the symbols afterwards...
Also, other packages that disable optimization completely (-O0
) if SAGE_DEBUG=yes
should IMHO issue a warning when doing so.
comment:26 Changed 11 years ago by
- Milestone changed from sage-4.7.1 to sage-4.7.2
- Status changed from needs_work to needs_review
New spkg (even with changes committed), same location: http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-1.23.p8.spkg
comment:27 in reply to: ↑ 18 ; follow-up: ↓ 30 Changed 11 years ago by
This still applies:
Replying to leif:
- (GNU)
patch
should be added to the dependencies section.
- There's neither a
patches/README.txt
nor a Patches section inSPKG.txt
.
success 'install'
should perhaps besuccess 'copying header files'
.
I'll perhaps add a reviewer patch for this, which of course doesn't affect that:
P.P.S.: src/include/Lvalue.h.bak is still present in the p8.
But it's mentioned in the Special Update/Build? Instructions, and only about 25 KB, so a minor issue.
comment:28 Changed 11 years ago by
Just found some more things:
src/src/Makefile.old
can also be deleted.
- The
Makefile
not only definesCC
tog++
, but also hardcodesg++
when linking the shared library.
comment:29 Changed 11 years ago by
Any reason to not build (and run) the test suite (from an spkg-check
, to be added)?
(Haven't tried, so it may be in an unusable state.)
Changed 11 years ago by
SPKG patch. The only change with slight functional effect is the rearrangement of flags (CFLAGS et al.). Apply to Jeroen's p8 of July 26th.
comment:30 in reply to: ↑ 27 Changed 11 years ago by
Replying to leif:
This still applies:
Replying to leif:
- (GNU)
patch
should be added to the dependencies section.
- There's neither a
patches/README.txt
nor a Patches section inSPKG.txt
.
success 'install'
should perhaps besuccess 'copying header files'
.
I'll perhaps add a reviewer patch for this.
Did so. I've also made further cosmetic changes to spkg-install
, and also changed the order of flags in CFLAGS
etc. such that user settings only get overridden when necessary.
I'll perhaps add a second reviewer patch fixing the hardcoded g++
in Lcalc's Makefile, eventually along with a p9 spkg with the other two superfluous upstream files deleted.
Changed 11 years ago by
SPKG patch. Makes this spkg also work with older PARI versions (prior to 2.4.4), adds some notes to SPKG.txt. Apply on top of "part_1".
comment:31 follow-up: ↓ 33 Changed 11 years ago by
- Description modified (diff)
A second reviewer patch is up.
This does not yet fix the hardcoded g++
, but changes the pari-2.4.4.patch
such that it will work with older PARI versions, e.g. our current 2.4.3.alpha.* spkgs, as well, so we can in principle remove the dependency on #11130 (and merge it earlier).
I've also detailed the description of the patches in SPKG.txt
and added a few more comments there.
comment:32 Changed 11 years ago by
Leif, will you post a new spkg with your changes added?
comment:33 in reply to: ↑ 31 ; follow-up: ↓ 34 Changed 11 years ago by
Replying to leif:
changes the
pari-2.4.4.patch
such that it will work with older PARI versions, e.g. our current 2.4.3.alpha.* spkgs, as well, so we can in principle remove the dependency on #11130 (and merge it earlier).
I'm not sure why we should do this. There is no reason to merge this before #11130, so making it compatible with earlier versions just looks like more complication to me.
comment:34 in reply to: ↑ 33 ; follow-up: ↓ 35 Changed 11 years ago by
- Cc rishi added
Replying to jdemeyer:
Replying to leif: Leif, will you post a new spkg with your changes added?
Yes, later today.
changes the
pari-2.4.4.patch
such that it will work with older PARI versions, e.g. our current 2.4.3.alpha.* spkgs, as well, so we can in principle remove the dependency on #11130 (and merge it earlier).I'm not sure why we should do this. There is no reason to merge this before #11130, so making it compatible with earlier versions just looks like more complication to me.
Well, it's not too complicated it wasn't worth to remove the dependency, be it for testing / reviewing, or further work on the package. (The new spkg will have other advantages btw.)
But that's IMHO something we should report upstream, as it won't be the last time Lcalc will have some PARI version-dependent parts... ;-)
comment:35 in reply to: ↑ 34 Changed 11 years ago by
comment:36 Changed 11 years ago by
Regardless of what leif
said, I think my spkg still stands to be reviewed as-is.
leif, please do not clutter the SPKG.txt
with a patches
section since we have a patches/README.txt
. No reason to state the same information in more than one place.
Also, I dislike
echo >&2 "fooooooooooooooo" \ "barrrrrrrrrrrrrr"
This is much harder to read than
echo >&2 "fooooooooooooooo" "barrrrrrrrrrrrrr"
comment:37 Changed 11 years ago by
As said before, I don't see much reason to support older versions of PARI. The code would only get more complicated without much gain.
comment:38 follow-up: ↓ 39 Changed 11 years ago by
leif, do you still plan to make a follow-up spkg? I don't mind making a new spkg myself based on your suggestions, I just need to know how to move ahead.
comment:39 in reply to: ↑ 38 ; follow-up: ↓ 40 Changed 11 years ago by
Replying to jdemeyer:
leif, do you still plan to make a follow-up spkg? I don't mind making a new spkg myself based on your suggestions, I just need to know how to move ahead.
I'll look at it right now, as I don't exactly recall what the remaining changes were.
I still don't think we should break Lcalc compatibility with older PARI versions for no reason; the change to make it work with both is quite trivial, and we should submit it upstream anyway.
comment:40 in reply to: ↑ 39 ; follow-up: ↓ 41 Changed 11 years ago by
Replying to leif:
I'll look at it right now, as I don't exactly recall what the remaining changes were.
Oh, it seems all my changes are already committed (July 30th btw.), unfortunately all in yet another p8. I'll try to separate them into a p9, then you can make whatever changes you want (e.g. reverting the removal of overlong lines), and provide a p10.
IMHO it's very convenient to have at least an overview of the patches in SPKG.txt
; IIRC, I spent quite some time recovering and documenting all the changes (mentioning just those that are still current) made over time on various tickets, of which at least some weren't referenced from SPKG.txt
/ the changelog, nor commit messages. (P.S.: Just looked into patches/
, there's no README.patches
at all.)
comment:41 in reply to: ↑ 40 ; follow-up: ↓ 43 Changed 11 years ago by
Replying to leif:
P.S.: Just looked into
patches/
, there's noREADME.patches
at all.)
My apologies, I got confused with the pari spkg, which does have a patches/README
.
comment:42 Changed 11 years ago by
Replying to leif:
[...] This does not yet fix the hardcoded
g++
[...]
This seems to still apply.
I guess this was what was delaying my spkg, i.e., why I didn't upload it.
[...] eventually along with a p9 spkg with the other two superfluous upstream files deleted. [...]
Not sure about that, but I presumably already did; will check this.
comment:43 in reply to: ↑ 41 Changed 11 years ago by
comment:44 Changed 11 years ago by
- Milestone changed from sage-4.7.2 to sage-4.7.3
comment:45 Changed 11 years ago by
- Description modified (diff)
- Owner changed from tbd to jdemeyer
New spkg containing most (not all) of leif's changes: http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-1.23.p9.spkg
Needs review.
Changed 11 years ago by
Changed 11 years ago by
comment:46 Changed 11 years ago by
comment:47 Changed 11 years ago by
- Description modified (diff)
Looks good to me. Are you making a full lcalc spkg at one point?