Opened 11 years ago

Closed 10 years ago

#9845 closed defect (fixed)

lcalc doesn't build on cygwin due to missing time.h include

Reported by: was Owned by: GeorgSWeber
Priority: major Milestone: sage-4.6
Component: build Keywords:
Cc: rishi, leif, jdemeyer Merged in: sage-4.6.alpha2
Authors: Jeroen Demeyer, Mike Hansen, William Stein Reviewers: Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

One needs to add

#include <time.h> 

to include/Lcommandline_numbertheory.h to get lcalc to build on cygwin.


Final spkg: http://sage.math.washington.edu/home/jdemeyer/spkg/lcalc-20100428-1.23.p5.spkg

Attachments (3)

lcalc-20100428-1.23.p2-p4.diff (23.7 KB) - added by leif 11 years ago.
Diff between the p2 (#9592) and Mike's p4 spkg - for review/reference.
lcalc-20100428-1.23.p2-p4.2.diff (1 bytes) - added by jdemeyer 10 years ago.
ignore this file
lcalc-20100428-1.23.p4-p5.diff (37.5 KB) - added by jdemeyer 10 years ago.
Diff between p4 and p5 - for review/reference.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 11 years ago by was

  • Status changed from new to needs_review

comment:3 Changed 11 years ago by mhansen

  • Authors set to Mike Hansen, William Stein
  • Cc rishi leif jdemeyer added

I've posted a new spkg at http://sage.math.washington.edu/home/mhansen/lcalc-20100428-1.23.p4.spkg which contains some of the fixes pointed out at #9775

comment:4 Changed 11 years ago by leif

Thanks for fixing much of what I mentioned at #9775.

I'm not sure if we should drop the usual "sanity check" at the top of spkg-install. (Btw, you missed the "p2" in its top comment.)

The first comment regarding SAGE_DEBUG and the message in the else part aren't current. ("Unset SAGE_DEBUG ..." should read "Set SAGE_DEBUG to yes ...". Though I'd prefer adding -g independently of that setting, since it doesn't influence performance, and most other spkgs do so.)

Replacing -lmpir by -lgmp is perhaps a minor issue; using $MAKE instead of make ($(MAKE) in Lcalc's Makefile itself, too) IMHO not. Unfortunately, the current spkg/install also uses make instead of $MAKE unless SAGE_PARALLEL_SPKG_BUILD=yes, which I consider a bug, since adding -j is not the only case in which one would set MAKE. One should never call make inside a Makefile or a script that's executed through some "make", because it might be a different one.

comment:5 Changed 11 years ago by leif

P.S.:

all:
#       make print_vars
        make libLfunction.so
        make lcalc
        make examples
#       make find_L
#       make test

should simply (for our purposes) be

all:    libLfunction.so lcalc examples

or even just

all:    lcalc examples

but one has to add the proper dependency, i.e.:

examples: libLfunction.so
        $(CC) $(CCFLAGS) $(INCLUDEFILES) example_programs/example.cc libLfunction.so -o example_programs/example $(GMP_FLAGS)

comment:6 Changed 11 years ago by leif

Btw, the -lmpir (or -lgmp) is only needed if we build a static version of lcalc, since only the also linked PARI library directly uses it.

Changed 11 years ago by leif

Diff between the p2 (#9592) and Mike's p4 spkg - for review/reference.

comment:7 Changed 11 years ago by leif

I've attached a diff between the lcalc-20100428-1.23.p2 spkg from #9592 and Mike's / William's lcalc-20100428-1.23.p4 for easier reviewing.

Note that #9592 also still needs review! (We need this modified package for upgrading PARI / Sage 4.6.)

comment:8 Changed 11 years ago by leif

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

comment:9 Changed 11 years ago by jdemeyer

  • Description modified (diff)

comment:10 follow-up: Changed 10 years ago by mpatel

Mike, are there Windows virtual machines running Cygwin available to potential reviewers? (We could give access instructions off-trac.) It would be great to reduce the number of remaining build tickets at CygwinPort. And remote Cygwin access might help with #9914 (see comment 4ff).

comment:11 in reply to: ↑ 10 Changed 10 years ago by jdemeyer

Replying to mpatel:

Mike, are there Windows virtual machines running Cygwin available to potential reviewers? (We could give access instructions off-trac.)

I wouldn't mind testing this if needed.

comment:12 Changed 10 years ago by jdemeyer

  • Description modified (diff)

I have made a trivial change to spkg-install (removed the comment "# since 'success' relies on an exit code, call set +e before running it."). I also removed the dist directory.

New spkg: http://sage.math.washington.edu/home/jdemeyer/spkg/lcalc-20100428-1.23.p4.spkg

comment:13 Changed 10 years ago by jdemeyer

  • Authors changed from Mike Hansen, William Stein to Jeroen Demeyer, Mike Hansen, William Stein
  • Description modified (diff)
  • Reviewers set to Leif Leonhardy

I also made the other changes suggested by Leif. New spkg: http://sage.math.washington.edu/home/jdemeyer/spkg/lcalc-20100428-1.23.p5.spkg

Changed 10 years ago by jdemeyer

ignore this file

Changed 10 years ago by jdemeyer

Diff between p4 and p5 - for review/reference.

comment:14 Changed 10 years ago by leif

Looks as if we no longer need patches/Lcommandline_elliptic.cc.cygwin (and patches/Lcommandline_elliptic.cc.cygwin.diff), since this patch isn't applied.

Mike (or Jeroen, if you can test this), is this Cygwin patch / addition obsolete:

// SAGE -- used below -- needed for Cygwin.
#ifndef llrint
inline long long int llrint (double x)
{
    long long int llrintres;
    asm
    ("fistpll %0"
    : "=m" (llrintres) : "t" (x) : "st");
    return llrintres;
}
#endif

(It's not included in the generic patch to Lcommandline_elliptic.cc, nor upstream.)

Apart from old typos, an obsolete comment regarding SAGE_DEBUG, and the recent changelog headings lacking the upstream (snapshot?) date (20100428), I'm quite happy with the new spkg (i.e., the Sage part; the patched Makefile is still suboptimal, but never mind). :)

If this spkg really contains an upstream snapshot, it is unclear from SPKG.txt when this was taken / actually put into the spkg.

comment:15 follow-up: Changed 10 years ago by leif

  • Status changed from needs_review to positive_review

The new p5 spkg successfully installed on Sage 4.6.alpha1 (with MAKE="make -j32") and passed ptestlong on Ubuntu 10.04 x86_64, so I'm setting this to "positive review".

Feel free to revert this in case any errors occur on other systems.

comment:16 in reply to: ↑ 15 Changed 10 years ago by leif

Replying to leif:

The new p5 spkg successfully installed on Sage 4.6.alpha1 (with MAKE="make -j32") and passed ptestlong on Ubuntu 10.04 x86_64, [...]

Same on Ubuntu 9.04 x86 (with MAKE="make -j8").

comment:17 Changed 10 years ago by mpatel

  • Merged in set to sage-4.6.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.