Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#9775 closed defect (duplicate)

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

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

Status badges

Description


Attachments (1)

trac_9775_spkg-lcalc_cygwin_dll.patch (1.5 KB) - added by mpatel 11 years ago.
SPKG patch of Mike's changes.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 11 years ago by mhansen

  • Authors set to Mike Hansen
  • Status changed from new to needs_review

comment:2 Changed 11 years ago by mhansen

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

Changed 11 years ago by mpatel

SPKG patch of Mike's changes.

comment:3 follow-up: Changed 11 years ago by jdemeyer

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 11 years ago by leif

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 11 years ago by leif

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
    else
        mkdir $SAGE_LOCAL/include/lcalc
        cp ../include/* $SAGE_LOCAL/include/lcalc
    fi
    
    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 libmpir.so (or its static version), but - if at all - libgmp.so 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 libmpir.so to libgmp.so.

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 11 years ago by leif

  • Cc rishi added

comment:7 Changed 11 years ago by mhansen

  • Milestone changed from sage-4.5.3 to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Status changed from needs_review to closed

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 11 years ago by leif

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

Note: See TracTickets for help on using tickets.