Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#9775 closed defect (duplicate)

lcalc should make a .dll file on Cygwin instead of .so file

Reported by: Mike Hansen Owned by: tbd
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: porting: Cygwin Keywords:
Cc: Rishikesh Merged in:
Authors: Mike Hansen Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges


Attachments (1)

trac_9775_spkg-lcalc_cygwin_dll.patch (1.5 KB) - added by Mitesh Patel 12 years ago.
SPKG patch of Mike's changes.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 12 years ago by Mike Hansen

Authors: Mike Hansen
Status: newneeds_review

comment:2 Changed 12 years ago by Mike Hansen

Note that #9592 should be closed or have a positive review before this one does.

Changed 12 years ago by Mitesh Patel

SPKG patch of Mike's changes.

comment:3 Changed 12 years ago by Jeroen Demeyer

There is a small type in SPKG.txt: on line 29, you should replace "lcalc-1.23.p2" by "lcalc-1.23.p3"

comment:4 in reply to:  3 Changed 12 years ago by Leif Leonhardy

Replying to jdemeyer:

There is a small type in SPKG.txt: on line 29, you should replace "lcalc-1.23.p2" by "lcalc-1.23.p3"

Also in the top comment of spkg-install.

comment:5 Changed 12 years ago by Leif Leonhardy

Some comments (on the spkg in general, not the Cygwin changes, with the exception of quoting $SAGE_LOCAL):

  • $CFLAG64 and $CXXFLAG64 should be quoted.
  • CXXFLAG64 is exported (?) twice. (It is in fact currently used in the Makefile.)
  • In general, e.g. -m64 should be added to CPPFLAGS as well.
  • In other packages, we disable optimization if SAGE_DEBUG=yes, and build with debugging symbols (-g) unconditionally, i.e. independent of the setting of SAGE_DEBUG.
  • $MAKE should be used instead of make. (Though make is called(!) inside the Makefile itself for the default target, all, which we build. See below, too.)
  • $SAGE_LOCAL should be quoted, too (for future support of spaces).
  • The following case distinction is superfluous (and the branches are redundant as well):
    if `test -d $SAGE_LOCAL/include/lcalc`; then
        rm -fr $SAGE_LOCAL/include/lcalc
        mkdir $SAGE_LOCAL/include/lcalc
        cp ../include/* $SAGE_LOCAL/include/lcalc
        mkdir $SAGE_LOCAL/include/lcalc
        cp ../include/* $SAGE_LOCAL/include/lcalc
    It should simply be:
        rm -fr "$SAGE_LOCAL"/include/lcalc
        mkdir -p "$SAGE_LOCAL"/include/lcalc
        cp ../include/* "$SAGE_LOCAL"/include/lcalc
  • I'm not sure if I should like the success() function (the messages are quite strange); same for the use of set -e.
  • There's no spkg-check, but unfortunately the test program has been removed from the sources anyway. Should be addressed in later versions (e.g. add a comment to "Special Update/Build? Instructions").
  • These files have been removed without telling Mercurial:
    $ hg status
    ! patches/lcalc-1.11-constification+solaris.patch
    ! patches/lcalc-1.11-gcc-4.3-build.patch
    ! patches/lcalc-1.11-memleak-fixes.patch
  • The patched Makefile (patches/Makefile.sage, lacking the corresponding diff) isn't much better than the original. It also should not make Lcalc link against (or its static version), but - if at all - instead, since we configure MPIR with --enable-gmpcompat anyway. As is, it's the only package that breaks building with GNU MP, unless one creates a symbolic link from to

It would be nice to address at least some (especially the last) of these points here, too, since it IMHO doesn't make much sense to frequently open new tickets and create new spkgs just for minor/clean-up changes.

comment:6 Changed 12 years ago by Leif Leonhardy

Cc: Rishikesh added

comment:7 Changed 12 years ago by Mike Hansen

Milestone: sage-4.5.3sage-duplicate/invalid/wontfix
Resolution: duplicate
Status: needs_reviewclosed

I'm going to close this as a duplicate of #9845. I'll post an spkg there which contains some changes from leif's review.

comment:8 Changed 12 years ago by Leif Leonhardy

Also, the dist/ (Debian) directory should be removed, cf. #5903.

Note: See TracTickets for help on using tickets.