Make lcalc compatible with the new PARI

[jdemeyer]

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.

[vbraun]

Looks good to me. Are you making a full lcalc spkg at one point?

[jdemeyer]

Replying to vbraun:

Looks good to me. Are you making a full lcalc spkg at one point?

Yes, when I have time.

[jdemeyer]

New spkg ready for review, needs to be built with the new PARI spkg.

[jdemeyer]

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

[leif]

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.

[leif]

spkg-install:

• The comment and a message regarding SAGE_DEBUG is not consistent. (SAGE_DEBUG has to be set to yes 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 within CC 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-alone lcalc 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.

[jdemeyer]

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.

[leif]

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).

[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.

[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. Basing a new spkg on a not-yet-positively-reviewed skpg is not a very good idea anyway (see #11605 vs #11130).

[leif]

From first glance:

• (GNU) patch should be added to the dependencies section.
• 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.)
• There's neither a patches/README.txt nor a Patches section in SPKG.txt.
• success 'install' should perhaps be success '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.

[leif]

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).

[leif]

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.)

[leif]

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 removes foo 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.

[leif]

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.

[leif]

P.P.S.: src/include/Lvalue.h.bak is still present in the p8.

[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.

[leif]

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.

[jdemeyer]

New spkg (even with changes committed), same location: ​http://boxen.math.washington.edu/home/jdemeyer/spkg/lcalc-1.23.p8.spkg

[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 in SPKG.txt.
  
• success 'install' should perhaps be success '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.

[leif]

Just found some more things:

• src/src/Makefile.old can also be deleted.

• The Makefile not only defines CC to g++, but also hardcodes g++ when linking the shared library.

[leif]

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.)

[leif]

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.

[leif]

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 in SPKG.txt.
  
• success 'install' should perhaps be success '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.

[leif]

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".

[leif]

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.

[jdemeyer]

Leif, will you post a new spkg with your changes added?

[jdemeyer]

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.

[leif]

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... ;-)

[leif]

Replying to leif:

Replying to jdemeyer:

Replying to leif: Leif, will you post a new spkg with your changes added?

Yes, later today.

"Today" still applies, only it's Wednesday now... ;-)

[jdemeyer]

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"

[jdemeyer]

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.

[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.

[leif]

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.

[leif]

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.)

[jdemeyer]

Replying to leif:

P.S.: Just looked into patches/, there's no README.patches at all.)

My apologies, I got confused with the pari spkg, which does have a patches/README.

[leif]

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.

[leif]

Replying to jdemeyer:

Replying to leif:

P.S.: Just looked into patches/, there's no README.patches at all.)

I got confused with the pari spkg, which does have a patches/README.

Me too... ;-)

[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.